[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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |