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

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 19/02/2021 08:49, Jan Beulich wrote:
> On 18.02.2021 14:25, Julien Grall wrote:
> > Hi,
> >
> > On 18/02/2021 13:05, Jan Beulich wrote:
> > > On 17.02.2021 17:07, Julien Grall wrote:
> > > > On 17/02/2021 15:01, Jan Beulich wrote:
> > > > > On 17.02.2021 15:24, 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.
> > > > > >
> > > > > > Currently page-table allocations can only happen from iommu_map(). As
> > > > > > the domain is dying, there is no good reason to continue to modify the
> > > > > > IOMMU page-tables.
> > > > > While I agree this to be the case right now, I'm not sure it is a
> > > > > good idea to build on it (in that you leave the unmap paths
> > > > > untouched).
> > > > I don't build on that assumption. See next patch.
> > > Yet as said there that patch makes unmapping perhaps more fragile,
> > > by introducing a latent error source into the path.
> > I still don't see what latent error my patch will introduce. Allocation
> > of page-tables are not guarantee to succeed.
> >
> > So are you implying that a code may rely on the page allocation to succeed?
> I'm implying that failure to split a superpage may have unknown
> consequences.
As failure to flush the TLBs (see below).

> Since we make no use of superpages when not
> sharing page tables, I call this a latent issue which may go
> unnoticed for quite some time once no longer latent.
And it would take a lot longer to unnotice an OOM as this should happen 
less often ;).
But, I think this is wrong to blame the lower layer for a (latent) bug 
in the upper layer...
Even with your solution to zap page-tables, there is still a risk of 
failure because the TLB flush may fail (the operation can return an error).
So regardless the solution, we still need to have callers that function 
correctly.
Anyway, what matters right now is fixing a host crash when running PV 
guest with passthrough.  Neither patch #3 nor the iommu_unmap() part is 
strictly necessary for 4.15 (as you say this is latent).
So I would suggest to defer this discussion post-4.15.

Cheers,

--
Julien Grall