Re: [PATCH v2 4/6] KVM: Documentation: Add the missing description for tdp_mmu_root_count into kvm_mmu_page

[Mingwei Zhang]

On Tue, Jun 27, 2023, Sean Christopherson wrote:
> On Mon, Jun 26, 2023, Mingwei Zhang wrote:
> > Add the description of tdp_mmu_root_count into kvm_mmu_page description.
> > tdp_mmu_root_count is an atomic counter used only in TDP MMU. Its usage and
> > meaning is slightly different with root_counter in shadow MMU. Update the
> > doc.
> > 
> > Signed-off-by: Mingwei Zhang <mizhang@xxxxxxxxxx>
> > ---
> >  Documentation/virt/kvm/x86/mmu.rst | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/virt/kvm/x86/mmu.rst b/Documentation/virt/kvm/x86/mmu.rst
> > index 5cd6cd5e8926..97d695207e11 100644
> > --- a/Documentation/virt/kvm/x86/mmu.rst
> > +++ b/Documentation/virt/kvm/x86/mmu.rst
> > @@ -231,6 +231,11 @@ Shadow pages contain the following information:
> >      A counter keeping track of how many hardware registers (guest cr3 or
> >      pdptrs) are now pointing at the page.  While this counter is nonzero, the
> >      page cannot be destroyed.  See role.invalid.
> > +  tdp_mmu_root_count:
> > +    An atomic reference counter in TDP MMU root page that allows for parallel
> > +    accesses.
> 
> I find the "parallel accesses" simultaneously redundant and confusing.  The fact
> that's it's an atomic implies that there are concurrent accesses.  And need for
> an atomic is really just a minor note, i.e. shouldn't be the focus of the
> documentation.
> 
> On a related topic, the description for "root_count" is stale now that KVM keeps
> references to roots.
> 
> What if we take this opportunity to unify the documentation?
> 
>   root_count / tdp_mmu_rount_count:
> 
>      A reference counter for root shadow pages.  vCPUs elevate the refcount when
>      getting a shadow page that will be used as a root, i.e. will be loaded into
>      hardware directly (CR3, PDPTRs, nCR3 EPTP).  Root pages cannnot be freed
>      while their refcount is non-zero.  The TDP MMU uses an atomic refcount as
>      vCPUs can acquire references while holding mmu_lock for read.  See
>      role.invalid and Root Pages.

Ok, I think this one is reasonable and this clarifies that this field
only works for root pages.
> 
> And then add a section specifically for root pages?  I think trying to cram
> everything important about root pages into the description for their refcount
> will be difficult and kludgy.  E.g. this doc should also provide an explanation of
> previous roots.
> 
> Root Pages
> ==========
> 
> Key talking points:
> 
>   - Definition of a root page
>   - Lifecycle of roots for both the shadow MMU and TDP MMU
>   - Previous root tracking, and why only KVM doesn'y track previous roots when
>     using PAE paging
>   - The importance of preserving roots that are currently not referenced by any
>     vCPU, i.e. why TDP MMU roots are initialized with a refcount of '2'
>   - Why shadow MMU roots don't gift a reference to the MMU itself, i.e. why they
>     naturally survive their refcount going to zero
> 

I am not sure if I can add the whole section in this commit. Maybe
I can push it back separately into a different series. For root_count, a
brief introduction of root pages should be good enough, which is explain
in your suggestion: page that "will be loaded into hardware directly
(CR3, PDPTRs, nCR3 EPTP)".
> 
> >   Accessing the page requires lifting the counter value. The
> > +    initial value is set to 2 indicating one reference from vCPU and one
> > +    from TDP MMU itself. Note this field is a union with root_count.
> >    parent_ptes:
> >      The reverse mapping for the pte/ptes pointing at this page's spt. If
> >      parent_ptes bit 0 is zero, only one spte points at this page and
> > -- 
> > 2.41.0.162.gfafddb0af9-goog
> >