+ CC Julien Grall
> On 24 Jun 2022, at 19:17, Demi Marie Obenour <[email protected]>
> 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 <[email protected]>
> ---
> 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
>
