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

["Thierry Escande" <thierry.escande@vates.tech>]

On 02/12/2024 09:27, Jürgen Groß wrote:
> 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?)

Looks like the quick and easy solution. Bumping MAX_CONTIG_ORDER to 10
would allow 4MB pools, enough for this particular driver. I'll send a
patch if that sounds reasonable.

Regards,
Thierry

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