Re: [Xen-devel] Question regarding swiotlb-xen in Linux kernel

[Joe Jin <joe.jin@xxxxxxxxxx>]

On 4/18/19 10:47 PM, Juergen Gross wrote:
> On 19/04/2019 00:31, Joe Jin wrote:
>> On 4/18/19 2:09 PM, Boris Ostrovsky wrote:
>>> On 4/18/19 3:36 AM, Juergen Gross wrote:
>>>> I'm currently investigating a problem related to swiotlb-xen. With a
>>>> specific driver a customer is capable to trigger a situation where a
>>>> MFN is mapped to multiple dom0 PFNs at the same time. There is no
>>>> guest involved, so this is not related to grants.
>>>>
>>>> Wit a debug kernel I have managed to track the inconsistency to a
>>>> call of xen_destroy_contiguous_region() from xen_swiotlb_free_coherent()
>>>> where the region was obviously not contiguous.
>>>>
>>>> xen_swiotlb_free_coherent() contains:
>>>>
>>>>         if (((dev_addr + size - 1 <= dma_mask)) ||
>>>>             range_straddles_page_boundary(phys, size))
>>>>                 xen_destroy_contiguous_region(phys, order);
>>>>
>>>> Shouldn't it be either:
>>>>
>>>>         if (((dev_addr + size - 1 <= dma_mask)) &&
>>>>             !range_straddles_page_boundary(phys, size))
>>>>                 xen_destroy_contiguous_region(phys, order);
>>>
>>> +Joe
>>>
>>> https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01920.html
>>>
>>> (The discussion happened in
>>> https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01814.html)
>>>
>>> And looks like we dropped it. Or was there a reason we haven't picked it up?
>>
>> I remembered the concern was whether memory not from Xen.
> 
> The current coding is wrong.

I agree with you, your patch same with I sent before, I'm good to have it.

> 
> I believe we should have at least a WARN_ONCE() in case
> xen_swiotlb_free_coherent() is being called with non-contiguous memory.
> And it should not call xen_destroy_contiguous_region() in that case.
It's potential issue, if alloc/free is not in pairs, it will be a problem.

> 
> Joe, did you observe such cases? I'm asking because the backtraces I
> have give me no clue _why_ the memory was non-contiguous. The calling
> function allocated the memory via xen_swiotlb_alloc_coherent() and when
> freeing it again it was not contiguous.

Not sure if you have commit 7250f422 "xen-swiotlb: use actually allocated 
size on check physical continuous"? without this commit, it's possible,
after apply the commit, we did not hit such kind cases.

Thanks,
Joe
> 
> Another topic is the question whether we should really call
> xen_destroy_contiguous_region() when freeing the memory if there was no
> need to use xen_create_contiguous_region() when allocating it.
> 
> 
> Juergen
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel