diff mbox series

[v10,10/27] KVM: x86: Refine xsave-managed guest register/MSR reset handling

Message ID 20240219074733.122080-11-weijiang.yang@intel.com
State New
Headers
Series Enable CET Virtualization |

Commit Message

Yang, Weijiang Feb. 19, 2024, 7:47 a.m. UTC 
  Tweak the code a bit to facilitate resetting more xstate components in
the future, e.g., CET's xstate-managed MSRs.

No functional change intended.

Suggested-by: Chao Gao <chao.gao@intel.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/kvm/x86.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

Comments

Chao Gao Feb. 20, 2024, 3:04 a.m. UTC | #1 
On Sun, Feb 18, 2024 at 11:47:16PM -0800, Yang Weijiang wrote:
>Tweak the code a bit to facilitate resetting more xstate components in
>the future, e.g., CET's xstate-managed MSRs.
>

>No functional change intended.

Strictly speaking, there is a functional change. in the previous logic, if
either of BNDCSR or BNDREGS state is not supported (kvm_mpx_supported() will
return false), KVM won't reset either of them. Since this gets changed, I vote
to drop 'No functional change ..'

>
>Suggested-by: Chao Gao <chao.gao@intel.com>
>Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>---
> arch/x86/kvm/x86.c | 30 +++++++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 10847e1cc413..5a9c07751c0e 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -12217,11 +12217,27 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
> 		static_branch_dec(&kvm_has_noapic_vcpu);
> }
> 
>+#define XSTATE_NEED_RESET_MASK	(XFEATURE_MASK_BNDREGS | \
>+				 XFEATURE_MASK_BNDCSR)
>+
>+static bool kvm_vcpu_has_xstate(unsigned long xfeature)

kvm_vcpu_has_xstate is a misnomer because it doesn't take a vCPU.

>+{
>+	switch (xfeature) {
>+	case XFEATURE_MASK_BNDREGS:
>+	case XFEATURE_MASK_BNDCSR:
>+		return kvm_cpu_cap_has(X86_FEATURE_MPX);
>+	default:
>+		return false;
>+	}
>+}
>+
> void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> {
> 	struct kvm_cpuid_entry2 *cpuid_0x1;
> 	unsigned long old_cr0 = kvm_read_cr0(vcpu);
>+	DECLARE_BITMAP(reset_mask, 64);
> 	unsigned long new_cr0;
>+	unsigned int i;
> 
> 	/*
> 	 * Several of the "set" flows, e.g. ->set_cr0(), read other registers
>@@ -12274,7 +12290,12 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> 	kvm_async_pf_hash_reset(vcpu);
> 	vcpu->arch.apf.halted = false;
> 
>-	if (vcpu->arch.guest_fpu.fpstate && kvm_mpx_supported()) {
>+	bitmap_from_u64(reset_mask, (kvm_caps.supported_xcr0 |
>+				     kvm_caps.supported_xss) &
>+				    XSTATE_NEED_RESET_MASK);
>+
>+	if (vcpu->arch.guest_fpu.fpstate &&
>+	    !bitmap_empty(reset_mask, XFEATURE_MAX)) {
> 		struct fpstate *fpstate = vcpu->arch.guest_fpu.fpstate;
> 
> 		/*
>@@ -12284,8 +12305,11 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> 		if (init_event)
> 			kvm_put_guest_fpu(vcpu);
> 
>-		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDREGS);
>-		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDCSR);
>+		for_each_set_bit(i, reset_mask, XFEATURE_MAX) {
>+			if (!kvm_vcpu_has_xstate(i))
>+				continue;

The kvm_vcpu_has_xstate() check is superfluous because @i is derived from
kvm_caps.supported_xcr0/xss, which already guarantees that all unsupported
xfeatures are filtered out.

I recommend dropping this check. w/ this change,

Reviewed-by: Chao Gao <chao.gao@intel.com>

>+			fpstate_clear_xstate_component(fpstate, i);
>+		}
> 
> 		if (init_event)
> 			kvm_load_guest_fpu(vcpu);
>-- 
>2.43.0
>
Yang, Weijiang Feb. 20, 2024, 1:23 p.m. UTC | #2 
On 2/20/2024 11:04 AM, Chao Gao wrote:
> On Sun, Feb 18, 2024 at 11:47:16PM -0800, Yang Weijiang wrote:
>> Tweak the code a bit to facilitate resetting more xstate components in
>> the future, e.g., CET's xstate-managed MSRs.
>>
>> No functional change intended.
> Strictly speaking, there is a functional change. in the previous logic, if
> either of BNDCSR or BNDREGS state is not supported (kvm_mpx_supported() will
> return false), KVM won't reset either of them.
> Since this gets changed, I vote
> to drop 'No functional change ..'

Yes, I'll remove it since existing logic is slightly changed.

>> Suggested-by: Chao Gao <chao.gao@intel.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>> arch/x86/kvm/x86.c | 30 +++++++++++++++++++++++++++---
>> 1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 10847e1cc413..5a9c07751c0e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -12217,11 +12217,27 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
>> 		static_branch_dec(&kvm_has_noapic_vcpu);
>> }
>>
>> +#define XSTATE_NEED_RESET_MASK	(XFEATURE_MASK_BNDREGS | \
>> +				 XFEATURE_MASK_BNDCSR)
>> +
>> +static bool kvm_vcpu_has_xstate(unsigned long xfeature)
> kvm_vcpu_has_xstate is a misnomer because it doesn't take a vCPU.

True, I'll change it, thanks!

>> +{
>> +	switch (xfeature) {
>> +	case XFEATURE_MASK_BNDREGS:
>> +	case XFEATURE_MASK_BNDCSR:
>> +		return kvm_cpu_cap_has(X86_FEATURE_MPX);
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>> {
>> 	struct kvm_cpuid_entry2 *cpuid_0x1;
>> 	unsigned long old_cr0 = kvm_read_cr0(vcpu);
>> +	DECLARE_BITMAP(reset_mask, 64);
>> 	unsigned long new_cr0;
>> +	unsigned int i;
>>
>> 	/*
>> 	 * Several of the "set" flows, e.g. ->set_cr0(), read other registers
>> @@ -12274,7 +12290,12 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>> 	kvm_async_pf_hash_reset(vcpu);
>> 	vcpu->arch.apf.halted = false;
>>
>> -	if (vcpu->arch.guest_fpu.fpstate && kvm_mpx_supported()) {
>> +	bitmap_from_u64(reset_mask, (kvm_caps.supported_xcr0 |
>> +				     kvm_caps.supported_xss) &
>> +				    XSTATE_NEED_RESET_MASK);
>> +
>> +	if (vcpu->arch.guest_fpu.fpstate &&
>> +	    !bitmap_empty(reset_mask, XFEATURE_MAX)) {
>> 		struct fpstate *fpstate = vcpu->arch.guest_fpu.fpstate;
>>
>> 		/*
>> @@ -12284,8 +12305,11 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>> 		if (init_event)
>> 			kvm_put_guest_fpu(vcpu);
>>
>> -		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDREGS);
>> -		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDCSR);
>> +		for_each_set_bit(i, reset_mask, XFEATURE_MAX) {
>> +			if (!kvm_vcpu_has_xstate(i))
>> +				continue;
> The kvm_vcpu_has_xstate() check is superfluous because @i is derived from
> kvm_caps.supported_xcr0/xss, which already guarantees that all unsupported
> xfeatures are filtered out.

Yeah, at least currently I can skip the check for CET/MPX feaures, will remove it, thanks!

>
> I recommend dropping this check. w/ this change,
>
> Reviewed-by: Chao Gao <chao.gao@intel.com>
>
>> +			fpstate_clear_xstate_component(fpstate, i);
>> +		}
>>
>> 		if (init_event)
>> 			kvm_load_guest_fpu(vcpu);
>> -- 
>> 2.43.0
>>
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 10847e1cc413..5a9c07751c0e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12217,11 +12217,27 @@  void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 		static_branch_dec(&kvm_has_noapic_vcpu);
 }
 
+#define XSTATE_NEED_RESET_MASK	(XFEATURE_MASK_BNDREGS | \
+				 XFEATURE_MASK_BNDCSR)
+
+static bool kvm_vcpu_has_xstate(unsigned long xfeature)
+{
+	switch (xfeature) {
+	case XFEATURE_MASK_BNDREGS:
+	case XFEATURE_MASK_BNDCSR:
+		return kvm_cpu_cap_has(X86_FEATURE_MPX);
+	default:
+		return false;
+	}
+}
+
 void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
 	struct kvm_cpuid_entry2 *cpuid_0x1;
 	unsigned long old_cr0 = kvm_read_cr0(vcpu);
+	DECLARE_BITMAP(reset_mask, 64);
 	unsigned long new_cr0;
+	unsigned int i;
 
 	/*
 	 * Several of the "set" flows, e.g. ->set_cr0(), read other registers
@@ -12274,7 +12290,12 @@  void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 	kvm_async_pf_hash_reset(vcpu);
 	vcpu->arch.apf.halted = false;
 
-	if (vcpu->arch.guest_fpu.fpstate && kvm_mpx_supported()) {
+	bitmap_from_u64(reset_mask, (kvm_caps.supported_xcr0 |
+				     kvm_caps.supported_xss) &
+				    XSTATE_NEED_RESET_MASK);
+
+	if (vcpu->arch.guest_fpu.fpstate &&
+	    !bitmap_empty(reset_mask, XFEATURE_MAX)) {
 		struct fpstate *fpstate = vcpu->arch.guest_fpu.fpstate;
 
 		/*
@@ -12284,8 +12305,11 @@  void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 		if (init_event)
 			kvm_put_guest_fpu(vcpu);
 
-		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDREGS);
-		fpstate_clear_xstate_component(fpstate, XFEATURE_BNDCSR);
+		for_each_set_bit(i, reset_mask, XFEATURE_MAX) {
+			if (!kvm_vcpu_has_xstate(i))
+				continue;
+			fpstate_clear_xstate_component(fpstate, i);
+		}
 
 		if (init_event)
 			kvm_load_guest_fpu(vcpu);