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

From: Chao Gao <chao.gao@intel.com>
To: Yang Weijiang <weijiang.yang@intel.com>
Cc: <seanjc@google.com>, <pbonzini@redhat.com>,
	<dave.hansen@intel.com>, <x86@kernel.org>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <peterz@infradead.org>,
	<rick.p.edgecombe@intel.com>, <mlevitsk@redhat.com>,
	<john.allen@amd.com>
Subject: Re: [PATCH v10 10/27] KVM: x86: Refine xsave-managed guest register/MSR reset handling
Date: Tue, 20 Feb 2024 11:04:27 +0800	[thread overview]
Message-ID: <ZdQWu3D3Jku1iAvd@chao-email> (raw)
In-Reply-To: <20240219074733.122080-11-weijiang.yang@intel.com>

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
>