| Message ID | 20200201185218.24473-40-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Introduce KVM cpu caps | expand |
Sean Christopherson <sean.j.christopherson@intel.com> writes: > Use the recently introduced KVM CPU caps to propagate SVM-only (kernel) > settings to supported CPUID flags. > > Note, setting a flag based on a *different* feature is effectively > emulation, and so must be done at runtime via ->set_supported_cpuid(). > > Opportunistically add a technically unnecessary break and fix an > indentation issue in svm_set_supported_cpuid(). > > No functional change intended. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/svm.c | 40 +++++++++++++++++++++++----------------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 630520f8adfa..f98a192459f7 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -1350,6 +1350,25 @@ static __init void svm_adjust_mmio_mask(void) > kvm_mmu_set_mmio_spte_mask(mask, mask, PT_WRITABLE_MASK | PT_USER_MASK); > } > Can we probably add the comment about what can be done here and what needs to go to svm_set_supported_cpuid()? (The one about 'emulation' from your commit message would do). > +static __init void svm_set_cpu_caps(void) > +{ > + /* CPUID 0x1 */ > + if (avic) > + kvm_cpu_cap_clear(X86_FEATURE_X2APIC); > + > + /* CPUID 0x80000001 */ > + if (nested) > + kvm_cpu_cap_set(X86_FEATURE_SVM); > + > + /* CPUID 0x8000000A */ > + /* Support next_rip if host supports it */ > + if (boot_cpu_has(X86_FEATURE_NRIPS)) > + kvm_cpu_cap_set(X86_FEATURE_NRIPS); Unrelated to your patch but the way we handle 'nrips' is a bit weird: we can disable it with 'nrips' module parameter but L1 hypervisor will get it unconditionally. Also, what about all the rest of 0x8000000A.EDX features? Nested SVM would appreciate some love... > + > + if (npt_enabled) > + kvm_cpu_cap_set(X86_FEATURE_NPT); > +} > + > static __init int svm_hardware_setup(void) > { > int cpu; > @@ -1462,6 +1481,8 @@ static __init int svm_hardware_setup(void) > pr_info("Virtual GIF supported\n"); > } > > + svm_set_cpu_caps(); > + > return 0; > > err: > @@ -6033,17 +6054,9 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu) > static void svm_set_supported_cpuid(struct kvm_cpuid_entry2 *entry) > { > switch (entry->function) { > - case 0x1: > - if (avic) > - cpuid_entry_clear(entry, X86_FEATURE_X2APIC); > - break; > - case 0x80000001: > - if (nested) > - cpuid_entry_set(entry, X86_FEATURE_SVM); > - break; > case 0x80000008: > if (boot_cpu_has(X86_FEATURE_LS_CFG_SSBD) || > - boot_cpu_has(X86_FEATURE_AMD_SSBD)) > + boot_cpu_has(X86_FEATURE_AMD_SSBD)) > cpuid_entry_set(entry, X86_FEATURE_VIRT_SSBD); > break; > case 0x8000000A: > @@ -6053,14 +6066,7 @@ static void svm_set_supported_cpuid(struct kvm_cpuid_entry2 *entry) > entry->ecx = 0; /* Reserved */ > entry->edx = 0; /* Per default do not support any > additional features */ > - > - /* Support next_rip if host supports it */ > - if (boot_cpu_has(X86_FEATURE_NRIPS)) > - cpuid_entry_set(entry, X86_FEATURE_NRIPS); > - > - /* Support NPT for the guest if enabled */ > - if (npt_enabled) > - cpuid_entry_set(entry, X86_FEATURE_NPT); > + break; > } > } Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
On 01/02/20 19:51, Sean Christopherson wrote: > + /* CPUID 0x8000000A */ > + /* Support next_rip if host supports it */ > + if (boot_cpu_has(X86_FEATURE_NRIPS)) > + kvm_cpu_cap_set(X86_FEATURE_NRIPS); Should this also be conditional on "nested"? Paolo
On Tue, Feb 25, 2020 at 04:10:18PM +0100, Paolo Bonzini wrote: > On 01/02/20 19:51, Sean Christopherson wrote: > > + /* CPUID 0x8000000A */ > > + /* Support next_rip if host supports it */ > > + if (boot_cpu_has(X86_FEATURE_NRIPS)) > > + kvm_cpu_cap_set(X86_FEATURE_NRIPS); > > Should this also be conditional on "nested"? I think that makes sense? AFAICT it should probably be conditional on "nrips" as well. X86_FEATURE_NPT should also be conditional on "nested". I'll tack on a patch to make those changes, the cleanup is easier without the things spread across different case statements, e.g. wrap the entire SVM feature leaf in "if (nested)".
On Thu, Feb 27, 2020 at 04:28:33PM -0800, Sean Christopherson wrote: > On Tue, Feb 25, 2020 at 04:10:18PM +0100, Paolo Bonzini wrote: > > On 01/02/20 19:51, Sean Christopherson wrote: > > > + /* CPUID 0x8000000A */ > > > + /* Support next_rip if host supports it */ > > > + if (boot_cpu_has(X86_FEATURE_NRIPS)) > > > + kvm_cpu_cap_set(X86_FEATURE_NRIPS); > > > > Should this also be conditional on "nested"? > > I think that makes sense? AFAICT it should probably be conditional on > "nrips" as well. X86_FEATURE_NPT should also be conditional on "nested". > I'll tack on a patch to make those changes, the cleanup is easier without > the things spread across different case statements, e.g. wrap the entire > SVM feature leaf in "if (nested)". Regarding NRIPS, the original commit added the "Support next_rip if host supports it" comment, but I can't tell is "host supports" means "supported in hardware" or "supported by KVM". In other words, should I make the cap dependent "nrips" or leave it as is?
On 28/02/20 01:36, Sean Christopherson wrote: > Regarding NRIPS, the original commit added the "Support next_rip if host > supports it" comment, but I can't tell is "host supports" means "supported > in hardware" or "supported by KVM". In other words, should I make the cap > dependent "nrips" or leave it as is? > The "nrips" parameter came later. For VMX we generally remove guest capabilities if the corresponding parameter is on, so it's a good idea to do the same for SVM. Paolo
On Fri, Feb 28, 2020 at 08:03:33AM +0100, Paolo Bonzini wrote: > On 28/02/20 01:36, Sean Christopherson wrote: > > Regarding NRIPS, the original commit added the "Support next_rip if host > > supports it" comment, but I can't tell is "host supports" means "supported > > in hardware" or "supported by KVM". In other words, should I make the cap > > dependent "nrips" or leave it as is? > > > > The "nrips" parameter came later. For VMX we generally remove guest > capabilities if the corresponding parameter is on, so it's a good idea > to do the same for SVM. Huh. I swear I looked at the code from the original commit and saw nrips there, but it was clearly added in 2019 via commit d647eb63e671 ("KVM: svm: add nrips module parameter").
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c index 630520f8adfa..f98a192459f7 100644 --- a/arch/x86/kvm/svm.c +++ b/arch/x86/kvm/svm.c @@ -1350,6 +1350,25 @@ static __init void svm_adjust_mmio_mask(void) kvm_mmu_set_mmio_spte_mask(mask, mask, PT_WRITABLE_MASK | PT_USER_MASK); } +static __init void svm_set_cpu_caps(void) +{ + /* CPUID 0x1 */ + if (avic) + kvm_cpu_cap_clear(X86_FEATURE_X2APIC); + + /* CPUID 0x80000001 */ + if (nested) + kvm_cpu_cap_set(X86_FEATURE_SVM); + + /* CPUID 0x8000000A */ + /* Support next_rip if host supports it */ + if (boot_cpu_has(X86_FEATURE_NRIPS)) + kvm_cpu_cap_set(X86_FEATURE_NRIPS); + + if (npt_enabled) + kvm_cpu_cap_set(X86_FEATURE_NPT); +} + static __init int svm_hardware_setup(void) { int cpu; @@ -1462,6 +1481,8 @@ static __init int svm_hardware_setup(void) pr_info("Virtual GIF supported\n"); } + svm_set_cpu_caps(); + return 0; err: @@ -6033,17 +6054,9 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu) static void svm_set_supported_cpuid(struct kvm_cpuid_entry2 *entry) { switch (entry->function) { - case 0x1: - if (avic) - cpuid_entry_clear(entry, X86_FEATURE_X2APIC); - break; - case 0x80000001: - if (nested) - cpuid_entry_set(entry, X86_FEATURE_SVM); - break; case 0x80000008: if (boot_cpu_has(X86_FEATURE_LS_CFG_SSBD) || - boot_cpu_has(X86_FEATURE_AMD_SSBD)) + boot_cpu_has(X86_FEATURE_AMD_SSBD)) cpuid_entry_set(entry, X86_FEATURE_VIRT_SSBD); break; case 0x8000000A: @@ -6053,14 +6066,7 @@ static void svm_set_supported_cpuid(struct kvm_cpuid_entry2 *entry) entry->ecx = 0; /* Reserved */ entry->edx = 0; /* Per default do not support any additional features */ - - /* Support next_rip if host supports it */ - if (boot_cpu_has(X86_FEATURE_NRIPS)) - cpuid_entry_set(entry, X86_FEATURE_NRIPS); - - /* Support NPT for the guest if enabled */ - if (npt_enabled) - cpuid_entry_set(entry, X86_FEATURE_NPT); + break; } }
Use the recently introduced KVM CPU caps to propagate SVM-only (kernel) settings to supported CPUID flags. Note, setting a flag based on a *different* feature is effectively emulation, and so must be done at runtime via ->set_supported_cpuid(). Opportunistically add a technically unnecessary break and fix an indentation issue in svm_set_supported_cpuid(). No functional change intended. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/svm.c | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-)