Message-ID: <50016f7d-ccfb-6986-470e-42f86c58c6d6@amd.com>
Date:   Mon, 24 Oct 2022 11:31:42 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     John Allen <John.Allen@....com>,
        Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, weijiang.yang@...el.com,
        rick.p.edgecombe@...el.com, x86@...nel.org
Subject: Re: [RFC PATCH] x86/sev-es: Include XSS value in GHCB CPUID request

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

Powered by blists - more mailing lists