[PATCH] Use EfiACPIReclaimMemory for ESRT

[PATCH] Use EfiACPIReclaimMemory for ESRT

[Demi Marie Obenour]

As discussed on xen-devel, using EfiRuntimeServicesData for more than is
absolutely necessary is a bad idea.
---
 xen/common/efi/boot.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index db0340c8e2628314226c618dda11ede4c62fdf3b..dba23439758d1e842d267dcd19448e0f9113b115 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -601,11 +601,13 @@ static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
     if ( physical_start > esrt || esrt - physical_start >= len )
         return 0;
     /*
-     * The specification requires EfiBootServicesData, but accept
-     * EfiRuntimeServicesData, which is a more logical choice.
+     * The specification requires EfiBootServicesData, but also accept
+     * EfiRuntimeServicesData (for compatibility) and EfiACPIReclaimMemory
+     * (which will contain the tables after successful kexec).
      */
     if ( (desc->Type != EfiRuntimeServicesData) &&
-         (desc->Type != EfiBootServicesData) )
+         (desc->Type != EfiBootServicesData) &&
+         (desc->Type != EfiACPIReclaimMemory) )
         return 0;
     available_len = len - (esrt - physical_start);
     if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
@@ -1144,18 +1146,19 @@ static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
     for ( i = 0; i < info_size; i += mdesc_size )
     {
         /*
-         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
+         * 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;
-        size_t esrt_size = get_esrt_size(memory_map + i);
+        const EFI_MEMORY_DESCRIPTOR *desc = memory_map + i;
+        size_t esrt_size = get_esrt_size(desc);
 
         if ( !esrt_size )
             continue;
-        if ( ((EFI_MEMORY_DESCRIPTOR *)(memory_map + i))->Type ==
-             EfiRuntimeServicesData )
+        if ( desc->Type == EfiRuntimeServicesData ||
+             desc->Type == EfiACPIReclaimMemory )
             break; /* ESRT already safe from reuse */
-        status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
+        status = efi_bs->AllocatePool(EfiACPIReclaimMemory, esrt_size,
                                       &new_esrt);
         if ( status == EFI_SUCCESS && new_esrt )
         {
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Re: [PATCH] Use EfiACPIReclaimMemory for ESRT

[Roger Pau Monné]

On Fri, Sep 30, 2022 at 05:02:02PM -0400, Demi Marie Obenour wrote:
> As discussed on xen-devel, using EfiRuntimeServicesData for more than is
> absolutely necessary is a bad idea.

I'm afraid this needs a proper commit message: commit messages need to
be able to stand alone on it's own in most cases, without references
to external sources.  IMO I would add a summary of the thread that
happened on xen-devel: scenario, issue and how it's fixed, and also
provide a link (from lists.xenproject.org) to the conversation thread.

It's also missing a SoB.

Thanks, Roger.

Re: [PATCH] Use EfiACPIReclaimMemory for ESRT

[Jan Beulich]

On 30.09.2022 23:02, Demi Marie Obenour wrote:
> As discussed on xen-devel, using EfiRuntimeServicesData for more than is
> absolutely necessary is a bad idea.
> ---
>  xen/common/efi/boot.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index db0340c8e2628314226c618dda11ede4c62fdf3b..dba23439758d1e842d267dcd19448e0f9113b115 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -601,11 +601,13 @@ static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
>      if ( physical_start > esrt || esrt - physical_start >= len )
>          return 0;
>      /*
> -     * The specification requires EfiBootServicesData, but accept
> -     * EfiRuntimeServicesData, which is a more logical choice.
> +     * The specification requires EfiBootServicesData, but also accept
> +     * EfiRuntimeServicesData (for compatibility) and EfiACPIReclaimMemory
> +     * (which will contain the tables after successful kexec).

What's the compatibility concern here? We haven't released any Xen
version yet where the table would be moved to EfiRuntimeServicesData.

Jan

>       */
>      if ( (desc->Type != EfiRuntimeServicesData) &&
> -         (desc->Type != EfiBootServicesData) )
> +         (desc->Type != EfiBootServicesData) &&
> +         (desc->Type != EfiACPIReclaimMemory) )
>          return 0;
>      available_len = len - (esrt - physical_start);
>      if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
> @@ -1144,18 +1146,19 @@ static void __init efi_relocate_esrt(EFI_SYSTEM_TABLE *SystemTable)
>      for ( i = 0; i < info_size; i += mdesc_size )
>      {
>          /*
> -         * ESRT needs to be moved to memory of type EfiRuntimeServicesData
> +         * 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;
> -        size_t esrt_size = get_esrt_size(memory_map + i);
> +        const EFI_MEMORY_DESCRIPTOR *desc = memory_map + i;
> +        size_t esrt_size = get_esrt_size(desc);
>  
>          if ( !esrt_size )
>              continue;
> -        if ( ((EFI_MEMORY_DESCRIPTOR *)(memory_map + i))->Type ==
> -             EfiRuntimeServicesData )
> +        if ( desc->Type == EfiRuntimeServicesData ||
> +             desc->Type == EfiACPIReclaimMemory )
>              break; /* ESRT already safe from reuse */
> -        status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
> +        status = efi_bs->AllocatePool(EfiACPIReclaimMemory, esrt_size,
>                                        &new_esrt);
>          if ( status == EFI_SUCCESS && new_esrt )
>          {

Re: [PATCH] Use EfiACPIReclaimMemory for ESRT

[Demi Marie Obenour]

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

On Tue, Oct 04, 2022 at 10:31:25AM +0200, Jan Beulich wrote:
> On 30.09.2022 23:02, Demi Marie Obenour wrote:
> > As discussed on xen-devel, using EfiRuntimeServicesData for more than is
> > absolutely necessary is a bad idea.
> > ---
> >  xen/common/efi/boot.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > index db0340c8e2628314226c618dda11ede4c62fdf3b..dba23439758d1e842d267dcd19448e0f9113b115 100644
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -601,11 +601,13 @@ static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
> >      if ( physical_start > esrt || esrt - physical_start >= len )
> >          return 0;
> >      /*
> > -     * The specification requires EfiBootServicesData, but accept
> > -     * EfiRuntimeServicesData, which is a more logical choice.
> > +     * The specification requires EfiBootServicesData, but also accept
> > +     * EfiRuntimeServicesData (for compatibility) and EfiACPIReclaimMemory
> > +     * (which will contain the tables after successful kexec).
> 
> What's the compatibility concern here? We haven't released any Xen
> version yet where the table would be moved to EfiRuntimeServicesData.

Old buggy firmware.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

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