[39/61] KVM: SVM: Convert feature updates from CPUID to KVM cpu caps
diff mbox series

Message ID 20200201185218.24473-40-sean.j.christopherson@intel.com
State New
Headers show
Series
  • KVM: x86: Introduce KVM cpu caps
Related show

Commit Message

Sean Christopherson Feb. 1, 2020, 6:51 p.m. UTC
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(-)

Comments

Vitaly Kuznetsov Feb. 24, 2020, 9:33 p.m. UTC | #1
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>
Paolo Bonzini Feb. 25, 2020, 3:10 p.m. UTC | #2
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
Sean Christopherson Feb. 28, 2020, 12:28 a.m. UTC | #3
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)".
Sean Christopherson Feb. 28, 2020, 12:36 a.m. UTC | #4
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?
Paolo Bonzini Feb. 28, 2020, 7:03 a.m. UTC | #5
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
Sean Christopherson Feb. 28, 2020, 3:09 p.m. UTC | #6
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").

Patch
diff mbox series

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;
 	}
 }