[PATCH v2] efi: Relocate the ESRT when booting via multiboot2

[PATCH v2] efi: Relocate the ESRT when booting via multiboot2

[Demi Marie Obenour]

This was missed in the initial patchset.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 xen/arch/x86/efi/efi-boot.h |   2 +
 xen/common/efi/boot.c       | 136 ++++++++++++++++++------------------
 2 files changed, 70 insertions(+), 68 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index abfc7ab0f31511e2c1ee402a09ac533d260444b2..a9a2991d6462dec9cea695c8b912b72df26bd511 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -825,6 +825,8 @@ void __init efi_multiboot2(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, true);
 }
 
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 32ae6b43bb53448421c908819cda552757157c1f..ea5f010df1c5ce40fb67a91fdbfe28f40865252c 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -625,6 +625,74 @@ static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
     return esrt_ptr->FwResourceCount * sizeof(esrt_ptr->Entries[0]);
 }
 
+static EFI_GUID __initdata esrt_guid = EFI_SYSTEM_RESOURCE_TABLE_GUID;
+
+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 * 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 += mdesc_size )
+    {
+        /*
+         * ESRT needs to be moved to memory of type EfiACPIReclaimMemory
+         * so that the memory it is in will not be used for other purposes.
+         */
+        void *new_esrt = NULL;
+        const EFI_MEMORY_DESCRIPTOR *desc = memory_map + i;
+        size_t esrt_size = get_esrt_size(desc);
+
+        if ( !esrt_size )
+            continue;
+        if ( desc->Type == EfiRuntimeServicesData ||
+             desc->Type == EfiACPIReclaimMemory )
+            break; /* ESRT already safe from reuse */
+        status = efi_bs->AllocatePool(EfiACPIReclaimMemory, 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);
+}
+
 /*
  * Include architecture specific implementation here, which references the
  * static globals defined above.
@@ -903,8 +971,6 @@ 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;
@@ -1113,72 +1179,6 @@ 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 * 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 += mdesc_size )
-    {
-        /*
-         * ESRT needs to be moved to memory of type EfiACPIReclaimMemory
-         * so that the memory it is in will not be used for other purposes.
-         */
-        void *new_esrt = NULL;
-        const EFI_MEMORY_DESCRIPTOR *desc = memory_map + i;
-        size_t esrt_size = get_esrt_size(desc);
-
-        if ( !esrt_size )
-            continue;
-        if ( desc->Type == EfiRuntimeServicesData ||
-             desc->Type == EfiACPIReclaimMemory )
-            break; /* ESRT already safe from reuse */
-        status = efi_bs->AllocatePool(EfiACPIReclaimMemory, 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, bool exit_boot_services)
 {
     EFI_STATUS status;
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH v2] efi: Relocate the ESRT when booting via multiboot2

[Jan Beulich]

On 14.12.2022 00:03, Demi Marie Obenour wrote:
> This was missed in the initial patchset.
> 
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>

I'm sorry, but no: In a v2 submission you should address prior comments.
On v1 I did offer to extend the description while committing, but you
should not take this as an invitation to make the committer again put
more time into getting this into the tree than should actually be needed
(and reviewers to re-figure the unmentioned aspect).

You also will want to get used to adding brief revision log remarks
outside of the commit message area. It would be relevant to know here
there the sole difference from v1 is re-basing.

Jan

Re: [PATCH v2] efi: Relocate the ESRT when booting via multiboot2

[Jan Beulich]

On 14.12.2022 10:34, Jan Beulich wrote:
> On 14.12.2022 00:03, Demi Marie Obenour wrote:
>> This was missed in the initial patchset.
>>
>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> 
> I'm sorry, but no: In a v2 submission you should address prior comments.
> On v1 I did offer to extend the description while committing, but you
> should not take this as an invitation to make the committer again put
> more time into getting this into the tree than should actually be needed
> (and reviewers to re-figure the unmentioned aspect).

So I thought I'd be kind and do what I said I didn't really want to do,
but things are worse: The patch again doesn't apply, this time because
of assuming an apparently private patch of yours to also be in place.
As it's merely patch context which is a problem, I'll edit the patch,
but only to be done with it rather than wasting yet more time.

Jan