Re: [PATCH] efi/libstub: Do not accept parts of memory before ExitBootServices()

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

On 3/25/25 09:39, Ard Biesheuvel wrote:
> On Tue, 25 Mar 2025 at 14:44, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
>>
>> On Tue, Mar 25, 2025 at 02:09:54PM +0100, Ard Biesheuvel wrote:
>>>> Since the problem happens before ExitBootServices(), can we allocate this
>>>> memory range with EFI API and free it back?
>>>>
>>>
>>> In principle, yes - we could allocate these misaligned chunks as
>>> EfiLoaderData, and it wouldn't even be necessary to free them, as they
>>> would become available to the OS automatically.
>>>
>>> But doing this in setup_e820() is tricky, because every page
>>> allocation modifies the EFI memory map, and we may have to restart
>>> from the beginning. And there is no guarantee that some asynchronous
>>> event in the firmware context does not attempt to allocate some pages,
>>> in a way that might result in another misaligned unaccepted region.
>>
>> Looking again at the code, setup_e820() (and therefore
>> process_unaccepted_memory()) called after efi_exit_boot_services() in
>> exit_boot(), so we can't use EFI API to allocate memory.
>>
> 
> Ah yes, I misremembered that. It also means that it is fine in
> principle to take over the communication with the hypervisor.
> 
> However, this is still tricky, because on SEV-SNP, accepting memory
> appears to rely on the GHCB page based communication being enabled,
> and this involves mapping it down to a single page so the C bit can be
> cleared. It would be nice if we could simply use the MSR based
> protocol for accepting memory.

We can probably do something along this line since there is an existing
function, __page_state_change(), that performs MSR protocol PSC. If we
change the arch_accept_memory() calls in process_unaccepted_memory() to
arch_accept_memory_early() then we can differentiate between this early
alignment setup timeframe. The early function can also use
sev_get_status() instead of sev_snp_enabled().

Let me mess around with it a bit and see what I come up with.

Thanks,
Tom

> 
>> And it bring us back to the issue being platform-specific. It should be
>> able to accept memory in principle.
>>
> 
> Indeed.
> 
>> I remember testing TDX boot with ridiculously large unit_size, like 256M.
>> And accept logic worked fine for me.
>>
>>> So ideally, firmware would adopt the same granularity when accepting
>>> memory, and we wouldn't have this problem. (Or maybe this is why
>>> nobody noticed until I found it by inspection?)
>>
>> It would be nice, yes, but we need to deal with requirements in current
>> spec.
>>
> 
> Yeah :-(