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;
Message-ID: <8227079db11c0473f1c368b305e40a94a73fc109.camel@intel.com>
Subject: Re: [PATCH v7 039/102] KVM: x86/mmu: Allow per-VM override of the
 TDP max page level
From:   Kai Huang <kai.huang@intel.com>
To:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>
Date:   Fri, 01 Jul 2022 00:27:24 +1200
In-Reply-To: <e686602e7b57ed0c3600c663d03a9bf76190db0c.1656366338.git.isaku.yamahata@intel.com>
References: <cover.1656366337.git.isaku.yamahata@intel.com>
         <e686602e7b57ed0c3600c663d03a9bf76190db0c.1656366338.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.2 (3.44.2-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-06-27 at 14:53 -0700, isaku.yamahata@intel.com wrote:
> From: Sean Christopherson <sean.j.christopherson@intel.com>
>=20
> TODO: This is a transient workaround patch until the large page support f=
or
> TDX is implemented.  Support large page for TDX and remove this patch.

I don't understand.  How does this patch have anything to do with what you =
are
talking about here?

If you want to remove this patch later, then why not just explain the reaso=
n to
remove when you actually have that patch?

>=20
> At this point, large page for TDX isn't supported, and need to allow gues=
t
> TD to work only with 4K pages.  On the other hand, conventional VMX VMs
> should continue to work with large page.  Allow per-VM override of the TD=
P
> max page level.

At which point/previous patch have you made/declared "large page for TDX is=
n't
supported"?

If you want to declare you don't want to support large page for TDX, IMHO j=
ust
declare it here, for instance:

"For simplicity, only support 4K page for TD guest."
 =20
>=20
> In the existing x86 KVM MMU code, there is already max_level member in
> struct kvm_page_fault with KVM_MAX_HUGEPAGE_LEVEL initial value.  The KVM
> page fault handler denies page size larger than max_level.
>=20
> Add per-VM member to indicate the allowed maximum page size with
> KVM_MAX_HUGEPAGE_LEVEL as default value and initialize max_level in struc=
t
> kvm_page_fault with it.  For the guest TD, the set per-VM value for allow=
s
> maximum page size to 4K page size.  Then only allowed page size is 4K.  I=
t
> means large page is disabled.

To me it's overcomplicated.  You just need simple sentences for such simple
infrastructural patch.  For instance:

"TDX requires special handling to support large private page.  For simplici=
ty,
only support 4K page for TD guest for now.  Add per-VM maximum page level
support to support different maximum page sizes for TD guest and convention=
al
VMX guest."

Just for your reference.

>=20
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 1 +
>  arch/x86/kvm/mmu/mmu.c          | 1 +
>  arch/x86/kvm/mmu/mmu_internal.h | 2 +-
>  3 files changed, 3 insertions(+), 1 deletion(-)
>=20
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_h=
ost.h
> index 39215daa8576..f4d4ed41641b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1146,6 +1146,7 @@ struct kvm_arch {
>  	unsigned long n_requested_mmu_pages;
>  	unsigned long n_max_mmu_pages;
>  	unsigned int indirect_shadow_pages;
> +	int tdp_max_page_level;
>  	u8 mmu_valid_gen;
>  	struct hlist_head mmu_page_hash[KVM_NUM_MMU_PAGES];
>  	struct list_head active_mmu_pages;
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index e0aa5ad3931d..80d7c7709af3 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5878,6 +5878,7 @@ int kvm_mmu_init_vm(struct kvm *kvm)
>  	node->track_write =3D kvm_mmu_pte_write;
>  	node->track_flush_slot =3D kvm_mmu_invalidate_zap_pages_in_memslot;
>  	kvm_page_track_register_notifier(kvm, node);
> +	kvm->arch.tdp_max_page_level =3D KVM_MAX_HUGEPAGE_LEVEL;
>  	kvm_mmu_set_mmio_spte_mask(kvm, shadow_default_mmio_mask,
>  				   shadow_default_mmio_mask,
>  				   ACC_WRITE_MASK | ACC_USER_MASK);
> diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_inter=
nal.h
> index bd2a26897b97..44a04fad4bed 100644
> --- a/arch/x86/kvm/mmu/mmu_internal.h
> +++ b/arch/x86/kvm/mmu/mmu_internal.h
> @@ -244,7 +244,7 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vc=
pu *vcpu, gpa_t cr2_or_gpa,
>  		.is_tdp =3D likely(vcpu->arch.mmu->page_fault =3D=3D kvm_tdp_page_faul=
t),
>  		.nx_huge_page_workaround_enabled =3D is_nx_huge_page_enabled(),
> =20
> -		.max_level =3D KVM_MAX_HUGEPAGE_LEVEL,
> +		.max_level =3D vcpu->kvm->arch.tdp_max_page_level,
>  		.req_level =3D PG_LEVEL_4K,
>  		.goal_level =3D PG_LEVEL_4K,
>  	};