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

Yang, Weijiang
2024-01-26T09:30:28Z

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
>>