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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 12 Apr 2021, Rahul Singh wrote:
> Hi Stefano,
> 
> > On 10 Apr 2021, at 1:27 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> > 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?