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

[Jan Beulich <jbeulich@suse.com>]

On 17.05.2022 19:07, Demi Marie Obenour wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -39,6 +39,25 @@
>    { 0x605dab50, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} }
>  #define APPLE_PROPERTIES_PROTOCOL_GUID \
>    { 0x91bd12fe, 0xf6c3, 0x44fb, { 0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0} }
> +#define ESRT_GUID    \
> +  { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }

I'm sorry, but it looks like my earlier comments still weren't clear
enough: The spec calls this EFI_SYSTEM_RESOURCE_TABLE_GUID, ...

> +typedef struct _ESRT_ENTRY {

... has no tag here, ...

> +    EFI_GUID FwClass;
> +    UINT32 FwType;
> +    UINT32 FwVersion;
> +    UINT32 FwLowestSupportedVersion;
> +    UINT32 FwCapsuleFlags;
> +    UINT32 FwLastAttemptVersion;
> +    UINT32 FwLastAttemptStatus;
> +} ESRT_ENTRY;

... calls this EFI_SYSTEM_RESOURCE_ENTRY, ...

> +typedef struct _ESRT {

... again has no tag here, and ...

> +    UINT32 Count;
> +    UINT32 Max;
> +    UINT64 Version;
> +    ESRT_ENTRY Entries[];
> +} ESRT;

... calls this EFI_SYSTEM_RESOURCE_TABLE. Also some of the field
names still aren't matching the spec.

> @@ -1067,6 +1120,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;
> +    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);
> +        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 )
> +            {
> +                PrintStr(L"Cannot install new ESRT\r\n");

Perhaps better PrintErr() here and ...

> +                efi_bs->FreePool(new_esrt);
> +            }
> +        }
> +        else
> +            PrintStr(L"Cannot allocate memory for ESRT\r\n");

... here?

Jan