On 9/15/23 12:32, Sean Christopherson wrote:
On Fri, Sep 15, 2023, Tom Lendacky wrote:
On 9/14/23 15:48, Tom Lendacky wrote:
On 9/14/23 15:28, Sean Christopherson wrote:
On Thu, Sep 14, 2023, Tom Lendacky wrote:
+ if (guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
+ svm_clr_intercept(svm, INTERCEPT_RDTSCP);
Same thing here.
Will do.
For RDTSCP, svm_recalc_instruction_intercepts() will set/clear the RDTSCP
intercept as part of the svm_vcpu_set_after_cpuid() path, but it will only
do it based on kvm_cpu_cap_has(X86_FEATURE_RDTSCP) being true, which is very
likely.
Do you think that is good enough and we can drop the setting and clearing of
the RDTSCP intercept in the sev_es_vcpu_set_after_cpuid() function and only
deal with the TSC_AUX MSR intercept?
The common handling should be good enough.
On a side note, it looks like RDTSCP would not be intercepted if the KVM cap
X86_FEATURE_RDTSCP feature is cleared, however unlikely, in
kvm_set_cpu_caps() and RDTSCP is not advertised to the guest (assuming the
guest is ignoring the RDTSCP CPUID bit).
Hmm, yes, though the only scenario in which KVM clears RDTSCP on AMD comes with
a WARN (it's a guard against KVM bugs). If the guest ignores CPUID and uses
RDTSCP anyways, the guest deserves its death, and leaking the host pCPU doesn't
seem like a major issue.
That said, if hardware behavior is to ignore unknown intercepts, e.g. if KVM can
safely set INTERCEPT_RDTSCP even when hardware doesn't support said intercept,
then I wouldn't be opposed to doing:
/*
* Intercept INVPCID if shadow paging is enabled to sync/free shadow
* roots, or if INVPCID is disabled in the guest to inject #UD.
*/
if (!kvm_cpu_cap_has(X86_FEATURE_INVPCID) ||
!npt_enabled || !guest_cpuid_has(&svm->vcpu, X86_FEATURE_INVPCID))
svm_set_intercept(svm, INTERCEPT_INVPCID);
else
svm_clr_intercept(svm, INTERCEPT_INVPCID);
if (kvm_cpu_cap_has(X86_FEATURE_RDTSCP) &&
guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
svm_clr_intercept(svm, INTERCEPT_RDTSCP);
else
svm_set_intercept(svm, INTERCEPT_RDTSCP);
Alternatively, KVM could check boot_cpu_has() instead or kvm_cpu_cap_has(), but
that's not foolproof either, e.g. see Intel's of hiding PCID to workaround the
TLB flushing bug on Alderlake. So my vote would either be to keep things as-is,
or do the above (if that's safe).
Keep things as-is works for me :)
Thanks,
Tom
