[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.