Re: [RFC/RFT PATCH] x86/efistub: Don't bother enabling SEV in the EFI stub

[Tom Lendacky]

On 4/14/25 14:45, Ard Biesheuvel wrote:
> Hi Tom,
> 
> Thanks for taking a look.
> 
> 
> On Mon, 14 Apr 2025 at 21:10, Tom Lendacky <thomas.lendacky@xxxxxxx> wrote:
>>
>> On 4/14/25 08:04, Ard Biesheuvel wrote:
>>> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
>>>
>>> One of the last things the EFI stub does before handing over to the core
>>> kernel when booting as a SEV guest is enabling SEV, even though this is
>>> mostly redundant: one of the first things the core kernel does is
>>> calling sme_enable(), after setting up the early GDT and IDT but before
>>> even setting up the kernel page tables.
>>>
>>> So let's just drop this call to sev_enable(), and rely on the core
>>> kernel to initiaize SEV correctly.
>>
>> SEV support does some things in sev_enable() that aren't done later in
>> the kernel proper. For example, we check certain hypervisor features and
>> save the CC blob into the boot parameters.
> 
> I misread the last part: the kernel proper checks struct boot_params
> and setup_data, whereas the decompressor checks both of those as well
> as the EFI config table.
> 
> It would make sense for the EFI stub to populate the field in struct
> boot_params directly.
> 
>> And as I look closer, I see
>> that we don't call initialize_identity_maps() from the EFI stub so we
>> aren't calling snp_check_features() from the EFI stub, which we should
>> have been doing.
>>
> 
> The EFI stub has its own check for this, based on the same underlying
> logic (have_unsupported_snp_features()). But it checks this much
> earlier so it can fail gracefully and return to the firmware, rather
> than terminate.

Ah, whew! I forgot we have a slightly different call for that in the EFI
stub, thanks for reminding me.

Thanks,
Tom

> 
>> Just removing the call does cause an SNP boot to crash. My testing shows
>> just ensuring that the cc_blob_address in the boot_params structure is
>> set results in a successful boot under SNP. So some of sev_enable()
>> needs to remain and some things need to be moved into the kernel proper.
>>
> 
> Thanks, this is very helpful. I'll dig a bit deeper and try to address
> the points you raised.