Re: [PATCH v5 01/15] iommu: turn need_iommu back into a boolean.

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

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 08 August 2018 14:39
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; George
> Dunlap <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>;
> Stefano Stabellini <sstabellini@kernel.org>; xen-devel <xen-
> devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> Subject: Re: [PATCH v5 01/15] iommu: turn need_iommu back into a
> boolean.
> 
> >>> On 03.08.18 at 19:22, <paul.durrant@citrix.com> wrote:
> > As noted in [1] the tri-state nature of need_iommu after commit 3e06b989
> is
> > confusing.
> >
> > Because arch_iommu_populate_page_table() removes pages from the
> page list
> > as it maps them it is actually safe to re-invoke multiple times without
> > the need for any specific indication it has been called before, so it
> > is actually safe to simply turn need_iommu back into a boolean with no
> > change to the population algorithm.
> >
> > [1] https://lists.xenproject.org/archives/html/xen-devel/2018-
> 07/msg01870.html
> 
> I have to admit that I wouldn't read "confusing" into that mail. And
> given the below, I continue to wonder whether you really, really
> need to change this.

Ok, I'll squash this patch this into the subsequent patch where I separate the ideas of a domain having IOMMU pagetables and requiring them to be kept in sync.

  Paul

> 
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -214,14 +214,14 @@ void iommu_teardown(struct domain *d)
> >  {
> >      const struct domain_iommu *hd = dom_iommu(d);
> >
> > -    d->need_iommu = 0;
> > +    d->need_iommu = false;
> >      hd->platform_ops->teardown(d);
> >      tasklet_schedule(&iommu_pt_cleanup_tasklet);
> >  }
> >
> >  int iommu_construct(struct domain *d)
> >  {
> > -    if ( need_iommu(d) > 0 )
> > +    if ( need_iommu(d) )
> >          return 0;
> >
> >      if ( !iommu_use_hap_pt(d) )
> > @@ -233,7 +233,7 @@ int iommu_construct(struct domain *d)
> >              return rc;
> >      }
> >
> > -    d->need_iommu = 1;
> > +    d->need_iommu = true;
> 
> So with the setting to -1 gone from the caller, need_iommu(d) will
> continue to return false until this completion point is reached. The
> fundamental idea of the tristate was that once we've started
> populating the IOMMU page tables (recall, the domain is not
> paused if this is a hotplug), all _other_ operations requiring IOMMU
> page table manipulation (grant table code, for example) will
> DTRT (tm) despite the code here perhaps never getting to notice
> the relevant page.
> 
> Trust me, it wasn't a lightweight decision to make this a tristate,
> I just couldn't see a better approach (short of using a second
> boolean, which I would have liked even less), and I still can't.
> 
> > @@ -493,7 +493,7 @@ static void iommu_dump_p2m_table(unsigned char
> key)
> >      ops = iommu_get_ops();
> >      for_each_domain(d)
> >      {
> > -        if ( is_hardware_domain(d) || need_iommu(d) <= 0 )
> > +        if ( is_hardware_domain(d) || !need_iommu(d) )
> >              continue;
> 
> I don't think the original semantics of the dumping to be skipped for
> domains with their IOMMU page tables under construction is being
> retained here.
> 
> Jan
> 


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