Re: [PATCH v19 067/130] KVM: TDX: Add load_mmu_pgd method for TDX

[Binbin Wu]

On 4/6/2024 8:58 AM, Huang, Kai wrote:
> On Sat, 2024-04-06 at 00:09 +0000, Edgecombe, Rick P wrote:
> > On Wed, 2024-04-03 at 10:33 -0700, Isaku Yamahata wrote:
> > > On Mon, Apr 01, 2024 at 11:49:43PM +0800,
> > > Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx> wrote:
> > >
> > > > On 2/26/2024 4:26 PM, isaku.yamahata@xxxxxxxxx wrote:
> > > > > From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> > > > >
> > > > > For virtual IO, the guest TD shares guest pages with VMM without
> > > > > encryption.
> > > > Virtual IO is a use case of shared memory, it's better to use it
> > > > as a example instead of putting it at the beginning of the sentence.
> > > >
> > > >
> > > > >     Shared EPT is used to map guest pages in unprotected way.
> > > > >
> > > > > Add the VMCS field encoding for the shared EPTP, which will be used by
> > > > > TDX to have separate EPT walks for private GPAs (existing EPTP) versus
> > > > > shared GPAs (new shared EPTP).
> > > > >
> > > > > Set shared EPT pointer value for the TDX guest to initialize TDX MMU.
> > > > May have a mention that the EPTP for priavet GPAs is set by TDX module.
> > > Sure, let me update the commit message.
> > How about this?
> Looks good.  Some nits though:
>
> > KVM: TDX: Add load_mmu_pgd method for TDX
> >
> > TDX has uses two EPT pointers, one for the private half of the GPA
> "TDX uses"
>
> > space and one for the shared half. The private half used the normal
> "used" -> "uses"
>
> > EPT_POINTER vmcs field and is managed in a special way by the TDX module.
> Perhaps add:
>
> KVM is not allowed to operate on the EPT_POINTER directly.
>
> > The shared half uses a new SHARED_EPT_POINTER field and will be managed by
> > the conventional MMU management operations that operate directly on the
> > EPT tables.
> I would like to explicitly call out KVM can update SHARED_EPT_POINTER directly:
>
> The shared half uses a new SHARED_EPT_POINTER field.  KVM is allowed to set it
> directly by the interface provided by the TDX module, and KVM is expected to
> manage the shared half just like it manages the existing EPT page table today.
>
>
> > This means for TDX the .load_mmu_pgd() operation will need to
> > know to use the SHARED_EPT_POINTER field instead of the normal one. Add a
> > new wrapper in x86 ops for load_mmu_pgd() that either directs the write to
> > the existing vmx implementation or a TDX one.
> >
> > For the TDX operation, EPT will always be used, so it can simpy write to


Maybe remove "so"?  IMO, there is no causal relationship between the 
first and second half of the sentence.

Typo, "simpy" -> "simply"

> > the SHARED_EPT_POINTER field.