Re: [PATCH 3/6] KVM: Documentation: Add the missing ptep in kvm_mmu_page

[Mingwei Zhang]

On Thu, Jun 22, 2023, Huang, Kai wrote:
> On Sun, 2023-06-18 at 00:08 +0000, Mingwei Zhang wrote:
> > Add the missing ptep in kvm_mmu_page description. ptep is used when TDP MMU
> > is enabled and it shares the storage with parent_ptes. Update the doc to
> > help readers to get up-to-date info.
> > 
> > Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
> > ---
> >  Documentation/virt/kvm/x86/mmu.rst | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/virt/kvm/x86/mmu.rst b/Documentation/virt/kvm/x86/mmu.rst
> > index 149dd3cba48f..36bfe0fe02bb 100644
> > --- a/Documentation/virt/kvm/x86/mmu.rst
> > +++ b/Documentation/virt/kvm/x86/mmu.rst
> > @@ -236,6 +236,10 @@ Shadow pages contain the following information:
> >      parent_ptes points at this single spte, otherwise, there exists multiple
> >      sptes pointing at this page and (parent_ptes & ~0x1) points at a data
> >      structure with a list of parent sptes.
> > +  ptep:
> > +    Pointer to the parent spte when TDP MMU is enabled. 
> > 
> 
> IMHO "parent spte" alone _may_ be confusing.  I think it's better to explicitly
> mention "pointing to this page" similar to the "parent_ptes" above.

Sure. I can change the style to be consistent with the descriptions of
'parent_ptes'.

> 
> Also, I think "when TDP MMU is enabled" isn't strictly true, depending on what
> does "when TDP MMU is enabled mean".  E.g., when tdp_mmu_enabled module
> parameter is true, we can still have a nested EPT shadow page from L2 which
> won't use this either IIUC.
> 
hmm, "when TDP MMU is enabled" should be "when used by TDP MMU". You are
right since when TDP MMU is used for L1, we may still have shadow MMUs
for L2s. I modify the description.

> > In TDP MMU, each
> > +    shadow page will have at most one parent. Note that this field is a
> > +    union with parent_ptes.
> 
> Also, perhaps "have at most one parent" can be more precise: only root page has
> no parent, while other non-root pages always have one parent SPTE pointing to
> each of them.

Will do in next version.