Re: [PATCHv10 04/11] efi/x86: Implement support for unaccepted memory

[Ard Biesheuvel]

On Tue, 9 May 2023 at 02:56, Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
>
> On Tue, May 09, 2023 at 12:11:41AM +0200, Ard Biesheuvel wrote:
> > > @@ -1324,13 +1325,15 @@ void __init e820__memblock_setup(void)
> > >          * e820_table is not finalized and e820__end_of_ram_pfn() cannot be
> > >          * used to get correct RAM size.
> > >          */
> > > -       if (boot_params.unaccepted_memory) {
> > > +       if (efi.unaccepted != EFI_INVALID_TABLE_ADDR) {
> > > +               struct efi_unaccepted_memory *unaccepted;
> > >                 unsigned long size;
> > >
> > > -               /* One bit per 2MB */
> > > -               size = DIV_ROUND_UP(e820__end_of_ram_pfn() * PAGE_SIZE,
> > > -                                   PMD_SIZE * BITS_PER_BYTE);
> > > -               memblock_reserve(boot_params.unaccepted_memory, size);
> > > +               unaccepted = __va(efi.unaccepted);
> > > +
> > > +               size = sizeof(struct efi_unaccepted_memory);
> > > +               size += unaccepted->size;
> > > +               memblock_reserve(efi.unaccepted, size);
> > >         }
> > >
> >
> > This could be moved to generic code (but we'll need to use early_memremap())
>
> I don't understand why early_memremap() is needed. EFI_LOADER_DATA already
> mapped into direct mapping. We only need to reserve the memory so it
> could not be reallocated for other things. Hm?
>

*If* we move this to generic code, we have to ensure that we don't
rely on x86 specific semantics. When parsing the EFI configuration
tables, other architectures don't have a complete direct map yet, as
they receive the memory description from EFI not from a translated
E820 map.

Note that this is only for getting the size of the reservation. Later
on, when we actually consume the contents of the bitmap, generic or
non-x86 code will need to use the ordinary memremap() API to map this
memory, and this amounts to a __va() call when the memory is already
mapped. But I am not suggesting changing that part for this series.
And even the hunk above can remain as you suggest - we can revisit it
once other architectures gain support for this.

The main thing I would like to avoid at this point in time is to add
new fields to struct bootparams that loaders such as GRUB may start to
populate as well - I don't think there is a very strong case for
pseudo-EFI boot [where GRUB calls ExitBootServices()] on confidential
VMs (as it prevents the EFI stub and the kernel from accessing the
measurement and attestation APIs), but let's not create more struct
bootparams based API if we can avoid it.