| Message ID | 20240913145655.10076-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/swiotlb: add alignment check for dma buffers | expand |
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
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.
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
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;
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(+)