| Message ID | 20200302235709.27467-37-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Introduce KVM cpu caps | expand |
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). 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
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 >
On 03/03/20 16:35, Sean Christopherson wrote: > 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. Yes, exactly. Paolo
On Tue, Mar 03, 2020 at 04:40:32PM +0100, Paolo Bonzini wrote: > On 03/03/20 16:35, Sean Christopherson wrote: > > 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. > > Yes, exactly. I'll tack that on to the front of the series. Should it be tagged Fixes? Feels like a fix, but is also more than a bit scary.
On 03/03/20 16:44, Sean Christopherson wrote: >>> 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. >> Yes, exactly. > I'll tack that on to the front of the series. Should it be tagged Fixes? > Feels like a fix, but is also more than a bit scary. If you don't mind, I prefer to do the changes myself and also fix the conflicts, in order to get my feet wet in the new cpu_caps world. I'll push it to a temporary branch for you to take a look, possibly tomorrow. Paolo
On Tue, Mar 03, 2020 at 04:47:46PM +0100, Paolo Bonzini wrote: > On 03/03/20 16:44, Sean Christopherson wrote: > >>> 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. > >> Yes, exactly. > > I'll tack that on to the front of the series. Should it be tagged Fixes? > > Feels like a fix, but is also more than a bit scary. > > If you don't mind, I prefer to do the changes myself and also fix the > conflicts, in order to get my feet wet in the new cpu_caps world. I'll > push it to a temporary branch for you to take a look, possibly tomorrow. That would be wonderful, thanks!
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 84b9a488a443..aacfd6af774a 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; diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 1a4ac20797a4..131f4b88d307 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -7145,6 +7145,8 @@ static void vmx_set_supported_cpuid(struct kvm_cpuid_entry2 *entry) case 0x80000001: if (!cpu_has_vmx_rdtscp()) cpuid_entry_clear(entry, X86_FEATURE_RDTSCP); + if (enable_ept && !cpu_has_vmx_ept_1g_page()) + cpuid_entry_clear(entry, X86_FEATURE_GBPAGES); break; default: break;