Re: [PATCH v7 01/22] x86/sev: Separate MSR and GHCB based snp_cpuid() via a callback

[Tom Lendacky <thomas.lendacky@amd.com>]

On 8/28/25 11:14, Ard Biesheuvel wrote:
> On Thu, 28 Aug 2025 at 17:33, Borislav Petkov <bp@alien8.de> wrote:
>>
>> + Joerg and Mike to doublecheck me.
>>
>> On Thu, Aug 28, 2025 at 12:22:04PM +0200, Ard Biesheuvel wrote:
>>> @@ -648,7 +611,7 @@ void __head do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
>>>       leaf.fn = fn;
>>>       leaf.subfn = subfn;
>>>
>>> -     ret = snp_cpuid(NULL, NULL, &leaf);
>>> +     ret = snp_cpuid(snp_cpuid_hv_msr, NULL, &leaf);
>>>       if (!ret)
>>>               goto cpuid_done;
>>>
>>
>> So this code becomes now:
>>
>> ---
>>         ret = snp_cpuid(snp_cpuid_hv_msr, NULL, &leaf);
>>         if (!ret)
>>                 goto cpuid_done;
>>
>> <--- tries to find the CPUID leaf in the CPUID table
>> <--- otherwise uses the MSR protocol to read CPUID from HV and massage it
>>
>>         if (ret != -EOPNOTSUPP)
>>                 goto fail;
>>
>>         if (__sev_cpuid_hv_msr(&leaf))
>>                 goto fail;
>>
>> <--- and now it tries to do the same - do CPUID over MSR protocol.
>>
>> This flow made sense before your change because it'll try to use the GHCB
>> protocol but you're zapping that now so, IOW, you can zap that second call
>> too:
>>
>>
>> diff --git a/arch/x86/boot/startup/sev-shared.c b/arch/x86/boot/startup/sev-shared.c
>> index ed88dfe7605e..fbfdfe0dce70 100644
>> --- a/arch/x86/boot/startup/sev-shared.c
>> +++ b/arch/x86/boot/startup/sev-shared.c
>> @@ -612,16 +612,9 @@ void __head do_vc_no_ghcb(struct pt_regs *regs, unsigned long exit_code)
>>         leaf.subfn = subfn;
>>
>>         ret = snp_cpuid(snp_cpuid_hv_msr, NULL, &leaf);
>> -       if (!ret)
>> -               goto cpuid_done;
>> -
>> -       if (ret != -EOPNOTSUPP)
>> -               goto fail;
>> -
>> -       if (__sev_cpuid_hv_msr(&leaf))
>> +       if (ret && ret != -EOPNOTSUPP)
>>                 goto fail;
>>
>> -cpuid_done:
>>         regs->ax = leaf.eax;
>>         regs->bx = leaf.ebx;
>>         regs->cx = leaf.ecx;
>>
> 
> This seems plausible but I'm not sure I understand 100% why this
> fallback logic was introduced in the first place, so I'll defer to the
> experts here.

The fallback logic is for supporting SNP guests (which will have a CPUID
table) and SEV-ES guest (which won't have a CPUID table).

Thanks,
Tom