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

["Yang, Weijiang" <weijiang.yang@intel.com>]

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@mail.gmail.com/>

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

>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Co-developed-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
>> Signed-off-by: Zhang Yi Z <yi.z.zhang@linux.intel.com>
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
>> ---
>> 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
>>