Re: [PATCH v2 1/2] xen/swiotlb: add alignment check for dma buffers

[Jürgen Groß]

On 29.11.24 18:36, Thierry Escande wrote:
> Hi,
>
> On 16/09/2024 08:47, Juergen Gross wrote:
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 35155258a7e2..ddf5b1df632e 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -78,9 +78,15 @@ static inline int range_straddles_page_boundary(phys_addr_t p, size_t size)
> >   {
> >   	unsigned long next_bfn, xen_pfn = XEN_PFN_DOWN(p);
> >   	unsigned int i, nr_pages = XEN_PFN_UP(xen_offset_in_page(p) + size);
> > +	phys_addr_t algn = 1ULL << (get_order(size) + PAGE_SHIFT);
> >   
> >   	next_bfn = pfn_to_bfn(xen_pfn);
> >   
> > +	/* If buffer is physically aligned, ensure DMA alignment. */
> > +	if (IS_ALIGNED(p, algn) &&
> > +	    !IS_ALIGNED(next_bfn << XEN_PAGE_SHIFT, algn))
> > +		return 1;
>
> There is a regression in the mpt3sas driver because of this change.
> When, in a dom0, this driver creates its DMA pool at probe time and
> calls dma_pool_zalloc() (see [1]), the call to
> range_straddles_page_boundary() (from xen_swiotlb_alloc_coherent())
> returns 1 because of the alignment checks added by this patch. Then the
> call to xen_create_contiguous_region() in xen_swiotlb_alloc_coherent()
> fails because the passed size order is too big (> MAX_CONTIG_ORDER).
> This driver sets the pool allocation block size to 2.3+ MBytes.
>
>  From previous discussions on the v1 patch, these checks are not
> necessary from xen_swiotlb_alloc_coherent() that already ensures
> alignment, right?

It ensures alignment regarding guest physical memory, but it doesn't do
so for machine memory.

For DMA machine memory proper alignment might be needed, too, so the
check is required. As an example, some crypto drivers seem to rely on
proper machine memory alignment, which was the reason for those checks
to be added.

Possible solutions include:

- rising the MAX_CONTIG_ORDER limit (to which value?)
- adding a way to allocate large DMA buffers with relaxed alignment
  requirements (this will impact the whole DMA infrastructure plus
  drivers like mp3sas which would need to use the new interface)
- modify the mpt3sas driver to stay within the current limits
- only guarantee proper machine memory alignment up to MAX_CONTIG_ORDER
  (risking to introduce hard to diagnose bugs for the rare use cases of
  such large buffers)


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature