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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.06.2022 03:23, Demi Marie Obenour wrote:
> @@ -1051,6 +1110,62 @@ static void __init 
> efi_set_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop, UINTN gop
>  #define INVALID_VIRTUAL_ADDRESS (0xBAAADUL << \
>                                   (EFI_PAGE_SHIFT + BITS_PER_LONG - 32))
>  
> +static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
> +{
> +    EFI_STATUS status;
> +    UINTN info_size = 0, map_key;
> +    unsigned int i;
> +    void *memory_map = NULL;
> +
> +    for (;;) {

Nit: Style:

    for ( ; ; )
    {

> +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
> +                                      &efi_mdesc_size, &mdesc_ver);

Unless you have a reason to (which I don't see), please don't alter
global variables here. 

> +        if ( status == EFI_SUCCESS && memory_map != NULL )
> +            break;
> +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL ) {

Nit: Brace placement.

> +            info_size *= 2;

Doubling the buffer size seems excessive to me. If you really want it
like that, I think this deserves saying a word in the description. As
said before, the same increment as in efi_exit_boot() would look best
to me, for consistency.

> +            if ( memory_map != NULL )
> +                efi_bs->FreePool(memory_map);
> +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, 
> &memory_map);
> +            if ( status == EFI_SUCCESS )
> +                continue;
> +        }
> +        return;

Perhaps emit a message?

> +    }
> +
> +    /* Try to obtain the ESRT.  Errors are not fatal. */
> +    for ( i = 0; i < info_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 )
> +            return; /* ESRT already safe from reuse */

"break" here so that ...

> +        status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
> +                                      &new_esrt);
> +        if ( status == EFI_SUCCESS && new_esrt )
> +        {
> +            memcpy(new_esrt, (void *)esrt, esrt_size);
> +            status = efi_bs->InstallConfigurationTable(&esrt_guid, new_esrt);
> +            if ( status != EFI_SUCCESS )
> +            {
> +                PrintErr(L"Cannot install new ESRT\r\n");
> +                efi_bs->FreePool(new_esrt);
> +            }
> +        }
> +        else
> +            PrintErr(L"Cannot allocate memory for ESRT\r\n");
> +        break;
> +    }

... you can free the memory map here.

> @@ -1067,6 +1182,13 @@ 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");
>  
> +    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);
> +
>      for ( retry = false; ; retry = true )
>      {
>          efi_memmap_size = info_size;

What is this change about? If it really is needed for some reason, I
further don't see why you don't use efi_bs (it cannot be used inside
the loop, but is fine to use ahead of it).

Jan