[PATCH] Relocate the ESRT when booting via multiboot2

[PATCH] 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>
---
I consider this to be a bug fix, and would prefer this to be included in
the 4.17 release series, even though I understand if this is too late
for 4.17 itself.

 xen/arch/x86/efi/efi-boot.h |   2 +
 xen/common/efi/boot.c       | 134 ++++++++++++++++++------------------
 2 files changed, 69 insertions(+), 67 deletions(-)

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 51967f6c3d5cd06b6b50bac06c1b8762cd0f7856..20a42a66dd2b93f3cd2be0230a372767a8763ed5 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -818,6 +818,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);
 }
 
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 7a97c64884bf55a012cfb8bd7ad56ddfc1e43e66..93939ca61d28717d29aaa0289262c55558dfe944 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -622,6 +622,73 @@ 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 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(memory_map + i);
+
+        if ( !esrt_size )
+            continue;
+        if ( ((EFI_MEMORY_DESCRIPTOR *)(memory_map + 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);
+}
+
 /*
  * Include architecture specific implementation here, which references the
  * static globals defined above.
@@ -900,8 +967,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;
@@ -1110,71 +1175,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 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(memory_map + i);
-
-        if ( !esrt_size )
-            continue;
-        if ( ((EFI_MEMORY_DESCRIPTOR *)(memory_map + 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;
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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

[Jan Beulich]

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

It looks as if, besides ...

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -818,6 +818,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);
>  }
>  

... this core piece, the rest is merely pure code movement, with the goal
of avoiding a forward declaration. If such is the case, please say so in
your description. I'll add a sentence to this effect when committing, so
the remark if for future patches. Provided of course it is true (i.e. I'm
not overlooking any tiny change) - please confirm.

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Demi Marie Obenour]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

On Mon, Dec 12, 2022 at 04:19:21PM +0100, Jan Beulich wrote:
> On 09.12.2022 01:52, Demi Marie Obenour wrote:
> > This was missed in the initial patchset.
> > 
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> 
> It looks as if, besides ...
> 
> > --- a/xen/arch/x86/efi/efi-boot.h
> > +++ b/xen/arch/x86/efi/efi-boot.h
> > @@ -818,6 +818,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);
> >  }
> >  
> 
> ... this core piece, the rest is merely pure code movement, with the goal
> of avoiding a forward declaration. If such is the case, please say so in
> your description. I'll add a sentence to this effect when committing, so
> the remark if for future patches. Provided of course it is true (i.e. I'm
> not overlooking any tiny change) - please confirm.

It is indeed just code movement.

> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks!  Do you want me to submit a separate patch for backporting to
4.17, or do you plan on doing that?
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jan Beulich]

On 12.12.2022 23:58, Demi Marie Obenour wrote:
> On Mon, Dec 12, 2022 at 04:19:21PM +0100, Jan Beulich wrote:
>> On 09.12.2022 01:52, Demi Marie Obenour wrote:
>>> This was missed in the initial patchset.
>>>
>>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
>>
>> It looks as if, besides ...
>>
>>> --- a/xen/arch/x86/efi/efi-boot.h
>>> +++ b/xen/arch/x86/efi/efi-boot.h
>>> @@ -818,6 +818,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);
>>>  }
>>>  
>>
>> ... this core piece, the rest is merely pure code movement, with the goal
>> of avoiding a forward declaration. If such is the case, please say so in
>> your description. I'll add a sentence to this effect when committing, so
>> the remark if for future patches. Provided of course it is true (i.e. I'm
>> not overlooking any tiny change) - please confirm.
> 
> It is indeed just code movement.
> 
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks!  Do you want me to submit a separate patch for backporting to
> 4.17, or do you plan on doing that?

There's no need for backport submissions except in extremely rare cases
where the backport turns out unusually cumbersome.

Jan

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

[Jan Beulich]

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

This doesn't apply to current staging, apparently because you didn't
put it on top of your own "Use EfiACPIReclaimMemory for ESRT".

Jan