Received-SPF: pass (google.com: domain of linux-kernel+bounces-204634-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240530210714.364118-1-rick.p.edgecombe@intel.com> <20240530210714.364118-4-rick.p.edgecombe@intel.com>
In-Reply-To: <20240530210714.364118-4-rick.p.edgecombe@intel.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Thu, 6 Jun 2024 18:04:17 +0200
Message-ID: <CABgObfYO2FgAOpvp-9jexp5fMh2xoYyE1CNs526z9S7i2Gao_g@mail.gmail.com>
Subject: Re: [PATCH v2 03/15] KVM: x86/mmu: Add a mirrored pointer to struct kvm_mmu_page
To: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: seanjc@google.com, kvm@vger.kernel.org, kai.huang@intel.com, 
	dmatlack@google.com, erdemaktas@google.com, isaku.yamahata@gmail.com, 
	linux-kernel@vger.kernel.org, sagis@google.com, yan.y.zhao@intel.com, 
	Isaku Yamahata <isaku.yamahata@intel.com>, Binbin Wu <binbin.wu@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, May 30, 2024 at 11:07=E2=80=AFPM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
>
> From: Isaku Yamahata <isaku.yamahata@intel.com>
>
> Add a mirrored pointer to struct kvm_mmu_page for the private page table =
and
> add helper functions to allocate/initialize/free a private page table pag=
e.
> Because KVM TDP MMU doesn't use unsync_children and write_flooding_count,
> pack them to have room for a pointer and use a union to avoid memory
> overhead.
>
> For private GPA, CPU refers to a private page table whose contents are
> encrypted. The dedicated APIs to operate on it (e.g. updating/reading its
> PTE entry) are used, and their cost is expensive.
>
> When KVM resolves the KVM page fault, it walks the page tables. To reuse
> the existing KVM MMU code and mitigate the heavy cost of directly walking
> the private page table, allocate one more page for the mirrored page tabl=
e
> for the KVM MMU code to directly walk. Resolve the KVM page fault with
> the existing code, and do additional operations necessary for the private
> page table. To distinguish such cases, the existing KVM page table is
> called a shared page table (i.e., not associated with a private page
> table), and the page table with a private page table is called a mirrored
> page table. The relationship is depicted below.
>
>               KVM page fault                     |
>                      |                           |
>                      V                           |
>         -------------+----------                 |
>         |                      |                 |
>         V                      V                 |
>      shared GPA           private GPA            |
>         |                      |                 |
>         V                      V                 |
>     shared PT root      mirror PT root           |    private PT root
>         |                      |                 |           |
>         V                      V                 |           V
>      shared PT           mirror PT     --propagate-->  private/mirrored P=
T
>         |                      |                 |           |
>         |                      \-----------------+------\    |
>         |                                        |      |    |
>         V                                        |      V    V
>   shared guest page                              |    private guest page
>                                                  |
>                            non-encrypted memory  |    encrypted memory
>                                                  |
> PT: Page table
> Shared PT: visible to KVM, and the CPU uses it for shared mappings.
> Private/mirrored PT: the CPU uses it, but it is invisible to KVM.  TDX
>                      module updates this table to map private guest pages=
.
> Mirror PT: It is visible to KVM, but the CPU doesn't use it.  KVM uses it
>              to propagate PT change to the actual private PT.

Which one is the "Mirror" and which one is the "Mirrored" PT is
uncomfortably confusing.

I hate to bikeshed even more, but while I like "Mirror PT" (a lot), I
would stick with "Private" or perhaps "External" for the pages owned
by the TDX module.

> +       /*
> +        * This cache is to allocate private page table. E.g. private EPT=
 used
> +        * by the TDX module.
> +        */
> +       struct kvm_mmu_memory_cache mmu_mirrored_spt_cache;

So this would be "mmu_external_spt_cache".

> -       unsigned int unsync_children;
> +       union {
> +               /* Those two members aren't used for TDP MMU */

s/Those/These/

> +               struct {
> +                       unsigned int unsync_children;
> +                       /*
> +                        * Number of writes since the last time traversal
> +                        * visited this page.
> +                        */
> +                       atomic_t write_flooding_count;
> +               };
> +               /*
> +                * Page table page of private PT.
> +                * Passed to TDX module, not accessed by KVM.
> +                */
> +               void *mirrored_spt;

external_spt

> +static inline void kvm_mmu_alloc_mirrored_spt(struct kvm_vcpu *vcpu, str=
uct kvm_mmu_page *sp)
> +{
> +       /*
> +        * mirrored_spt is allocated for TDX module to hold private EPT m=
appings,
> +        * TDX module will initialize the page by itself.
> +        * Therefore, KVM does not need to initialize or access mirrored_=
spt.
> +        * KVM only interacts with sp->spt for mirrored EPT operations.
> +        */
> +       sp->mirrored_spt =3D kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_m=
irrored_spt_cache);
> +}
> +
> +static inline void kvm_mmu_alloc_private_spt(struct kvm_vcpu *vcpu, stru=
ct kvm_mmu_page *sp)
> +{
> +       /*
> +        * private_spt is allocated for TDX module to hold private EPT ma=
ppings,
> +        * TDX module will initialize the page by itself.
> +        * Therefore, KVM does not need to initialize or access private_s=
pt.
> +        * KVM only interacts with sp->spt for mirrored EPT operations.
> +        */
> +       sp->mirrored_spt =3D kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_m=
irrored_spt_cache);
> +}

Duplicate function.

Naming aside, looks good.

Paolo

On Thu, May 30, 2024 at 11:07=E2=80=AFPM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
>
> From: Isaku Yamahata <isaku.yamahata@intel.com>
>
> Add a mirrored pointer to struct kvm_mmu_page for the private page table =
and
> add helper functions to allocate/initialize/free a private page table pag=
e.
> Because KVM TDP MMU doesn't use unsync_children and write_flooding_count,
> pack them to have room for a pointer and use a union to avoid memory
> overhead.
>
> For private GPA, CPU refers to a private page table whose contents are
> encrypted. The dedicated APIs to operate on it (e.g. updating/reading its
> PTE entry) are used, and their cost is expensive.
>
> When KVM resolves the KVM page fault, it walks the page tables. To reuse
> the existing KVM MMU code and mitigate the heavy cost of directly walking
> the private page table, allocate one more page for the mirrored page tabl=
e
> for the KVM MMU code to directly walk. Resolve the KVM page fault with
> the existing code, and do additional operations necessary for the private
> page table. To distinguish such cases, the existing KVM page table is
> called a shared page table (i.e., not associated with a private page
> table), and the page table with a private page table is called a mirrored
> page table. The relationship is depicted below.
>
>               KVM page fault                     |
>                      |                           |
>                      V                           |
>         -------------+----------                 |
>         |                      |                 |
>         V                      V                 |
>      shared GPA           private GPA            |
>         |                      |                 |
>         V                      V                 |
>     shared PT root      mirror PT root           |    private PT root
>         |                      |                 |           |
>         V                      V                 |           V
>      shared PT           mirror PT     --propagate-->  private/mirrored P=
T
>         |                      |                 |           |
>         |                      \-----------------+------\    |
>         |                                        |      |    |
>         V                                        |      V    V
>   shared guest page                              |    private guest page
>                                                  |
>                            non-encrypted memory  |    encrypted memory
>                                                  |
> PT: Page table
> Shared PT: visible to KVM, and the CPU uses it for shared mappings.
> Private/mirrored PT: the CPU uses it, but it is invisible to KVM.  TDX
>                      module updates this table to map private guest pages=
.
> Mirror PT: It is visible to KVM, but the CPU doesn't use it.  KVM uses it
>              to propagate PT change to the actual private PT.
>
> Add a helper kvm_has_mirrored_tdp() to trigger this behavior and wire it
> to the TDX vm type.
>
> Co-developed-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
> TDX MMU Prep v2:
>  - Rename private->mirror
>  - Don't trigger off of shared mask
>
> TDX MMU Prep:
> - Rename terminology, dummy PT =3D> mirror PT. and updated the commit mes=
sage
>   By Rick and Kai.
> - Added a comment on union of private_spt by Rick.
> - Don't handle the root case in kvm_mmu_alloc_private_spt(), it will not
>   be needed in future patches. (Rick)
> - Update comments (Yan)
> - Remove kvm_mmu_init_private_spt(), open code it in later patches (Yan)
>
> v19:
> - typo in the comment in kvm_mmu_alloc_private_spt()
> - drop CONFIG_KVM_MMU_PRIVATE
> ---
>  arch/x86/include/asm/kvm_host.h |  5 ++++
>  arch/x86/kvm/mmu.h              |  5 ++++
>  arch/x86/kvm/mmu/mmu.c          |  7 +++++
>  arch/x86/kvm/mmu/mmu_internal.h | 47 ++++++++++++++++++++++++++++++---
>  arch/x86/kvm/mmu/tdp_mmu.c      |  1 +
>  5 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_h=
ost.h
> index aabf1648a56a..250899a0239b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -817,6 +817,11 @@ struct kvm_vcpu_arch {
>         struct kvm_mmu_memory_cache mmu_shadow_page_cache;
>         struct kvm_mmu_memory_cache mmu_shadowed_info_cache;
>         struct kvm_mmu_memory_cache mmu_page_header_cache;
> +       /*
> +        * This cache is to allocate private page table. E.g. private EPT=
 used
> +        * by the TDX module.
> +        */
> +       struct kvm_mmu_memory_cache mmu_mirrored_spt_cache;
>
>         /*
>          * QEMU userspace and the guest each have their own FPU state.
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index dc80e72e4848..0c3bf89cf7db 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -318,4 +318,9 @@ static inline gpa_t kvm_translate_gpa(struct kvm_vcpu=
 *vcpu,
>                 return gpa;
>         return translate_nested_gpa(vcpu, gpa, access, exception);
>  }
> +
> +static inline bool kvm_has_mirrored_tdp(const struct kvm *kvm)
> +{
> +       return kvm->arch.vm_type =3D=3D KVM_X86_TDX_VM;
> +}
>  #endif
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index b97241945596..5070ba7c6e89 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -685,6 +685,12 @@ static int mmu_topup_memory_caches(struct kvm_vcpu *=
vcpu, bool maybe_indirect)
>                                        1 + PT64_ROOT_MAX_LEVEL + PTE_PREF=
ETCH_NUM);
>         if (r)
>                 return r;
> +       if (kvm_has_mirrored_tdp(vcpu->kvm)) {
> +               r =3D kvm_mmu_topup_memory_cache(&vcpu->arch.mmu_mirrored=
_spt_cache,
> +                                              PT64_ROOT_MAX_LEVEL);
> +               if (r)
> +                       return r;
> +       }
>         r =3D kvm_mmu_topup_memory_cache(&vcpu->arch.mmu_shadow_page_cach=
e,
>                                        PT64_ROOT_MAX_LEVEL);
>         if (r)
> @@ -704,6 +710,7 @@ static void mmu_free_memory_caches(struct kvm_vcpu *v=
cpu)
>         kvm_mmu_free_memory_cache(&vcpu->arch.mmu_pte_list_desc_cache);
>         kvm_mmu_free_memory_cache(&vcpu->arch.mmu_shadow_page_cache);
>         kvm_mmu_free_memory_cache(&vcpu->arch.mmu_shadowed_info_cache);
> +       kvm_mmu_free_memory_cache(&vcpu->arch.mmu_mirrored_spt_cache);
>         kvm_mmu_free_memory_cache(&vcpu->arch.mmu_page_header_cache);
>  }
>
> diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_inter=
nal.h
> index 706f0ce8784c..faef40a561f9 100644
> --- a/arch/x86/kvm/mmu/mmu_internal.h
> +++ b/arch/x86/kvm/mmu/mmu_internal.h
> @@ -101,7 +101,22 @@ struct kvm_mmu_page {
>                 int root_count;
>                 refcount_t tdp_mmu_root_count;
>         };
> -       unsigned int unsync_children;
> +       union {
> +               /* Those two members aren't used for TDP MMU */
> +               struct {
> +                       unsigned int unsync_children;
> +                       /*
> +                        * Number of writes since the last time traversal
> +                        * visited this page.
> +                        */
> +                       atomic_t write_flooding_count;
> +               };
> +               /*
> +                * Page table page of private PT.
> +                * Passed to TDX module, not accessed by KVM.
> +                */
> +               void *mirrored_spt;
> +       };
>         union {
>                 struct kvm_rmap_head parent_ptes; /* rmap pointers to par=
ent sptes */
>                 tdp_ptep_t ptep;
> @@ -124,9 +139,6 @@ struct kvm_mmu_page {
>         int clear_spte_count;
>  #endif
>
> -       /* Number of writes since the last time traversal visited this pa=
ge.  */
> -       atomic_t write_flooding_count;
> -
>  #ifdef CONFIG_X86_64
>         /* Used for freeing the page asynchronously if it is a TDP MMU pa=
ge. */
>         struct rcu_head rcu_head;
> @@ -145,6 +157,33 @@ static inline int kvm_mmu_page_as_id(struct kvm_mmu_=
page *sp)
>         return kvm_mmu_role_as_id(sp->role);
>  }
>
> +static inline void *kvm_mmu_mirrored_spt(struct kvm_mmu_page *sp)
> +{
> +       return sp->mirrored_spt;
> +}
> +
> +static inline void kvm_mmu_alloc_mirrored_spt(struct kvm_vcpu *vcpu, str=
uct kvm_mmu_page *sp)
> +{
> +       /*
> +        * mirrored_spt is allocated for TDX module to hold private EPT m=
appings,
> +        * TDX module will initialize the page by itself.
> +        * Therefore, KVM does not need to initialize or access mirrored_=
spt.
> +        * KVM only interacts with sp->spt for mirrored EPT operations.
> +        */
> +       sp->mirrored_spt =3D kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_m=
irrored_spt_cache);
> +}
> +
> +static inline void kvm_mmu_alloc_private_spt(struct kvm_vcpu *vcpu, stru=
ct kvm_mmu_page *sp)
> +{
> +       /*
> +        * private_spt is allocated for TDX module to hold private EPT ma=
ppings,
> +        * TDX module will initialize the page by itself.
> +        * Therefore, KVM does not need to initialize or access private_s=
pt.
> +        * KVM only interacts with sp->spt for mirrored EPT operations.
> +        */
> +       sp->mirrored_spt =3D kvm_mmu_memory_cache_alloc(&vcpu->arch.mmu_m=
irrored_spt_cache);
> +}
> +
>  static inline bool kvm_mmu_page_ad_need_write_protect(struct kvm_mmu_pag=
e *sp)
>  {
>         /*
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 1259dd63defc..e7cd4921afe7 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -53,6 +53,7 @@ void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm)
>
>  static void tdp_mmu_free_sp(struct kvm_mmu_page *sp)
>  {
> +       free_page((unsigned long)sp->mirrored_spt);
>         free_page((unsigned long)sp->spt);
>         kmem_cache_free(mmu_page_header_cache, sp);
>  }
> --
> 2.34.1
>