Re: [PATCH v2 03/15] KVM: x86/mmu: Add a mirrored pointer to struct kvm_mmu_page

[Edgecombe, Rick P]

On Thu, 2024-06-06 at 18:04 +0200, Paolo Bonzini wrote:
> > PT: Page table
> > Shared PT: visible to KVM, and the CPU uses it for shared mappings.
> > Private/mirrored PT: the CPU uses it, but it is invisible to KVM.  TDX
> >                       module updates this table to map private guest pages.
> > Mirror PT: It is visible to KVM, but the CPU doesn't use it.  KVM uses it
> >               to propagate PT change to the actual private PT.
> 
> Which one is the "Mirror" and which one is the "Mirrored" PT is
> uncomfortably confusing.
> 
> I hate to bikeshed even more, but while I like "Mirror PT" (a lot), I
> would stick with "Private" or perhaps "External" for the pages owned
> by the TDX module.

Thanks. Yes, mirror and mirrored is too close.

Kai raised the point that TDX's special need for separate, dedicated "private"
PTEs is confusing from the SEV and SW protected VM perspective, so we've tried
to remove private in that context. "External" seems like a good name.

[snip]
> 
> > +static inline void kvm_mmu_alloc_mirrored_spt(struct kvm_vcpu *vcpu, struct
> > kvm_mmu_page *sp)
> > +{
> > +       /*
> > +        * mirrored_spt is allocated for TDX module to hold private EPT
> > mappings,
> > +        * TDX module will initialize the page by itself.
> > +        * Therefore, KVM does not need to initialize or access
> > mirrored_spt.
> > +        * KVM only interacts with sp->spt for mirrored EPT operations.
> > +        */
> > +       sp->mirrored_spt = kvm_mmu_memory_cache_alloc(&vcpu-
> > >arch.mmu_mirrored_spt_cache);
> > +}
> > +
> > +static inline void kvm_mmu_alloc_private_spt(struct kvm_vcpu *vcpu, struct
> > kvm_mmu_page *sp)
> > +{
> > +       /*
> > +        * private_spt is allocated for TDX module to hold private EPT
> > mappings,
> > +        * TDX module will initialize the page by itself.
> > +        * Therefore, KVM does not need to initialize or access private_spt.
> > +        * KVM only interacts with sp->spt for mirrored EPT operations.
> > +        */
> > +       sp->mirrored_spt = kvm_mmu_memory_cache_alloc(&vcpu-
> > >arch.mmu_mirrored_spt_cache);
> > +}
> 
> Duplicate function.

Argh, I thought I fixed this before sending it out.

> 
> Naming aside, looks good.

Great, thanks for the review! Snipped comments all sound good.