Re: [PATCH v3 0/3] Generic SMMU Bindings

[Rahul Singh <Rahul.Singh@arm.com>]

HI Stefano,

> On 13 Apr 2021, at 1:27 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Mon, 12 Apr 2021, Rahul Singh wrote:
>> Hi Stefano,
>> 
>>> On 10 Apr 2021, at 1:27 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Tue, 6 Apr 2021, Stefano Stabellini wrote:
>>>> On Mon, 5 Apr 2021, Julien Grall wrote:
>>>>> On 26/01/2021 22:58, Stefano Stabellini wrote:
>>>>>> Hi all,
>>>>> 
>>>>> Hi Stefano,
>>>>> 
>>>>>> This series introduces support for the generic SMMU bindings to
>>>>>> xen/drivers/passthrough/arm/smmu.c.
>>>>>> 
>>>>>> The last version of the series was
>>>>>> https://marc.info/?l=xen-devel&m=159539053406643
>>>>> Some changes in the SMMU drivers went in recently. I believe this touched
>>>>> similar area to this series. Would you be able to check that this series still
>>>>> work as intented?
>>>> 
>>>> Thanks for the heads up, no, unfortunately they don't work :-(
>>>> 
>>>> They badly clash. I did the forward port of the three patches but they
>>>> fail at runtime in my tests. I ran out of time to figure out what is the
>>>> problem, and I'll have to pick it up at some point in the future (it
>>>> might not be any time soon unfortunately).
>>>> 
>>>> Rahul, if you have any ideas about what the problem is please let me
>>>> know. This is the branch with the forward port:
>>>> 
>>>> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic
>>> 
>>> I did some more investigation and spotted a minor error in my forward
>>> port. This an updated branch based on staging:
>>> 
>>> http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git smmu-generic-2
>>> 
>>> However, the real issue is that Rahul's patches break SMMU support on
>>> ZynqMP even without my changes. I ran a bisection and found out that
>>> patch #2 is the culprit:
>>> 
>>> 5ee3fa0b21ea xen/arm: smmuv1: Consolidate stream map entry state
>>> 
>>> It causes the abort appended below. The problem doesn't seem
>>> particularly ZynqMP specific. Rahul, can you reproduce it on your side?
>> 
>> Yes. I can reproduce the issue on Xilinx QEMU as we don’t have access to physical ZynqMP and found out that
>> associating an iommu group pointer with the S2CR causing the issue.
>> 
>> Associating the group pointer with S2CR is part of the patch "xen/arm: smmuv1: Intelligent SMR allocation”.
>> 
>> I just revert that part of the code from the patch and it works fine for me. Please find the attached patch for the same.
>> 
>> As per your analysis "5ee3fa0b21ea xen/arm: smmuv1: Consolidate stream map entry state” is causing the issue but what I found out that
>> "xen/arm: smmuv1: Intelligent SMR allocation” is causing the issue.
>> Can you please test it on the physical device and let me know if it works for you also to make sure we both observing the same issue.
> 
> Great! Yes, I can confirm that your patch fixed the issue, now I can
> boot staging on ZynqMP without errors and I can do device assignment
> too. Thank you so much!
> 
> The other good news is that the three "Generic SMMU Bindings" patches
> work too on top of yours with the fix!
> 
> Is the patch you submitted the valid fix for the problem? In other words,
> should we go ahead, review, and commit the patch you attached or do you
> want to send a different version of the patch for inclusion in Xen
> staging?

Thank you for testing the patch. Patch as it is ok for review. I will send the patch for review.

Regards,
Rahul