Re: [PATCH 07/10] crypto: Use ARCH_DMA_MINALIGN instead of ARCH_KMALLOC_MINALIGN

[Ard Biesheuvel]

On Thu, 14 Apr 2022 at 17:01, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Thu, 14 Apr 2022 at 16:53, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Apr 14, 2022 at 04:36:46PM +0200, Ard Biesheuvel wrote:
> > > On Thu, 14 Apr 2022 at 16:27, Greg Kroah-Hartman
> > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Apr 14, 2022 at 03:52:53PM +0200, Ard Biesheuvel wrote:
> ...
> > > > > What we might do, given the fact that only inbound non-cache coherent
> > > > > DMA is problematic, is dropping the kmalloc alignment to 8 like on
> > > > > x86, and falling back to bounce buffering when a misaligned, non-cache
> > > > > coherent inbound DMA mapping is created, using the SWIOTLB bounce
> > > > > buffering code that we already have, and is already in use on most
> > > > > affected systems for other reasons (i.e., DMA addressing limits)
> > > >
> > > > Ick, that's a mess.
> > > >
> > > > > This will cause some performance regressions, but in a way that seems
> > > > > fixable to me: taking network drivers as an example, the RX buffers
> > > > > that are filled using inbound DMA are typically owned by the driver
> > > > > itself, which could be updated to round up its allocations and DMA
> > > > > mappings. Block devices typically operate on quantities that are
> > > > > aligned sufficiently already. In other cases, we will likely notice
> > > > > if/when this fallback is taken on a hot path, but if we don't, at
> > > > > least we know a bounce buffer is being used whenever we cannot perform
> > > > > the DMA safely in-place.
> > > >
> > > > We can move to having an "allocator-per-bus" for memory like this to
> > > > allow the bus to know if this is a DMA requirement or not.
> > > >
> > > > So for all USB drivers, we would have:
> > > >         usb_kmalloc(size, flags);
> > > > and then it might even be easier to verify with static tools that the
> > > > USB drivers are sending only properly allocated data.  Same for SPI and
> > > > other busses.
> > > >
> > >
> > > As I pointed out earlier in the thread, alignment/padding requirements
> > > for non-coherent DMA are a property of the CPU's cache hierarchy, not
> > > of the device. So I'm not sure I follow how a per-subsystem
> > > distinction would help here. In the case of USB especially, would that
> > > mean that block, media and networking subsystems would need to be
> > > aware of the USB-ness of the underlying transport?
> >
> > That's what we have required today, yes.  That's only because we knew
> > that for some USB controllers, that was a requirement and we had no way
> > of passing that information back up the stack so we just made it a
> > requirement.
> >
> > But I do agree this is messy.  It's even messier for things like USB
> > where it's not the USB device itself that matters, it's the USB
> > controller that the USB device is attached to.  And that can be _way_ up
> > the device hierarchy.  Attach something like a NFS mount over a PPP
> > network connection on a USB to serial device and ugh, where do you
> > begin?  :)
> >
>
> Exactly.
>
> > And is this always just an issue of the CPU cache hierarchy?  And not the
> > specific bridge that a device is connected to that CPU on?  Or am I
> > saying the same thing here?
> >
>
> Yes, this is a system property not a device property, and the driver
> typically doesn't have any knowledge of this. For example, if a PCI
> host bridge happens to be integrated in a non-cache coherent way, any
> PCI device plugged into it becomes non-coherent, and the associated
> driver needs to do the right thing. This is why we rely on the DMA
> layer to take care of this.
>
> > I mean take a USB controller for example.  We could have a system where
> > one USB controller is on a PCI bus, while another is on a "platform"
> > bus.  Both of those are connected to the CPU in different ways and so
> > could have different DMA rules.  Do we downgrade everything in the
> > system for the worst connection possible?
> >
>
> No, we currently support a mix of coherent and non-coherent just fine,
> and this shouldn't change. It's just that the mere fact that
> non-coherent devices might exist is increasing the memory footprint of
> all kmalloc allocations.
>
> > Again, consider a USB driver allocating memory to transfer stuff, should
> > it somehow know the cache hierarchy that it is connected to?  Right now
> > we punt and do not do that at the expense of a bit of potentially
> > wasted memory for small allocations.
> >
>
> This whole discussion is based on the premise that this is an expense
> we would prefer to avoid. Currently, every kmalloc allocation is
> rounded up to 128 bytes on arm64, while x86 uses only 8.

I guess I didn't answer that last question. Yes, I guess dma_kmalloc()
should be used in such cases. Combined with my bounce buffering hack,
the penalty for using plain kmalloc() instead would be a potential
performance hit when used for inbound DMA, instead of data corruption
(if we'd reduce the kmalloc() alignment when introducing
dma_kmalloc())