Re: [for-4.15][RESEND PATCH v4 1/2] xen/x86: iommu: Ignore IOMMU mapping requests when a domain is dying

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 24/02/2021 14:07, Jan Beulich wrote:
> On 24.02.2021 10:43, Julien Grall wrote:
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > The new x86 IOMMU page-tables allocator will release the pages when
> > relinquishing the domain resources. However, this is not sufficient
> > when the domain is dying because nothing prevents page-table to be
> > allocated.
> >
> > As the domain is dying, it is not necessary to continue to modify the
> > IOMMU page-tables as they are going to be destroyed soon.
> >
> > At the moment, page-table allocates will only happen when iommu_map().
> > So after this change there will be no more page-table allocation
> > happening.
> While I'm still not happy about this asymmetry, I'm willing to accept
> it in the interest of getting the underlying issue addressed. May I
> ask though that you add something like "... because we don't use
> superpage mappings yet when not sharing page tables"?
Sure.

> But there are two more minor things:
>
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -267,6 +267,12 @@ int iommu_free_pgtables(struct domain *d)
> >       struct page_info *pg;
> >       unsigned int done = 0;
> >   
> > +    if ( !is_iommu_enabled(d) )
> > +        return 0;
> Why is this addition needed? Hitting a not yet initialize spin lock
> is - afaict - no worse than a not yet initialized list, so it would
> seem to me that this can't be the reason. No other reason looks to
> be called out by the description.
struct domain_iommu will be initially zeroed as it is part of struct domain.

For the list, we are so far fine because page_list_remove_head() 
tolerates NULL. If we were using the normal list operations (e.g. 
list_del), then this code would have segfaulted.
Now about the spinlock, at least lock debugging expects a non-zero 
initial value. We are lucky here because this path is not called with 
IRQ disabled. If it were, Xen would crash as it would consider the lock 
was used in a non-IRQ safe environment.
So in the spinlock case, we are really playing with the fire. Hence, the 
check here.
> > +    /* After this barrier, no more IOMMU mapping can happen */
> > +    spin_barrier(&hd->arch.mapping_lock);
> On the v3 discussion I thought you did agree to change the wording
> of the comment to something like "no new IOMMU mappings can be
> inserted"?
Sorry I missed this comment. I will update it in the next version.

Cheers,

--
Julien Grall