Re: [RFC PATCH] x86/sev-es: Include XSS value in GHCB CPUID request

[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@xxxxxxx>
> > > ---
> > > 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@xxxxxxx/
> > >
> > > 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