Re: [PATCH v6] Preserve the EFI System Resource Table for dom0

[Jan Beulich <jbeulich@suse.com>]

On 18.05.2022 19:32, Demi Marie Obenour wrote:
> @@ -567,6 +587,39 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
>  }
>  #endif
>  
> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;

Just out of curiosity: It's an arbitrary choice to use this initializer,
i.e. no initializer (and hence zero) would do as well (with ...

> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
> +{
> +    size_t available_len, len;
> +    const UINTN physical_start = desc->PhysicalStart;
> +    const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
> +
> +    len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> +    if ( esrt == EFI_INVALID_TABLE_ADDR )

... an adjustment here, of course)?

> +        return 0;
> +    if ( physical_start > esrt || esrt - physical_start >= len )
> +        return 0;
> +    /*
> +     * The specification requires EfiBootServicesData, but accept
> +     * EfiRuntimeServicesData, which is a more logical choice.
> +     */
> +    if ( (desc->Type != EfiRuntimeServicesData) &&
> +         (desc->Type != EfiBootServicesData) )
> +        return 0;
> +    available_len = len - (esrt - physical_start);
> +    if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
> +        return 0;
> +    available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
> +    esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
> +    if ( esrt_ptr->FwResourceVersion != EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION ||

Nit (style): Overlong line.

> +         !esrt_ptr->FwResourceCount )
> +        return 0;
> +    if ( esrt_ptr->FwResourceCount > available_len / sizeof(esrt_ptr->Entries[0]) )
> +        return 0;
> +    return esrt_ptr->FwResourceCount * sizeof(esrt_ptr->Entries[0]);
> +}

Nit (style again): We generally put a blank line ahead of a function's
main return statement.

> @@ -1067,6 +1122,46 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>      if ( !efi_memmap )
>          blexit(L"Unable to allocate memory for EFI memory map");
>  
> +    efi_memmap_size = info_size;

I don't think this global needs setting here, yet? The local will
do just fine here, likely yielding smaller code. But I realize that's
connected to how you did your change vs what I was expecting you to
do (see below).

> +    status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
> +                                                     efi_memmap, &map_key,
> +                                                     &efi_mdesc_size,
> +                                                     &mdesc_ver);
> +    if ( EFI_ERROR(status) )
> +        PrintErrMesg(L"Cannot obtain memory map", status);
> +
> +    /* Try to obtain the ESRT.  Errors are not fatal. */
> +    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> +    {
> +        /*
> +         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
> +         * so that the memory it is in will not be used for other purposes.
> +         */
> +        void *new_esrt = NULL;
> +        size_t esrt_size = get_esrt_size(efi_memmap + i);
> +
> +        if ( !esrt_size )
> +            continue;
> +        if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type ==
> +             EfiRuntimeServicesData )
> +            break; /* ESRT already safe from reuse */
> +        status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
> +                                      &new_esrt);

I should have re-raised the earlier voiced concern when reviewing v5 (or
maybe already v4), and I'm sorry for not having paid close enough
attention: This may add up to two more entries in the memory map (if an
entry is split and its middle part is used; of course with an unusual
implementation there could be even more of a growth). Yet below your
addition, before obtaining the final memory map, you don't re- obtain
(and re-increase) the size needed. As to (re-)increase: In fact, prior
to the allocation you do there shouldn't be a need to bump the space by
8 extra entries. That's a safety measure only for possible allocations
happening across ExitBootServices().

And yes, in earlier versions you had

-    info_size += 8 * efi_mdesc_size;
+    info_size += 8 * (efi_mdesc_size + 1);

there, but that's not what would be needed anyway (if trying to avoid
a 2nd pass of establishing the needed size). Instead in such an event
you need to bump 8 to 10 (or at least 9, when assuming that normally it
wouldn't be the middle part of a new range which would be used, but
rather the leading or trailing one).

While I'd be okay with addressing the two nits above while committing,
this allocation size aspect first wants settling on. Personally I'd
prefer the more involved solution, but I'd be okay with merely
bumping the 8 (plus the addition of a suitable comment, explaining
the now multiple [two] constituent parts of a seemingly arbitrary
number). If you want to go this easier route, I guess I could also
make that adjustment while committing (and adding my R-b).

Jan