diff mbox series

[v3,9/9] KVM: x86: Add XSAVE Support for Architectural LBRs

Message ID 20210303135756.1546253-10-like.xu@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series KVM: x86/pmu: Guest Architectural LBR Enabling | expand

Commit Message

Like Xu March 3, 2021, 1:57 p.m. UTC 
On processors whose XSAVE feature set supports XSAVES and XRSTORS,
the availability of support for Architectural LBR configuration state save
and restore can be determined from CPUID.(EAX=0DH, ECX=1):EDX:ECX[bit 15].
The detailed leaf for Arch LBRs is enumerated in CPUID.(EAX=0DH, ECX=0FH).

XSAVES provides a faster means than RDMSR for guest to read all LBRs.
When guest IA32_XSS[bit 15] is set, the Arch LBRs state can be saved using
XSAVES and restored by XRSTORS with the appropriate RFBM.

If the KVM fails to pass-through the LBR msrs to the guest, the LBR msrs
will be reset to prevent the leakage of host records via XSAVES. In this
case, the guest results may be inaccurate as the legacy LBR.

Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 arch/x86/kvm/vmx/pmu_intel.c | 2 ++
 arch/x86/kvm/vmx/vmx.c       | 2 ++
 arch/x86/kvm/x86.c           | 2 ++
 3 files changed, 6 insertions(+)

Comments

Sean Christopherson March 3, 2021, 6:03 p.m. UTC | #1 
On Wed, Mar 03, 2021, Like Xu wrote:
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 034708a3df20..ec4593e0ee6d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7268,6 +7268,8 @@ static __init void vmx_set_cpu_caps(void)
>  	supported_xss = 0;
>  	if (!cpu_has_vmx_xsaves())
>  		kvm_cpu_cap_clear(X86_FEATURE_XSAVES);
> +	else if (kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
> +		supported_xss |= XFEATURE_MASK_LBR;
>  
>  	/* CPUID 0x80000001 */
>  	if (!cpu_has_vmx_rdtscp())
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index d773836ceb7a..bca2e318ff24 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10433,6 +10433,8 @@ int kvm_arch_hardware_setup(void *opaque)
>  
>  	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
>  		supported_xss = 0;
> +	else
> +		supported_xss &= host_xss;

Not your fault by any means, but I would prefer to have matching logic for XSS
and XCR0.  The existing clearing of supported_xss here is pointless.  E.g. I'd
prefer something like the following, though Paolo may have a different opinion.

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 6d7e760fdfa0..c781034463e5 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7244,12 +7244,15 @@ static __init void vmx_set_cpu_caps(void)
                kvm_cpu_cap_clear(X86_FEATURE_INVPCID);
        if (vmx_pt_mode_is_host_guest())
                kvm_cpu_cap_check_and_set(X86_FEATURE_INTEL_PT);
+       if (!cpu_has_vmx_arch_lbr()) {
+               kvm_cpu_cap_clear(X86_FEATURE_ARCH_LBR);
+               supported_xss &= ~XFEATURE_MASK_LBR;
+       }

        if (vmx_umip_emulated())
                kvm_cpu_cap_set(X86_FEATURE_UMIP);

        /* CPUID 0xD.1 */
-       supported_xss = 0;
        if (!cpu_has_vmx_xsaves())
                kvm_cpu_cap_clear(X86_FEATURE_XSAVES);

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7b0adebec1ef..5f9eb1f5b840 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -205,6 +205,8 @@ static struct kvm_user_return_msrs __percpu *user_return_msrs;
                                | XFEATURE_MASK_BNDCSR | XFEATURE_MASK_AVX512 \
                                | XFEATURE_MASK_PKRU)

+#define KVM_SUPPORTED_XSS      XFEATURE_MASK_LBR
+
 u64 __read_mostly host_efer;
 EXPORT_SYMBOL_GPL(host_efer);

@@ -8037,6 +8039,11 @@ int kvm_arch_init(void *opaque)
                supported_xcr0 = host_xcr0 & KVM_SUPPORTED_XCR0;
        }

+       if (boot_cpu_has(X86_FEATURE_XSAVES))
+               rdmsrl(MSR_IA32_XSS, host_xss);
+               supported_xss = host_xss & KVM_SUPPORTED_XSS;
+       }
+
        if (pi_inject_timer == -1)
                pi_inject_timer = housekeeping_enabled(HK_FLAG_TIMER);
 #ifdef CONFIG_X86_64
@@ -10412,9 +10419,6 @@ int kvm_arch_hardware_setup(void *opaque)

        rdmsrl_safe(MSR_EFER, &host_efer);

-       if (boot_cpu_has(X86_FEATURE_XSAVES))
-               rdmsrl(MSR_IA32_XSS, host_xss);
-
        r = ops->hardware_setup();
        if (r != 0)
                return r;
@@ -10422,9 +10426,6 @@ int kvm_arch_hardware_setup(void *opaque)
        memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
        kvm_ops_static_call_update();

-       if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
-               supported_xss = 0;
-
 #define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
        cr4_reserved_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_);
 #undef __kvm_cpu_cap_has
Like Xu March 4, 2021, 3:43 a.m. UTC | #2 
On 2021/3/4 2:03, Sean Christopherson wrote:
> On Wed, Mar 03, 2021, Like Xu wrote:
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 034708a3df20..ec4593e0ee6d 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -7268,6 +7268,8 @@ static __init void vmx_set_cpu_caps(void)
>>   	supported_xss = 0;
>>   	if (!cpu_has_vmx_xsaves())
>>   		kvm_cpu_cap_clear(X86_FEATURE_XSAVES);
>> +	else if (kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
>> +		supported_xss |= XFEATURE_MASK_LBR;
>>   
>>   	/* CPUID 0x80000001 */
>>   	if (!cpu_has_vmx_rdtscp())
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index d773836ceb7a..bca2e318ff24 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -10433,6 +10433,8 @@ int kvm_arch_hardware_setup(void *opaque)
>>   
>>   	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
>>   		supported_xss = 0;
>> +	else
>> +		supported_xss &= host_xss;
> 
> Not your fault by any means, but I would prefer to have matching logic for XSS
> and XCR0.  The existing clearing of supported_xss here is pointless.  E.g. I'd
> prefer something like the following, though Paolo may have a different opinion.

I have no preference for where to do rdmsrl() in kvm_arch_init()
or kvm_arch_hardware_setup().

It's true the assignment of supported_xss in the kvm/intel
tree is redundant and introducing KVM_SUPPORTED_XSS is also fine to me.


> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6d7e760fdfa0..c781034463e5 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7244,12 +7244,15 @@ static __init void vmx_set_cpu_caps(void)
>                  kvm_cpu_cap_clear(X86_FEATURE_INVPCID);
>          if (vmx_pt_mode_is_host_guest())
>                  kvm_cpu_cap_check_and_set(X86_FEATURE_INTEL_PT);
> +       if (!cpu_has_vmx_arch_lbr()) {
> +               kvm_cpu_cap_clear(X86_FEATURE_ARCH_LBR);
> +               supported_xss &= ~XFEATURE_MASK_LBR;
> +       }
> 

I will move the above part to the LBR patch
and leave the left part as a pre-patch for Paolo's review.

>          if (vmx_umip_emulated())
>                  kvm_cpu_cap_set(X86_FEATURE_UMIP);
> 
>          /* CPUID 0xD.1 */
> -       supported_xss = 0;
>          if (!cpu_has_vmx_xsaves())
>                  kvm_cpu_cap_clear(X86_FEATURE_XSAVES);

if (!cpu_has_vmx_xsaves())
	supported_xss = 0;
	kvm_cpu_cap_clear(X86_FEATURE_XSAVES);

> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7b0adebec1ef..5f9eb1f5b840 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -205,6 +205,8 @@ static struct kvm_user_return_msrs __percpu *user_return_msrs;
>                                  | XFEATURE_MASK_BNDCSR | XFEATURE_MASK_AVX512 \
>                                  | XFEATURE_MASK_PKRU)
> 
> +#define KVM_SUPPORTED_XSS      XFEATURE_MASK_LBR
> +
>   u64 __read_mostly host_efer;
>   EXPORT_SYMBOL_GPL(host_efer);
> 
> @@ -8037,6 +8039,11 @@ int kvm_arch_init(void *opaque)
>                  supported_xcr0 = host_xcr0 & KVM_SUPPORTED_XCR0;
>          }
> 
> +       if (boot_cpu_has(X86_FEATURE_XSAVES))

{

> +               rdmsrl(MSR_IA32_XSS, host_xss);
> +               supported_xss = host_xss & KVM_SUPPORTED_XSS;
> +       }
> +
>          if (pi_inject_timer == -1)
>                  pi_inject_timer = housekeeping_enabled(HK_FLAG_TIMER);
>   #ifdef CONFIG_X86_64
> @@ -10412,9 +10419,6 @@ int kvm_arch_hardware_setup(void *opaque)
> 
>          rdmsrl_safe(MSR_EFER, &host_efer);
> 
> -       if (boot_cpu_has(X86_FEATURE_XSAVES))
> -               rdmsrl(MSR_IA32_XSS, host_xss);
> -
>          r = ops->hardware_setup();
>          if (r != 0)
>                  return r;
> @@ -10422,9 +10426,6 @@ int kvm_arch_hardware_setup(void *opaque)
>          memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
>          kvm_ops_static_call_update();
> 
> -       if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
> -               supported_xss = 0;
> -
>   #define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
>          cr4_reserved_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_);
>   #undef __kvm_cpu_cap_has
>
Sean Christopherson March 4, 2021, 4:31 p.m. UTC | #3 
On Thu, Mar 04, 2021, Like Xu wrote:
> On 2021/3/4 2:03, Sean Christopherson wrote:
> >          if (vmx_umip_emulated())
> >                  kvm_cpu_cap_set(X86_FEATURE_UMIP);
> > 
> >          /* CPUID 0xD.1 */
> > -       supported_xss = 0;
> >          if (!cpu_has_vmx_xsaves())
> >                  kvm_cpu_cap_clear(X86_FEATURE_XSAVES);
> 
> if (!cpu_has_vmx_xsaves())
> 	supported_xss = 0;

Argh, I forgot XSAVES has a VMCS control.  That's why kvm_arch_hardware_setup()
clears supported_xss if !XSAVES.  I guess just leave that existing code, but
maybe add a comment.

Paolo, any thoughts on how to keep supported_xss aligned with support_xcr0,
without spreading the logic around too much?

> 	kvm_cpu_cap_clear(X86_FEATURE_XSAVES);
> 
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 7b0adebec1ef..5f9eb1f5b840 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -205,6 +205,8 @@ static struct kvm_user_return_msrs __percpu *user_return_msrs;
> >                                  | XFEATURE_MASK_BNDCSR | XFEATURE_MASK_AVX512 \
> >                                  | XFEATURE_MASK_PKRU)
> > 
> > +#define KVM_SUPPORTED_XSS      XFEATURE_MASK_LBR
> > +
> >   u64 __read_mostly host_efer;
> >   EXPORT_SYMBOL_GPL(host_efer);
> > 
> > @@ -8037,6 +8039,11 @@ int kvm_arch_init(void *opaque)
> >                  supported_xcr0 = host_xcr0 & KVM_SUPPORTED_XCR0;
> >          }
> > 
> > +       if (boot_cpu_has(X86_FEATURE_XSAVES))
> 
> {
> 
> > +               rdmsrl(MSR_IA32_XSS, host_xss);
> > +               supported_xss = host_xss & KVM_SUPPORTED_XSS;
> > +       }
> > +
> >          if (pi_inject_timer == -1)
> >                  pi_inject_timer = housekeeping_enabled(HK_FLAG_TIMER);
> >   #ifdef CONFIG_X86_64
> > @@ -10412,9 +10419,6 @@ int kvm_arch_hardware_setup(void *opaque)
> > 
> >          rdmsrl_safe(MSR_EFER, &host_efer);
> > 
> > -       if (boot_cpu_has(X86_FEATURE_XSAVES))
> > -               rdmsrl(MSR_IA32_XSS, host_xss);
> > -
> >          r = ops->hardware_setup();
> >          if (r != 0)
> >                  return r;
> > @@ -10422,9 +10426,6 @@ int kvm_arch_hardware_setup(void *opaque)
> >          memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
> >          kvm_ops_static_call_update();
> > 
> > -       if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
> > -               supported_xss = 0;
> > -
> >   #define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
> >          cr4_reserved_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_);
> >   #undef __kvm_cpu_cap_has
> > 
>
Xu, Like March 5, 2021, 2:57 a.m. UTC | #4 
On 2021/3/5 0:31, Sean Christopherson wrote:
> Paolo, any thoughts on how to keep supported_xss aligned with support_xcr0,
> without spreading the logic around too much?

 From 58be4152ced441395dfc439f446c5ad53bd48576 Mon Sep 17 00:00:00 2001
From: Like Xu <like.xu@linux.intel.com>
Date: Thu, 4 Mar 2021 13:21:38 +0800
Subject: [PATCH] KVM: x86: Refine the matching and clearing logic for 
supported_xss

"The existing clearing of supported_xss here is pointless".
Let's refine the code path in this way: initialize the supported_xss
with the filter of KVM_SUPPORTED_XSS mask and update its value in
a bit clear manner (rather than bit setting).

Before:
(1) kvm_arch_hardware_setup
     if (boot_cpu_has(X86_FEATURE_XSAVES))
         rdmsrl(MSR_IA32_XSS, host_xss);
     if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
         supported_xss = 0;
     else supported_xss &= host_xss;
(2) vmx_set_cpu_caps
     supported_xss = 0;
     if (!cpu_has_vmx_xsaves())
         kvm_cpu_cap_clear(X86_FEATURE_XSAVES);
     else set available bits to supported_xss

After:
(1) kvm_arch_init
     if (boot_cpu_has(X86_FEATURE_XSAVES))
         rdmsrl(MSR_IA32_XSS, host_xss);
         supported_xss = host_xss & KVM_SUPPORTED_XSS;
(2) vmx_set_cpu_caps
     if (!cpu_has_vmx_xsaves())
         kvm_cpu_cap_clear(X86_FEATURE_XSAVES);
         supported_xss = 0;
     else clear un-available bits for supported_xss

Suggested-by: Sean Christopherson <seanjc@google.com>
Original-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
  arch/x86/kvm/vmx/vmx.c |  5 +++--
  arch/x86/kvm/x86.c     | 13 +++++++------
  2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4bc4bb49aaa9..8706323547c4 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7288,9 +7288,10 @@ static __init void vmx_set_cpu_caps(void)
          kvm_cpu_cap_set(X86_FEATURE_UMIP);

      /* CPUID 0xD.1 */
-    supported_xss = 0;
-    if (!cpu_has_vmx_xsaves())
+    if (!cpu_has_vmx_xsaves()) {
          kvm_cpu_cap_clear(X86_FEATURE_XSAVES);
+        supported_xss = 0;
+    }

      /* CPUID 0x80000001 */
      if (!cpu_has_vmx_rdtscp())
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d773836ceb7a..99cb62035bb2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -205,6 +205,8 @@ static struct kvm_user_return_msrs __percpu 
*user_return_msrs;
                  | XFEATURE_MASK_BNDCSR | XFEATURE_MASK_AVX512 \
                  | XFEATURE_MASK_PKRU)

+#define KVM_SUPPORTED_XSS     0
+
  u64 __read_mostly host_efer;
  EXPORT_SYMBOL_GPL(host_efer);

@@ -8046,6 +8048,11 @@ int kvm_arch_init(void *opaque)
          supported_xcr0 = host_xcr0 & KVM_SUPPORTED_XCR0;
      }

+    if (boot_cpu_has(X86_FEATURE_XSAVES)) {
+        rdmsrl(MSR_IA32_XSS, host_xss);
+        supported_xss = host_xss & KVM_SUPPORTED_XSS;
+    }
+
      if (pi_inject_timer == -1)
          pi_inject_timer = housekeeping_enabled(HK_FLAG_TIMER);
  #ifdef CONFIG_X86_64
@@ -10421,9 +10428,6 @@ int kvm_arch_hardware_setup(void *opaque)

      rdmsrl_safe(MSR_EFER, &host_efer);

-    if (boot_cpu_has(X86_FEATURE_XSAVES))
-        rdmsrl(MSR_IA32_XSS, host_xss);
-
      r = ops->hardware_setup();
      if (r != 0)
          return r;
@@ -10431,9 +10435,6 @@ int kvm_arch_hardware_setup(void *opaque)
      memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
      kvm_ops_static_call_update();

-    if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
-        supported_xss = 0;
-
  #define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
      cr4_reserved_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_);
  #undef __kvm_cpu_cap_has
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 48a817be60ab..08114f70c496 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -768,6 +768,8 @@  void vmx_passthrough_lbr_msrs(struct kvm_vcpu *vcpu)
 	return;
 
 warn:
+	if (kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
+		wrmsrl(MSR_ARCH_LBR_DEPTH, lbr_desc->records.nr);
 	pr_warn_ratelimited("kvm: vcpu-%d: fail to passthrough LBR.\n",
 		vcpu->vcpu_id);
 }
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 034708a3df20..ec4593e0ee6d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7268,6 +7268,8 @@  static __init void vmx_set_cpu_caps(void)
 	supported_xss = 0;
 	if (!cpu_has_vmx_xsaves())
 		kvm_cpu_cap_clear(X86_FEATURE_XSAVES);
+	else if (kvm_cpu_cap_has(X86_FEATURE_ARCH_LBR))
+		supported_xss |= XFEATURE_MASK_LBR;
 
 	/* CPUID 0x80000001 */
 	if (!cpu_has_vmx_rdtscp())
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d773836ceb7a..bca2e318ff24 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10433,6 +10433,8 @@  int kvm_arch_hardware_setup(void *opaque)
 
 	if (!kvm_cpu_cap_has(X86_FEATURE_XSAVES))
 		supported_xss = 0;
+	else
+		supported_xss &= host_xss;
 
 #define __kvm_cpu_cap_has(UNUSED_, f) kvm_cpu_cap_has(f)
 	cr4_reserved_bits = __cr4_reserved_bits(__kvm_cpu_cap_has, UNUSED_);