Re: [PATCH for-4.15 3/4] [RFC] xen/iommu: x86: Clear the root page-table before freeing the page-tables

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

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?
> > > 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?
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".
Cheers,

--
Julien Grall