Re: [PATCH v2 36/66] KVM: x86: Handle GBPAGE CPUID adjustment for EPT in VMX code

[Sean Christopherson]

On Tue, Mar 03, 2020 at 03:59:14PM +0100, Paolo Bonzini wrote:
> On 03/03/20 00:56, Sean Christopherson wrote:
> > Move the clearing of the GBPAGE CPUID bit into VMX to eliminate an
> > instance of the undesirable "unsigned f_* = *_supported ? F(*) : 0"
> > pattern in the common CPUID handling code, and to pave the way toward
> > eliminating ->get_lpage_level().
> > 
> > No functional change intended.
> 
> And no functional change is done indeed but there is a preexisting bug 
> that should be fixed.
> 
> cpu_has_vmx_ept_1g_page() has no relationship to whether 1GB pages should be
> marked as supported in CPUID.  This has no ill effect because we're only
> clearing the bit, but it results in 1GB pages not being available when
> EPT is disabled (even though they are actually supported thanks to
> shadowing).

Oof, that took me a long time to process.  You're saying that KVM can
allow the guest to use GBPAGES when shadow paging is enabled because KVM
can effectively emulate GBPAGES.  And IIUC, you're also saying that
cpuid.GBPAGES should never be influenced by EPT restrictions.

That all makes sense.

> The right fix should be this:
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 84b9a488a443..8bbba8eb4ce5 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -416,8 +416,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  	int r, i, max_idx;
>  	unsigned f_nx = is_efer_nx() ? F(NX) : 0;
>  #ifdef CONFIG_X86_64
> -	unsigned f_gbpages = (kvm_x86_ops->get_lpage_level() == PT_PDPE_LEVEL)
> -				? F(GBPAGES) : 0;
> +	unsigned f_gbpages = F(GBPAGES);
>  	unsigned f_lm = F(LM);
>  #else
>  	unsigned f_gbpages = 0;
> @@ -691,6 +690,8 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>  	case 0x80000001:
>  		entry->edx &= kvm_cpuid_8000_0001_edx_x86_features;
>  		cpuid_entry_mask(entry, CPUID_8000_0001_EDX);
> +		if (!tdp_enabled)
> +			cpuid_entry_set(entry, X86_FEATURE_GBPAGES);
>  		entry->ecx &= kvm_cpuid_8000_0001_ecx_x86_features;
>  		cpuid_entry_mask(entry, CPUID_8000_0001_ECX);
>  		break;
> 
> Paolo
>