Re: [PATCH 10/16] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Thu, 2024-05-16 at 14:07 +1200, Huang, Kai wrote:
> 
> I meant it seems we should just strip shared bit away from the GPA in 
> handle_ept_violation() and pass it as 'cr2_or_gpa' here, so fault->addr 
> won't have the shared bit.
> 
> Do you see any problem of doing so?

We would need to add it back in "raw_gfn" in kvm_tdp_mmu_map().

In the past I did something like the private/shared split, but for execute-only
aliases and a few other wacky things.

It also had a synthetic error code. For awhile I had it so GPA had alias bits
(i.e. shared bit) not stripped, like TDX has today, but there was always some
code that got surprised by the extra bits in the GPA. I want to say it was the
emulation of PAE or something like that (execute-only had to support all the
normal VM stuff).

So in the later revisions I actually had a helper to take a GFN and PF error
code and put the alias bits back in. Then alias bits got stripped immediately
and at the same time the synthetic error code was set. Something similar could
probably work to recreate "raw_gfn" from a fault.

IIRC (and I could easily be wrong), when I discussed this with Sean he said TDX
didn't need to support whatever issue I was working around, and the original
solution was slightly better for TDX.

In any case, I doubt Sean is wedded to a remark he may or may not have made long
ago. But looking at the TDX code today, it doesn't feel that confusing to me.

So I'm not against adding the shared bits back in later, but it doesn't seem
that big of a gain to me. It also has kind of been tried before a long time ago.

> 
> > 
> > > 
> > > >           }
> > > >     
> > > > diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h
> > > > index fae559559a80..8a64bcef9deb 100644
> > > > --- a/arch/x86/kvm/mmu/tdp_iter.h
> > > > +++ b/arch/x86/kvm/mmu/tdp_iter.h
> > > > @@ -91,7 +91,7 @@ struct tdp_iter {
> > > >           tdp_ptep_t pt_path[PT64_ROOT_MAX_LEVEL];
> > > >           /* A pointer to the current SPTE */
> > > >           tdp_ptep_t sptep;
> > > > -       /* The lowest GFN mapped by the current SPTE */
> > > > +       /* The lowest GFN (shared bits included) mapped by the current
> > > > SPTE
> > > > */
> > > >           gfn_t gfn;
> > > 
> > > IMHO we need more clarification of this design.
> > 
> > Have you seen the documentation patch? Where do you think it should be? You
> > mean
> > in the tdp_iter struct?
> 
> My thinking:
> 
> Changelog should clarify why include shared bit to 'gfn' in tdp_iter.
> 
> And here around the 'gfn' we can have some simple sentence to explain 
> why to include the shared bit.

Doesn't seem unreasonable.