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

Tom Lendacky
2025-09-09T21:45:32Z

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