Messages in this thread | | | Date | Mon, 24 Oct 2022 11:31:42 -0500 | Subject | Re: [RFC PATCH] x86/sev-es: Include XSS value in GHCB CPUID request | From | Tom Lendacky <> |
| |
On 10/24/22 11:23, John Allen wrote: > On 10/14/2022 3:27 PM, Sean Christopherson wrote: >> On Wed, Oct 12, 2022, John Allen wrote: >>> When a guest issues a cpuid instruction for Fn0000000D_x0B >>> (CetUserOffset), KVM will intercept and need to access the guest >> >> s/KVM will/the hypervisor may >> >>> XSS value. >> >> Heh, "need" is debatable. >> >>> For SEV-ES, this is encrypted and needs to be >>> included in the GHCB to be visible to the hypervisor. The rdmsr >>> instruction needs to be called directly as the code may be used in early >>> boot in which case the rdmsr wrappers should be avoided as they are >>> incompatible with the decompression boot phase. >>> >>> Signed-off-by: John Allen <john.allen@amd.com> >>> --- >>> This patch is logically part of the SVM guest shadow stack support >>> series seen >>> here: >>> https://lore.kernel.org/all/20221012203910.204793-1-john.allen@amd.com/ >>> >>> Sending this patch separately from the main series as it should apply >>> to the >>> tip tree as opposed to the kvm tree as this patch is related to guest >>> kernel >>> support. >>> --- >>> arch/x86/kernel/sev-shared.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/arch/x86/kernel/sev-shared.c b/arch/x86/kernel/sev-shared.c >>> index 3a5b0c9c4fcc..34469fac03f0 100644 >>> --- a/arch/x86/kernel/sev-shared.c >>> +++ b/arch/x86/kernel/sev-shared.c >>> @@ -887,6 +887,21 @@ static enum es_result vc_handle_cpuid(struct ghcb >>> *ghcb, >>> /* xgetbv will cause #GP - use reset value for xcr0 */ >>> ghcb_set_xcr0(ghcb, 1); >>> + if (has_cpuflag(X86_FEATURE_SHSTK) && regs->ax == 0xd) { >> >> IIRC, XCR0 and XSS are only needed for sub-leafs 0 and 1, i.e. this and >> the code >> above don't need to expose XCR0/XSS to the host for ECX > 1. >> >> FWIW, I think it's ridiculous that the guest willingly exposes state to >> the host, >> it's not _that_ difficult to do the math in the guest. > > That makes sense to me. I think given that the XSS code here is tied in > with the SVM shadow stack patches, I'll submit a separate patch to first > address only exposing XCR0 for sub-leafs 0 and 1. Then I'll address XSS in > the next version of the SVM shadow stack patches. > >> >>> + unsigned long lo, hi; >>> + u64 xss; >>> + >>> + /* >>> + * Since vc_handle_cpuid may be used during early boot, the >>> + * rdmsr wrappers are incompatible and should not be used. >>> + * Invoke the instruction directly. >>> + */ >>> + asm volatile("rdmsr" : "=a" (lo), "=d" (hi) >>> + : "c" (MSR_IA32_XSS)); >> >> Doesn't __rdmsr() do what you want? But even that seems unnecessary, >> isn't the >> current XSS available in xfeatures_mask_supervisor()? > > Yes, I think you're right. That should make this change a lot more palatable.
That may depend on how early this CPUID leaf is requested. If requested before xfeatures_mask_supervisor is initialized, then it can't be used, yet. Maybe there is something that can be checked to see if xfeatures_mask_supervisor has been setup and then use it, otherwise read the MSR.
Thanks, Tom
> > Thanks, > John > >> >>> + xss = (hi << 32) | lo; >>> + ghcb_set_xss(ghcb, xss); >>> + } >>> + >>> ret = sev_es_ghcb_hv_call(ghcb, ctxt, SVM_EXIT_CPUID, 0, 0); >>> if (ret != ES_OK) >>> return ret; >>> -- >>> 2.34.3 >>> >
| |