On 25/02/25 07:56, Xiaoyao Li wrote: > On 2/24/2025 7:38 PM, Adrian Hunter wrote: >> On 20/02/25 12:50, Xiaoyao Li wrote: >>> On 1/29/2025 5:58 PM, Adrian Hunter wrote: >>>> From: Sean Christopherson <seanjc@xxxxxxxxxx> >>>> >>>> Allow the use of kvm_load_host_xsave_state() with >>>> vcpu->arch.guest_state_protected == true. This will allow TDX to reuse >>>> kvm_load_host_xsave_state() instead of creating its own version. >>>> >>>> For consistency, amend kvm_load_guest_xsave_state() also. >>>> >>>> Ensure that guest state that kvm_load_host_xsave_state() depends upon, >>>> such as MSR_IA32_XSS, cannot be changed by user space, if >>>> guest_state_protected. >>>> >>>> [Adrian: wrote commit message] >>>> >>>> Link: https://lore.kernel.org/r/Z2GiQS_RmYeHU09L@xxxxxxxxxx >>>> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx> >>>> --- >>>> TD vcpu enter/exit v2: >>>> - New patch >>>> --- >>>> arch/x86/kvm/svm/svm.c | 7 +++++-- >>>> arch/x86/kvm/x86.c | 18 +++++++++++------- >>>> 2 files changed, 16 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c >>>> index 7640a84e554a..b4bcfe15ad5e 100644 >>>> --- a/arch/x86/kvm/svm/svm.c >>>> +++ b/arch/x86/kvm/svm/svm.c >>>> @@ -4253,7 +4253,9 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu, >>>> svm_set_dr6(svm, DR6_ACTIVE_LOW); >>>> clgi(); >>>> - kvm_load_guest_xsave_state(vcpu); >>>> + >>>> + if (!vcpu->arch.guest_state_protected) >>>> + kvm_load_guest_xsave_state(vcpu); >>>> kvm_wait_lapic_expire(vcpu); >>>> @@ -4282,7 +4284,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu, >>>> if (unlikely(svm->vmcb->control.exit_code == SVM_EXIT_NMI)) >>>> kvm_before_interrupt(vcpu, KVM_HANDLING_NMI); >>>> - kvm_load_host_xsave_state(vcpu); >>>> + if (!vcpu->arch.guest_state_protected) >>>> + kvm_load_host_xsave_state(vcpu); >>>> stgi(); >>>> /* Any pending NMI will happen here */ >>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>> index bbb6b7f40b3a..5cf9f023fd4b 100644 >>>> --- a/arch/x86/kvm/x86.c >>>> +++ b/arch/x86/kvm/x86.c >>>> @@ -1169,11 +1169,9 @@ EXPORT_SYMBOL_GPL(kvm_lmsw); >>>> void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu) >>>> { >>>> - if (vcpu->arch.guest_state_protected) >>>> - return; >>>> + WARN_ON_ONCE(vcpu->arch.guest_state_protected); >>>> if (kvm_is_cr4_bit_set(vcpu, X86_CR4_OSXSAVE)) { >>>> - >>>> if (vcpu->arch.xcr0 != kvm_host.xcr0) >>>> xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0); >>>> @@ -1192,13 +1190,11 @@ EXPORT_SYMBOL_GPL(kvm_load_guest_xsave_state); >>>> void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu) >>>> { >>>> - if (vcpu->arch.guest_state_protected) >>>> - return; >>>> - >>>> if (cpu_feature_enabled(X86_FEATURE_PKU) && >>>> ((vcpu->arch.xcr0 & XFEATURE_MASK_PKRU) || >>>> kvm_is_cr4_bit_set(vcpu, X86_CR4_PKE))) { >>>> - vcpu->arch.pkru = rdpkru(); >>>> + if (!vcpu->arch.guest_state_protected) >>>> + vcpu->arch.pkru = rdpkru(); >>> >>> this needs justification. >> >> It was proposed by Sean here: >> >> https://lore.kernel.org/all/Z2WZ091z8GmGjSbC@xxxxxxxxxx/ >> >> which is part of the email thread referenced by the "Link:" tag above > > IMHO, this change needs to be put in patch 07, which is the better place to justify it. > >>> >>>> if (vcpu->arch.pkru != vcpu->arch.host_pkru) >>>> wrpkru(vcpu->arch.host_pkru); >>>> } >>> >>> >>>> @@ -3916,6 +3912,10 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >>>> if (!msr_info->host_initiated && >>>> !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES)) >>>> return 1; >>>> + >>>> + if (vcpu->arch.guest_state_protected) >>>> + return 1; >>>> + >>> >>> this and below change need to be a separate patch. So that we can discuss independently. >>> >>> I see no reason to make MSR_IA32_XSS special than other MSRs. When guest_state_protected, most of the MSRs that aren't emulated by KVM are inaccessible by KVM. >> >> Yes, TDX will block access to MSR_IA32_XSS anyway because >> tdx_has_emulated_msr() will return false for MSR_IA32_XSS. >> >> However kvm_load_host_xsave_state() is not TDX-specific code and it >> relies upon vcpu->arch.ia32_xss, so there is reason to block >> access to it when vcpu->arch.guest_state_protected is true. > > It is TDX specific logic that TDX requires vcpu->arch.ia32_xss unchanged since TDX is going to utilize kvm_load_host_xsave_state() to restore host xsave state and relies on vcpu->arch.ia32_xss to be always the value of XFAM & XSS_MASK. > > So please put this change into the TDX specific patch with the clear justfication. This patch set is owned by Paolo now, so it is up to him. >>> >>>> /* >>>> * KVM supports exposing PT to the guest, but does not support >>>> * IA32_XSS[bit 8]. Guests have to use RDMSR/WRMSR rather than >>>> @@ -4375,6 +4375,10 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >>>> if (!msr_info->host_initiated && >>>> !guest_cpuid_has(vcpu, X86_FEATURE_XSAVES)) >>>> return 1; >>>> + >>>> + if (vcpu->arch.guest_state_protected) >>>> + return 1; >>>> + >>>> msr_info->data = vcpu->arch.ia32_xss; >>>> break; >>>> case MSR_K7_CLK_CTL: >>> >> >
