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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.08.18 at 19:22, <paul.durrant@xxxxxxxxxx> 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.

> --- 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel