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

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

[Juergen Gross]

When checking a memory buffer to be consecutive in machine memory,
the alignment needs to be checked, too. Failing to do so might result
in DMA memory not being aligned according to its requested size,
leading to error messages like:

  4xxx 0000:2b:00.0: enabling device (0140 -> 0142)
  4xxx 0000:2b:00.0: Ring address not aligned
  4xxx 0000:2b:00.0: Failed to initialise service qat_crypto
  4xxx 0000:2b:00.0: Resetting device qat_dev0
  4xxx: probe of 0000:2b:00.0 failed with error -14

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/swiotlb-xen.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 35155258a7e2..11f4b1195324 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);
+	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)) &&
+	    !IS_ALIGNED(next_bfn, 1UL << order))
+		return 1;
+
 	for (i = 1; i < nr_pages; i++)
 		if (pfn_to_bfn(++xen_pfn) != ++next_bfn)
 			return 1;
-- 
2.43.0

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

[Jan Beulich]

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.

Unrelated to this, but in related code: xen_swiotlb_alloc_coherent()
can't validly use XEN_PAGE_SHIFT, can it (in the way it does at
least)? If XEN_PAGE_SHIFT < PAGE_SHIFT, and with order calculated
from the latter, the used size will be too small afaict.

Jan

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

[Stefano Stabellini]

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.

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

[Jan Beulich]

On 14.09.2024 02:38, Stefano Stabellini wrote:
> 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.

Whereas I would be inclined to suggest to put it in the one place it's
needed, not the least to avoid the abuse of the function (going just
from its name).

> 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.

Hmm, right.

Jan