RE: [PATCH 2/2] ntb_hw_switchtec: Check for alignment of the buffer in mw_set_trans()

[Allen Hubbe]

From: Logan Gunthorpe
> On 11/12/17 12:17 PM, Allen Hubbe wrote:
> >> mw_get_align doesn't communicate the fact that the buffer has to be
> >> aligned by its size.
> >
> > Is that not the purpose of the addr_align out parameter of ntb_mw_get_align()?
> 
> addr_align provides the minimum alignment required by the device but it
> has no idea how big a buffer the caller is trying to create so it can't
> express that it needs to be aligned by its size.
> 
> To be clear, the minimum alignment the Switchtec device requires is 4KB
> so it will return 4k in addr_align. Thus, if you have a 4KB buffer it
> may be aligned to 4KB. But if you have a 1MB buffer it must be aligned
> to the nearest 1M.

In switchtec_ntb_mw_get_align, for the lut case it seems to require alignment the same as Intel, aligned to mw size, but for the non-lut case you are saying that SZ_4K is not necessarily correct.  The SZ_4K is the minimum, but the actual alignment restriction depends on the size of the buffer actually translated.  Right?

Also, for the lut case, it looks like the size also has to be the same size as the mw.  So, a client can't allocate a smaller buffer, assume we can get one that is aligned, point the start of the mw at it, and limit the size of the mw?

For the non-lut case I wonder, with the restriction that addr needs to be aligned to the size of the buffer, does the size of the buffer also need to be some power of two?  That would make sense, if it determines the alignment.  If so, SZ_4K wouldn't be correct for size_align, either.

Do you need the intended buffer size passed in as another parameter to ntb_mw_get_align?  The point of ntb_mw_get_align is to figure out all the alignment restrictions before allocating memory.

> >> It may also be that all hardware does not have this
> >> restriction (ie. if the hardware adds to the base address instead of
> >> just replacing the lower bits).
> >>
> >> There is definitely a need to print this error somewhere as I hit this
> >> case and it caused very weird behavior. It was a huge pain to debug.
> >> Also, it's a security issue and huge bug if we end up mapping the memory
> >> we didn't think we were mapping.
> >
> > Of course the driver should validate its parameters not allow bad mappings.  I was only commenting
> on the dev_err() message to the console.
> 
> Ok. I still feel like it would be difficult to debug if ntb_transport
> simply was unable to establish a connection without some message in
> dmesg telling the user why.
> 
> Also, keep in mind this is a somewhat unusual occurrence. In most cases
> dma_alloc_coherent() always provides a buffer that is aligned to it's
> size. It's just that the CMA (if used) provides a tunable config option
> which allows for larger buffers to not be aligned to their size.
> 
> Logan