Re: [PATCH v9 13/27] KVM: x86: Refresh CPUID on write to guest MSR_IA32_XSS

[Yang, Weijiang]

On 1/25/2024 6:57 PM, Chao Gao wrote:
> On Tue, Jan 23, 2024 at 06:41:46PM -0800, Yang Weijiang wrote:
> > Update CPUID.(EAX=0DH,ECX=1).EBX to reflect current required xstate size
> > due to XSS MSR modification.
> > CPUID(EAX=0DH,ECX=1).EBX reports the required storage size of all enabled
> > xstate features in (XCR0 | IA32_XSS). The CPUID value can be used by guest
> > before allocate sufficient xsave buffer.
> >
> > Note, KVM does not yet support any XSS based features, i.e. supported_xss
> > is guaranteed to be zero at this time.
> >
> > Opportunistically modify XSS write access logic as:
> > If XSAVES is not enabled in the guest CPUID, forbid setting IA32_XSS msr
> > to anything but 0, even if the write is host initiated.
> any reason to allow host to write 0? looks we are not doing this for many
> other MSRs.

Paolo mentioned this point for many times, and the latest one can be found at:
Re: [PATCH v5 04/19] KVM:x86: Refresh CPUID on write to guest MSR_IA32_XSS - Paolo Bonzini (kernel.org) <https://lore.kernel.org/kvm/CABgObfbvr8F8g5hJN6jn95m7u7m2+8ACkqO25KAZwRmJ9AncZg@xxxxxxxxxxxxxx/>

For other MSRs, Sean proposed to enforce the policy in batch, but the work is delayed.

> > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > Co-developed-by: Zhang Yi Z <yi.z.zhang@xxxxxxxxxxxxxxx>
> > Signed-off-by: Zhang Yi Z <yi.z.zhang@xxxxxxxxxxxxxxx>
> > Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> > Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> > ---
> > arch/x86/include/asm/kvm_host.h |  3 ++-
> > arch/x86/kvm/cpuid.c            | 15 ++++++++++++++-
> > arch/x86/kvm/x86.c              | 16 ++++++++++++----
> > 3 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 40dd796ea085..6efaaaa15945 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -772,7 +772,6 @@ struct kvm_vcpu_arch {
> > 	bool at_instruction_boundary;
> > 	bool tpr_access_reporting;
> > 	bool xfd_no_write_intercept;
> > -	u64 ia32_xss;
> > 	u64 microcode_version;
> > 	u64 arch_capabilities;
> > 	u64 perf_capabilities;
> > @@ -828,6 +827,8 @@ struct kvm_vcpu_arch {
> >
> > 	u64 xcr0;
> > 	u64 guest_supported_xcr0;
> > +	u64 guest_supported_xss;
> > +	u64 ia32_xss;
> >
> > 	struct kvm_pio_request pio;
> > 	void *pio_data;
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index acc360c76318..3ab133530573 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -275,7 +275,8 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
> > 	best = cpuid_entry2_find(entries, nent, 0xD, 1);
> > 	if (best && (cpuid_entry_has(best, X86_FEATURE_XSAVES) ||
> > 		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
> > -		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
> > +		best->ebx = xstate_required_size(vcpu->arch.xcr0 |
> > +						 vcpu->arch.ia32_xss, true);
> >
> > 	best = __kvm_find_kvm_cpuid_features(vcpu, entries, nent);
> > 	if (kvm_hlt_in_guest(vcpu->kvm) && best &&
> > @@ -312,6 +313,17 @@ static u64 vcpu_get_supported_xcr0(struct kvm_vcpu *vcpu)
> > 	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;
> > }
> >
> > +static u64 vcpu_get_supported_xss(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm_cpuid_entry2 *best;
> > +
> > +	best = kvm_find_cpuid_entry_index(vcpu, 0xd, 1);
> > +	if (!best)
> > +		return 0;
> > +
> > +	return (best->ecx | ((u64)best->edx << 32)) & kvm_caps.supported_xss;
> > +}
> > +
> > static bool kvm_cpuid_has_hyperv(struct kvm_cpuid_entry2 *entries, int nent)
> > {
> > #ifdef CONFIG_KVM_HYPERV
> > @@ -362,6 +374,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > 	}
> >
> > 	vcpu->arch.guest_supported_xcr0 = vcpu_get_supported_xcr0(vcpu);
> > +	vcpu->arch.guest_supported_xss = vcpu_get_supported_xss(vcpu);
> >
> > 	kvm_update_pv_runtime(vcpu);
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index b3a39886e418..7b7a15aab3aa 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3924,20 +3924,28 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > 			vcpu->arch.ia32_tsc_adjust_msr += adj;
> > 		}
> > 		break;
> > -	case MSR_IA32_XSS:
> > -		if (!msr_info->host_initiated &&
> > -		    !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES))
> > +	case MSR_IA32_XSS: {
> unnecessary bracket.

Yes, will remove it.

> > +		/*
> > +		 * If KVM reported support of XSS MSR, even guest CPUID doesn't
> IIUC, below code doesn't check if KVM reported support of XSS MSR. so, the comment
> doesn't match what the code does.

Here I refers to what it does in patch 12.

> > +		 * support XSAVES, still allow userspace to set default value(0)
> > +		 * to this MSR.
> > +		 */
> > +		if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVES) &&
> > +		    !(msr_info->host_initiated && data == 0))
> > 			return 1;
> > 		/*
> > 		 * KVM supports exposing PT to the guest, but does not support
> > 		 * IA32_XSS[bit 8]. Guests have to use RDMSR/WRMSR rather than
> > 		 * XSAVES/XRSTORS to save/restore PT MSRs.
> > 		 */
> > -		if (data & ~kvm_caps.supported_xss)
> > +		if (data & ~vcpu->arch.guest_supported_xss)
> > 			return 1;
> > +		if (vcpu->arch.ia32_xss == data)
> > +			break;
> > 		vcpu->arch.ia32_xss = data;
> > 		kvm_update_cpuid_runtime(vcpu);
> > 		break;
> > +	}
> > 	case MSR_SMI_COUNT:
> > 		if (!msr_info->host_initiated)
> > 			return 1;
> > --
> > 2.39.3