[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH for-4.15 3/4] [RFC] xen/iommu: x86: Clear the root page-table before freeing the page-tables
> -----Original Message----- > From: Jan Beulich <jbeulich@xxxxxxxx> > Sent: 23 December 2020 16:46 > To: Julien Grall <julien@xxxxxxx>; Paul Durrant <paul@xxxxxxx> > Cc: hongyxia@xxxxxxxxxxxx; Julien Grall <jgrall@xxxxxxxxxx>; > xen-devel@xxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH for-4.15 3/4] [RFC] xen/iommu: x86: Clear the root > page-table before freeing the > page-tables > > On 23.12.2020 17:29, Julien Grall wrote: > > On 23/12/2020 16:24, Jan Beulich wrote: > >> On 23.12.2020 17:16, Julien Grall wrote: > >>> On 23/12/2020 16:11, Jan Beulich wrote: > >>>> On 23.12.2020 16:16, Julien Grall wrote: > >>>>> On 23/12/2020 15:00, Jan Beulich wrote: > >>>>>> On 23.12.2020 15:56, Julien Grall wrote: > >>>>>>> On 23/12/2020 14:12, Jan Beulich wrote: > >>>>>>>> On 22.12.2020 16:43, Julien Grall wrote: > >>>>>>>>> This is an RFC because it would break AMD IOMMU driver. One option > >>>>>>>>> would > >>>>>>>>> be to move the call to the teardown callback earlier on. Any > >>>>>>>>> opinions? > >> > >> Please note this (in your original submission). I simply ... > >> > >>>>>>>> We already have > >>>>>>>> > >>>>>>>> static void amd_iommu_domain_destroy(struct domain *d) > >>>>>>>> { > >>>>>>>> dom_iommu(d)->arch.amd.root_table = NULL; > >>>>>>>> } > >>>>>>>> > >>>>>>>> and this function is AMD's teardown handler. Hence I suppose > >>>>>>>> doing the same for VT-d would be quite reasonable. And really > >>>>>>>> VT-d's iommu_domain_teardown() also already has > >>>>>>>> > >>>>>>>> hd->arch.vtd.pgd_maddr = 0; > >>>>>>> > >>>>>>> Let me have a look if that works. > >>>>>>> > >>>>>>>> > >>>>>>>> I guess what's missing is prevention of the root table > >>>>>>>> getting re-setup. > >>>>>>> > >>>>>>> This is taken care in the follow-up patch by forbidding page-table > >>>>>>> allocation. I can mention it in the commit message. > >>>>>> > >>>>>> My expectation is that with that subsequent change the change here > >>>>>> (or any variant of it) would become unnecessary. > >>>>> > >>>>> I am not be sure. iommu_unmap() would still get called from put_page(). > >>>>> Are you suggesting to gate the code if d->is_dying as well? > >>>> > >>>> Unmap shouldn't be allocating any memory right now, as in > >>>> non-shared-page-table mode we don't install any superpages > >>>> (unless I misremember). > >>> > >>> It doesn't allocate memory, but it will try to access the IOMMU > >>> page-tables (see more below). > >>> > >>>> > >>>>> Even if this patch is deemed to be unecessary to fix the issue. > >>>>> This issue was quite hard to chase/reproduce. > >>>>> > >>>>> I think it would still be good to harden the code by zeroing > >>>>> hd->arch.vtd.pgd_maddr to avoid anyone else wasting 2 days because the > >>>>> pointer was still "valid". > >>>> > >>>> But my point was that this zeroing already happens. > >>>> What I > >>>> suspect is that it gets re-populated after it was zeroed, > >>>> because of page table manipulation that shouldn't be > >>>> occurring anymore for a dying domain. > >>> > >>> AFAICT, the zeroing is happening in ->teardown() helper. > >>> > >>> It is only called when the domain is fully destroyed (see call in > >>> arch_domain_destroy()). This will happen much after relinquishing the > >>> code. > >>> > >>> Could you clarify why you think it is already zeroed and by who? > >> > >> ... trusted you on what you stated there. But perhaps I somehow > >> misunderstood that sentence to mean you want to put your addition > >> into the teardown functions, when apparently you meant to invoke > >> them earlier in the process. Without clearly identifying why this > >> would be a safe thing to do, I couldn't imagine that's what you > >> suggest as alternative. > > > > This was a wording issue. I meant moving ->teardown() before (or calling > > from) iommu_free_pgtables(). > > > > Shall I introduce a new callback then? > > Earlier zeroing won't help unless you prevent re-population, or > unless you make the code capable of telling "still zero" from > "already zero". But I have to admit I'd like to also have Paul's > opinion on the matter. > I have a pending series to dynamically unshare IOMMU mappings (to allow logdirty to be enabled on domains with currently-shared EPT) which gets rid of the lazy allocation of the root table and uses pgd_maddr == NULL as a test of whether EPT is shared or not. Therefore it would seem best, to fix this immediate problem, to also stop lazy allocation of pgd_maddr, and re-introduce a per-implementation free_pgtables() callback which zeroes pgd_maddr and then calls the common function. Paul
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |