Re: [Xen-devel] [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 7 Jan 2019, Paul Durrant wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> > Sent: 04 January 2019 17:46
> > To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> > Cc: 'Stefano Stabellini' <sstabellini@xxxxxxxxxx>; Andrew Cooper
> > <Andrew.Cooper3@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxx;
> > julien.grall@xxxxxxx; jbeulich@xxxxxxxx
> > Subject: RE: [Xen-devel] [PATCH for-4.12] xen/iommu: fix dev assignment on
> > ARM after 91d4eca7
> > 
> > On Fri, 4 Jan 2019, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Stefano Stabellini [mailto:sstabellini@xxxxxxxxxx]
> > > > Sent: 03 January 2019 18:20
> > > > To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
> > > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> > devel@xxxxxxxxxxxxx;
> > > > julien.grall@xxxxxxx; jbeulich@xxxxxxxx; Paul Durrant
> > > > <Paul.Durrant@xxxxxxxxxx>
> > > > Subject: Re: [Xen-devel] [PATCH for-4.12] xen/iommu: fix dev
> > assignment on
> > > > ARM after 91d4eca7
> > > >
> > > > On Thu, 3 Jan 2019, Andrew Cooper wrote:
> > > > > On 03/01/2019 00:28, Stefano Stabellini wrote:
> > > > > > Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
> > > > > > need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
> > > > > >
> > > > > > arch_iommu_populate_page_table returns -ENOSYS which causes
> > > > > > iommu_construct to return early, although it is not an error.
> > > > > >
> > > > > > Interestingly, the if ( rc ) was present even before 91d4eca7, but
> > it
> > > > was
> > > > > > still working before. Now, with the new hd->status field it won't
> > > > > > complete the initialization successfully.
> > > > > >
> > > > > > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > > > > >
> > > > > > diff --git a/xen/drivers/passthrough/iommu.c
> > > > b/xen/drivers/passthrough/iommu.c
> > > > > > index ac62d7f..a63329b 100644
> > > > > > --- a/xen/drivers/passthrough/iommu.c
> > > > > > +++ b/xen/drivers/passthrough/iommu.c
> > > > > > @@ -269,7 +269,7 @@ int iommu_construct(struct domain *d)
> > > > > >          hd->need_sync = true;
> > > > > >
> > > > > >          rc = arch_iommu_populate_page_table(d);
> > > > >
> > > > > The comment in ARM's arch_iommu_populate_page_table() says
> > > > >
> > > > > /* The IOMMU shares the p2m with the CPU */
> > > > >
> > > > > Which means that iommu_use_hap_pt() (just out of context above this
> > > > > hunk) is wrong.  It should return true, which will prevent us
> > entering
> > > > > this path during initialisation.
> > > >
> > > > iommu_use_hap_pt is implemented by calling has_iommu_pt(d) (also on
> > x86
> > > > is part of the implementation), which is implemented as:
> > > >
> > > >   (dom_iommu(d)->status != IOMMU_STATUS_disabled)
> > > >
> > > > IOMMU_STATUS_disabled is zero in the enum, so status starts as
> > > > IOMMU_STATUS_disabled. It is set to IOMMU_STATUS_initializing for dom0
> > > > in iommu_hwdom_init, but it is not set to IOMMU_STATUS_initializing
> > for
> > > > other domains anywhere, leading to this error.
> > >
> > > It is set for other domains... see iommu_construct().
> > 
> > Hi Paul, unfortunately it doesn't work that way. Let me explain:
> > 
> > 
> > iommu_construct() sets hd->status to IOMMU_STATUS_initializing *after*
> > calling iommu_use_hap_pt(d), and specifically inside the if statement:
> > 
> >     if ( !iommu_use_hap_pt(d) )
> >     {
> >         int rc;
> > 
> >         hd->status = IOMMU_STATUS_initializing;
> >         hd->need_sync = true;
> > 
> >         rc = arch_iommu_populate_page_table(d);
> >         if ( rc )
> >         {
> >             if ( rc != -ERESTART )
> >             {
> >                 hd->need_sync = false;
> >                 hd->status = IOMMU_STATUS_disabled;
> >             }
> > 
> >             return rc;
> >         }
> >     }
> > 
> > However, for iommu_use_hap_pt(d) to return the correct value, which
> > should be always true on ARM, status needs to be already set to
> > IOMMU_STATUS_initializing.
> > 
> > Because when we enter iommu_construct status is IOMMU_STATUS_disabled,
> > iommu_use_hap_pt(d) calls has_iommu_pt(d) which wrongly returns false
> > because it is implemented as:
> > 
> > #define has_iommu_pt(d) (dom_iommu(d)->status != IOMMU_STATUS_disabled)
> > 
> > 
> > I think we need to move the initialization of status to
> > IOMMU_STATUS_initializing earlier, specifically to iommu_domain_init.
> 
> The use of iommu_use_hap_pt() here is indeed a problem, but I think it would 
> be sufficient to move the line "hd->status = IOMMU_STATUS_initializing" to 
> just before the if rather than to a completely separate function.

Yes, that works too.
 
---
xen/iommu: fix dev assignment on ARM after 91d4eca7

Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".

arch_iommu_populate_page_table returns -ENOSYS which causes
iommu_construct to return early, although it is not an error.

hd->status needs to be set to IOMMU_STATUS_initializing before calling
iommu_use_hap_pt, otherwise iommu_use_hap_pt will return the wrong
value.

Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index ac62d7f..a6f69f4 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -261,11 +261,11 @@ int iommu_construct(struct domain *d)
     if ( hd->status == IOMMU_STATUS_initialized )
         return 0;
 
+    hd->status = IOMMU_STATUS_initializing;
     if ( !iommu_use_hap_pt(d) )
     {
         int rc;
 
-        hd->status = IOMMU_STATUS_initializing;
         hd->need_sync = true;
 
         rc = arch_iommu_populate_page_table(d);

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