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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 03 January 2019 18:20
> To: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xen.org;
> julien.grall@arm.com; jbeulich@suse.com; Paul Durrant
> <Paul.Durrant@citrix.com>
> 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@xilinx.com>
> > >
> > > 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().

  Paul

> 
> It looks like we want to move the initialization of status from
> iommu_hwdom_init to iommu_domain_init, which is called for everybody.
> 
> 
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index ac62d7f..7cf6aaf 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -180,6 +180,7 @@ int iommu_domain_init(struct domain *d)
>      if ( !iommu_enabled )
>          return 0;
> 
> +    hd->status = IOMMU_STATUS_initializing;
>      hd->platform_ops = iommu_get_ops();
>      return hd->platform_ops->init(d);
>  }
> @@ -206,7 +207,6 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
> 
>      register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m
> table", 0);
> 
> -    hd->status = IOMMU_STATUS_initializing;
>      hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
>      if ( need_iommu_pt_sync(d) )
>      {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel