Re: [Xen-devel] [BUG] After upgrade to Xen 4.12.0 iommu=no-igfx

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
[snip]
> > > diff --git a/xen/drivers/passthrough/iommu.c 
> > > b/xen/drivers/passthrough/iommu.c
> > > index 79ec6719f5..9d91f0d633 100644
> > > --- a/xen/drivers/passthrough/iommu.c
> > > +++ b/xen/drivers/passthrough/iommu.c
> > > @@ -185,7 +185,7 @@ 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);
> > > +    hd->need_sync = !iommu_use_hap_pt(d);
> >
> > But this is going to mean the if() below is true for non-strict dom0, which 
> > means it pointlessly
> maps the dom0 page list when hwdom_iommu_map() should have already mapped all 
> conventional RAM.
> 
> Right, this all seems quite broken. Non-translated guests (ie: PV)
> would always need iommu page-table sync, but set_identity_p2m_entry
> contains the following:
> 
> if ( !paging_mode_translate(p2m->domain) )
> {
>     if ( !need_iommu_pt_sync(d) )
>         return 0;
>     return iommu_legacy_map(d, _dfn(gfn_l), _mfn(gfn_l), PAGE_ORDER_4K,
>                             IOMMUF_readable | IOMMUF_writable);
> }
> 
> I wonder whether the usage of need_iommu_pt_sync is wrong there, and
> should instead be !has_iommu_pt(d), since non-translated domains would
> never share the iommu page-tables anyway.

You want syncing if the domain has IOMMU page tables and shared EPT is not in 
use, so this logic just seems wrong.

> 
> In any case, I think need_sync must be set when the iommu page tables
> are not shared, and gating it on iommu_hwdom_strict seems wrong to me,
> the strictness of the iommu doesn't affect whether a sync is need or
> not.

I think need_sync is gated on strict mode because, in 'relaxed' mode, the 
mappings that are set up when dom0 is started are supposed to be static (modulo 
hotplug RAM)... so modifications to the domain's page list are not supposed to 
have any effect, and so no synchronization need be done.

> 
> I've updated the patch to avoid the pointless mapping of dom0 page
> list, but I haven't included the change to set_identity_p2m_entry.
> 

So, I think the albeit odd looking logic in iommu_hwdom_init() was actually 
correct, but the code in set_identity_p2m_entry() is wrong.

  Paul

> Thanks, Roger.
> ---8<---
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 79ec6719f5..abf9e38607 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -185,8 +185,8 @@ 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) )
> +    hd->need_sync = !iommu_use_hap_pt(d);
> +    if ( iommu_hwdom_strict && need_iommu_pt_sync(d) )
>      {
>          struct page_info *page;
>          unsigned int i = 0, flush_flags = 0;


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