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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

+ CC Julien Grall

> On 24 Jun 2022, at 19:17, Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> 
> wrote:
> 
> The EFI System Resource Table (ESRT) is necessary for fwupd to identify
> firmware updates to install.  According to the UEFI specification §23.4,
> the ESRT shall be stored in memory of type EfiBootServicesData.  However,
> memory of type EfiBootServicesData is considered general-purpose memory
> by Xen, so the ESRT needs to be moved somewhere where Xen will not
> overwrite it.  Copy the ESRT to memory of type EfiRuntimeServicesData,
> which Xen will not reuse.  dom0 can use the ESRT if (and only if) it is
> in memory of type EfiRuntimeServicesData.
> 
> Earlier versions of this patch reserved the memory in which the ESRT was
> located.  This created awkward alignment problems, and required either
> splitting the E820 table or wasting memory.  It also would have required
> a new platform op for dom0 to use to indicate if the ESRT is reserved.
> By copying the ESRT into EfiRuntimeServicesData memory, the E820 table
> does not need to be modified, and dom0 can just check the type of the
> memory region containing the ESRT.  The copy is only done if the ESRT is
> not already in EfiRuntimeServicesData memory, avoiding memory leaks on
> repeated kexec.
> 
> See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/
> for details.
> 
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> xen/common/efi/boot.c | 134 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 134 insertions(+)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index a25e1d29f1..f6f34aa816 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -39,6 +39,26 @@
>   { 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 EFI_SYSTEM_RESOURCE_TABLE_GUID    \
> +  { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 
> 0x80} }
> +#define EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION 1
> +
> +typedef struct {
> +    EFI_GUID FwClass;
> +    UINT32 FwType;
> +    UINT32 FwVersion;
> +    UINT32 LowestSupportedFwVersion;
> +    UINT32 CapsuleFlags;
> +    UINT32 LastAttemptVersion;
> +    UINT32 LastAttemptStatus;
> +} EFI_SYSTEM_RESOURCE_ENTRY;
> +
> +typedef struct {
> +    UINT32 FwResourceCount;
> +    UINT32 FwResourceCountMax;
> +    UINT64 FwResourceVersion;
> +    EFI_SYSTEM_RESOURCE_ENTRY Entries[];
> +} EFI_SYSTEM_RESOURCE_TABLE;
> 
> typedef EFI_STATUS
> (/* _not_ EFIAPI */ *EFI_SHIM_LOCK_VERIFY) (
> @@ -567,6 +587,41 @@ static int __init efi_check_dt_boot(const 
> EFI_LOADED_IMAGE *loaded_image)
> }
> #endif
> 
> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
> +
> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
> +{
> +    size_t available_len, len;
> +    const UINTN physical_start = desc->PhysicalStart;

Hi,

From my tests on an arm64 machine so far I can tell that desc is NULL here,
for this reason we have the problem.

I’ll have a further look on the cause of this, but if you are faster than me 
you are
welcome to give your opinion on that.

Cheers,
Luca

> +    const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
> +
> +    len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> +    if ( esrt == EFI_INVALID_TABLE_ADDR )
> +        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) ||
> +         !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]);
> +}
> +
> /*
>  * Include architecture specific implementation here, which references the
>  * static globals defined above.
> @@ -845,6 +900,8 @@ static UINTN __init 
> efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
>     return gop_mode;
> }
> 
> +static EFI_GUID __initdata esrt_guid = EFI_SYSTEM_RESOURCE_TABLE_GUID;
> +
> static void __init efi_tables(void)
> {
>     unsigned int i;
> @@ -868,6 +925,8 @@ static void __init efi_tables(void)
>             efi.smbios = (unsigned long)efi_ct[i].VendorTable;
>         if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) )
>             efi.smbios3 = (unsigned long)efi_ct[i].VendorTable;
> +        if ( match_guid(&esrt_guid, &efi_ct[i].VendorGuid) )
> +            esrt = (UINTN)efi_ct[i].VendorTable;
>     }
> 
> #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
> @@ -1051,6 +1110,70 @@ 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, mdesc_size;
> +    void *memory_map = NULL;
> +    UINT32 ver;
> +    unsigned int i;
> +
> +    for ( ; ; ) {
> +        status = efi_bs->GetMemoryMap(&info_size, memory_map, &map_key,
> +                                      &mdesc_size, &ver);
> +        if ( status == EFI_SUCCESS && memory_map != NULL )
> +            break;
> +        if ( status == EFI_BUFFER_TOO_SMALL || memory_map == NULL )
> +        {
> +            info_size += 8 * efi_mdesc_size;
> +            if ( memory_map != NULL )
> +                efi_bs->FreePool(memory_map);
> +            memory_map = NULL;
> +            status = efi_bs->AllocatePool(EfiLoaderData, info_size, 
> &memory_map);
> +            if ( status == EFI_SUCCESS )
> +                continue;
> +            PrintErr(L"Cannot allocate memory to relocate ESRT\r\n");
> +        }
> +        else
> +            PrintErr(L"Cannot obtain memory map to relocate ESRT\r\n");
> +        return;
> +    }
> +
> +    /* 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 )
> +            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 )
> +            {
> +                PrintErr(L"Cannot install new ESRT\r\n");
> +                efi_bs->FreePool(new_esrt);
> +            }
> +        }
> +        else
> +            PrintErr(L"Cannot allocate memory for ESRT\r\n");
> +        break;
> +    }
> +
> +    efi_bs->FreePool(memory_map);
> +}
> +
> static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
> {
>     EFI_STATUS status;
> @@ -1413,6 +1536,8 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE 
> *SystemTable)
>     if ( gop )
>         efi_set_gop_mode(gop, gop_mode);
> 
> +    efi_relocate_esrt(SystemTable);
> +
>     efi_exit_boot(ImageHandle, SystemTable);
> 
>     efi_arch_post_exit_boot(); /* Doesn't return. */
> @@ -1753,3 +1878,12 @@ void __init efi_init_memory(void)
>     unmap_domain_page(efi_l4t);
> }
> #endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> -- 
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
>