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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 13 Sep 2024, Jan Beulich wrote:
> On 13.09.2024 16:56, Juergen Gross wrote:
> > --- 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);
> > +   unsigned int order = get_order(size);
> >  
> >     next_bfn = pfn_to_bfn(xen_pfn);
> >  
> > +   /* If buffer is physically aligned, ensure DMA alignment. */
> > +   if (IS_ALIGNED(p, 1UL << (order + PAGE_SHIFT)) &&
> 
> Why this check? xen_swiotlb_alloc_coherent() guarantees it, while
> xen_swiotlb_free_coherent() only checks properties of the original
> allocation. And for xen_swiotlb_map_page() this looks actively
> wrong to me, in case that function was called with offset non-zero.

I understand xen_swiotlb_alloc_coherent and xen_swiotlb_free_coherent
not needing the check, but I think we might need the check for
xen_swiotlb_map_page. At that point, I would keep the check for all
callers. Unless there is another way to detect whether the mapping needs
alignment specifically for map_page?

For the offset, in theory if the device needs alignment, the offset
should be zero? If the offset is not zero, then there should be no
alignment requirement. The way Juergen wrote the check, we would take
the fast path if offset != zero, which makes sense to me.