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

Message ID 20230626182016.4127366-5-mizhang@google.com
State New
Headers
Series Update document description for kvm_mmu_page and kvm_mmu_page_role |

Commit Message

Mingwei Zhang June 26, 2023, 6:20 p.m. UTC
  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@google.com>
---
 Documentation/virt/kvm/x86/mmu.rst | 5 +++++
 1 file changed, 5 insertions(+)
  

Comments

Kai Huang June 26, 2023, 10:21 p.m. UTC | #1
On Mon, 2023-06-26 at 18:20 +0000, 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@google.com>
> ---
>  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.  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
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>
  
Sean Christopherson June 27, 2023, 3:50 p.m. UTC | #2
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@google.com>
> ---
>  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.

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


>   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
>
  
Mingwei Zhang July 31, 2023, 6:41 p.m. UTC | #3
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@google.com>
> > ---
> >  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
> >
  

Patch

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.  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