Re: [PATCH v3 02/15] KVM: x86/mmu: Use KVM-governed feature framework to track "GBPAGES enabled"

[Yuan Yao]

On Tue, Aug 15, 2023 at 01:36:40PM -0700, Sean Christopherson wrote:
> Use the governed feature framework to track whether or not the guest can
> use 1GiB pages, and drop the one-off helper that wraps the surprisingly
> non-trivial logic surrounding 1GiB page usage in the guest.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/kvm/cpuid.c             | 17 +++++++++++++++++
>  arch/x86/kvm/governed_features.h |  2 ++
>  arch/x86/kvm/mmu/mmu.c           | 20 +++-----------------
>  3 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 4ba43ae008cb..67e9f79fe059 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -312,11 +312,28 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  	struct kvm_cpuid_entry2 *best;
> +	bool allow_gbpages;
>
>  	BUILD_BUG_ON(KVM_NR_GOVERNED_FEATURES > KVM_MAX_NR_GOVERNED_FEATURES);
>  	bitmap_zero(vcpu->arch.governed_features.enabled,
>  		    KVM_MAX_NR_GOVERNED_FEATURES);
>
> +	/*
> +	 * If TDP is enabled, let the guest use GBPAGES if they're supported in
> +	 * hardware.  The hardware page walker doesn't let KVM disable GBPAGES,
> +	 * i.e. won't treat them as reserved, and KVM doesn't redo the GVA->GPA
> +	 * walk for performance and complexity reasons.  Not to mention KVM
> +	 * _can't_ solve the problem because GVA->GPA walks aren't visible to
> +	 * KVM once a TDP translation is installed.  Mimic hardware behavior so
> +	 * that KVM's is at least consistent, i.e. doesn't randomly inject #PF.
> +	 * If TDP is disabled, honor *only* guest CPUID as KVM has full control
> +	 * and can install smaller shadow pages if the host lacks 1GiB support.
> +	 */
> +	allow_gbpages = tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
> +				      guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);

tdp_enabled only changes at kvm_configure_mmu() when hardware setup for
VMX and SVM, so:

Reviewed-by: Yuan Yao <yuan.yao@xxxxxxxxx>

> +	if (allow_gbpages)
> +		kvm_governed_feature_set(vcpu, X86_FEATURE_GBPAGES);
> +
>  	best = kvm_find_cpuid_entry(vcpu, 1);
>  	if (best && apic) {
>  		if (cpuid_entry_has(best, X86_FEATURE_TSC_DEADLINE_TIMER))
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> index 40ce8e6608cd..b29c15d5e038 100644
> --- a/arch/x86/kvm/governed_features.h
> +++ b/arch/x86/kvm/governed_features.h
> @@ -5,5 +5,7 @@ BUILD_BUG()
>
>  #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
>
> +KVM_GOVERNED_X86_FEATURE(GBPAGES)
> +
>  #undef KVM_GOVERNED_X86_FEATURE
>  #undef KVM_GOVERNED_FEATURE
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 5bdda75bfd10..9e4cd8b4a202 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4779,28 +4779,13 @@ static void __reset_rsvds_bits_mask(struct rsvd_bits_validate *rsvd_check,
>  	}
>  }
>
> -static bool guest_can_use_gbpages(struct kvm_vcpu *vcpu)
> -{
> -	/*
> -	 * If TDP is enabled, let the guest use GBPAGES if they're supported in
> -	 * hardware.  The hardware page walker doesn't let KVM disable GBPAGES,
> -	 * i.e. won't treat them as reserved, and KVM doesn't redo the GVA->GPA
> -	 * walk for performance and complexity reasons.  Not to mention KVM
> -	 * _can't_ solve the problem because GVA->GPA walks aren't visible to
> -	 * KVM once a TDP translation is installed.  Mimic hardware behavior so
> -	 * that KVM's is at least consistent, i.e. doesn't randomly inject #PF.
> -	 */
> -	return tdp_enabled ? boot_cpu_has(X86_FEATURE_GBPAGES) :
> -			     guest_cpuid_has(vcpu, X86_FEATURE_GBPAGES);
> -}
> -
>  static void reset_guest_rsvds_bits_mask(struct kvm_vcpu *vcpu,
>  					struct kvm_mmu *context)
>  {
>  	__reset_rsvds_bits_mask(&context->guest_rsvd_check,
>  				vcpu->arch.reserved_gpa_bits,
>  				context->cpu_role.base.level, is_efer_nx(context),
> -				guest_can_use_gbpages(vcpu),
> +				guest_can_use(vcpu, X86_FEATURE_GBPAGES),
>  				is_cr4_pse(context),
>  				guest_cpuid_is_amd_or_hygon(vcpu));
>  }
> @@ -4877,7 +4862,8 @@ static void reset_shadow_zero_bits_mask(struct kvm_vcpu *vcpu,
>  	__reset_rsvds_bits_mask(shadow_zero_check, reserved_hpa_bits(),
>  				context->root_role.level,
>  				context->root_role.efer_nx,
> -				guest_can_use_gbpages(vcpu), is_pse, is_amd);
> +				guest_can_use(vcpu, X86_FEATURE_GBPAGES),
> +				is_pse, is_amd);
>
>  	if (!shadow_me_mask)
>  		return;
> --
> 2.41.0.694.ge786442a9b-goog
>