Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 27 Jun 2023 08:50:02 -0700
In-Reply-To: <20230626182016.4127366-5-mizhang@google.com>
Mime-Version: 1.0
References: <20230626182016.4127366-1-mizhang@google.com> <20230626182016.4127366-5-mizhang@google.com>
Message-ID: <ZJsFKjbaCKk+fFkv@google.com>
Subject: Re: [PATCH v2 4/6] KVM: Documentation: Add the missing description
 for tdp_mmu_root_count into kvm_mmu_page
From:   Sean Christopherson <seanjc@google.com>
To:     Mingwei Zhang <mizhang@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        Kai Huang <kai.huang@intel.com>,
        Jim Mattson <jmattson@google.com>,
        David Matlack <dmatlack@google.com>,
        Ben Gardon <bgardon@google.com>, Xu Yilun <yilun.xu@intel.com>,
        Zhi Wang <zhi.wang.linux@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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
>