[v2] Use EfiACPIReclaimMemory for ESRT

[PATCH v2] Use EfiACPIReclaimMemory for ESRT

Posted by Demi Marie Obenour 1 year, 6 months ago

A previous patch tried to get Linux to use the ESRT under Xen if it is
in memory of type EfiRuntimeServicesData.  However, this turns out to be
a bad idea.  Ard Biesheuvel pointed out that EfiRuntimeServices* memory
winds up fragmenting both the EFI page tables and the direct map, and
that EfiACPIReclaimMemory is a much better choice for this purpose.

Link: https://lists.xenproject.org/archives/html/xen-devel/2022-09/msg01365.html
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
Changes since v1:

- Add link to Ard Biesheuvel’s post on xen-devel
- Expand comment to mention buggy firmware that place the ESRT in
  EfiBootServicesData.

 xen/common/efi/boot.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index db0340c8e2628314226c618dda11ede4c62fdf3b..b3de1011ee58a67a82a94da050eb1343f4e37faa 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -601,11 +601,14 @@ 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 with buggy firmware)
+     * 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 +1147,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 v3] Use EfiACPIReclaimMemory for ESRT

Posted by Jan Beulich 1 year, 4 months ago

On 07.12.2022 23:42, Demi Marie Obenour wrote:
> A previous patch tried to get Linux to use the ESRT under Xen if it is
> in memory of type EfiRuntimeServicesData.  However, EfiRuntimeServices*
> memory needs to be included in the EFI page tables, so it is best to
> minimize the amount of memory of this type.  Since EFI runtime services
> do not need access to the ESRT, EfiACPIReclaimMemory is a better choice.
> 
> Link: https://lists.xenproject.org/archives/html/xen-devel/2022-09/msg01365.html
> Fixes: dc7da0874ba4 ("EFI: preserve the System Resource Table for dom0")
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>

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

> Should this be included in 4.17?  It is a bug fix for a feature new to
> 4.17, so I suspect yes, but it is ultimately up to Henry Wang.  The code
> is identical to v2, but I have improved the commit message.

It may be too late now, looking at the state of the tree. Henry, Julien?

Jan

RE: [PATCH v3] Use EfiACPIReclaimMemory for ESRT

Posted by Henry Wang 1 year, 4 months ago

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Subject: Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT
> 
> On 07.12.2022 23:42, Demi Marie Obenour wrote:
> > A previous patch tried to get Linux to use the ESRT under Xen if it is
> > in memory of type EfiRuntimeServicesData.  However, EfiRuntimeServices*
> > memory needs to be included in the EFI page tables, so it is best to
> > minimize the amount of memory of this type.  Since EFI runtime services
> > do not need access to the ESRT, EfiACPIReclaimMemory is a better choice.
> >
> > Link: https://lists.xenproject.org/archives/html/xen-devel/2022-
> 09/msg01365.html
> > Fixes: dc7da0874ba4 ("EFI: preserve the System Resource Table for dom0")
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> > Should this be included in 4.17?  It is a bug fix for a feature new to
> > 4.17, so I suspect yes, but it is ultimately up to Henry Wang.  The code
> > is identical to v2, but I have improved the commit message.
> 
> It may be too late now, looking at the state of the tree. Henry, Julien?

Like I said in v2, I don't object the change if you would like to include this patch
to 4.17, so if you are sure this patch is safe and want to commit it, feel free to add:

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Since we also need to commit:
"[PATCH for-4.17] SUPPORT.md: Define support lifetime" so from my side
I am no problem. Julien might have different opinion though, if Julien object
the change I would like to respect his opinion and leave this patch uncommitted.

Kind regards,
Henry

> 
> Jan

Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT

Posted by Julien Grall 1 year, 4 months ago

Hi,

On 08/12/2022 08:53, Henry Wang wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Subject: Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT
>>
>> On 07.12.2022 23:42, Demi Marie Obenour wrote:
>>> A previous patch tried to get Linux to use the ESRT under Xen if it is
>>> in memory of type EfiRuntimeServicesData.  However, EfiRuntimeServices*
>>> memory needs to be included in the EFI page tables, so it is best to
>>> minimize the amount of memory of this type.  Since EFI runtime services
>>> do not need access to the ESRT, EfiACPIReclaimMemory is a better choice.
>>>
>>> Link: https://lists.xenproject.org/archives/html/xen-devel/2022-
>> 09/msg01365.html
>>> Fixes: dc7da0874ba4 ("EFI: preserve the System Resource Table for dom0")
>>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
>>
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>>> Should this be included in 4.17?  It is a bug fix for a feature new to
>>> 4.17, so I suspect yes, but it is ultimately up to Henry Wang.  The code
>>> is identical to v2, but I have improved the commit message.
>>
>> It may be too late now, looking at the state of the tree. Henry, Julien?
> 
> Like I said in v2, I don't object the change if you would like to include this patch
> to 4.17, so if you are sure this patch is safe and want to commit it, feel free to add:
> 
> Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> 
> Since we also need to commit:
> "[PATCH for-4.17] SUPPORT.md: Define support lifetime" so from my side
> I am no problem. Julien might have different opinion though, if Julien object
> the change I would like to respect his opinion and leave this patch uncommitted.

I have committed it after SUPPORT.md. So if for some reasons we are seen 
any issues with Osstest, then I can tag the tree without this patch 
(that said, I would rather prefer if we have staging-4.17 == stable-4.17).

My plan is to prepare the tarball tomorrow.

Cheers,

-- 
Julien Grall

RE: [PATCH v3] Use EfiACPIReclaimMemory for ESRT

Posted by Henry Wang 1 year, 4 months ago

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT
> 
> Hi,
> 
> >>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> >>
> >> Acked-by: Jan Beulich <jbeulich@suse.com>
> >>
> >>> Should this be included in 4.17?  It is a bug fix for a feature new to
> >>> 4.17, so I suspect yes, but it is ultimately up to Henry Wang.  The code
> >>> is identical to v2, but I have improved the commit message.
> >>
> >> It may be too late now, looking at the state of the tree. Henry, Julien?
> >
> > Like I said in v2, I don't object the change if you would like to include this
> patch
> > to 4.17, so if you are sure this patch is safe and want to commit it, feel free
> to add:
> >
> > Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> >
> > Since we also need to commit:
> > "[PATCH for-4.17] SUPPORT.md: Define support lifetime" so from my side
> > I am no problem. Julien might have different opinion though, if Julien
> object
> > the change I would like to respect his opinion and leave this patch
> uncommitted.
> 
> I have committed it after SUPPORT.md. So if for some reasons we are seen
> any issues with Osstest, then I can tag the tree without this patch

This is a great solution :)

> (that said, I would rather prefer if we have staging-4.17 == stable-4.17).

Looks like now staging-4.17 == stable-4.17 now, with this patch pushed.
So we are ready to tag.

> 
> My plan is to prepare the tarball tomorrow.

Thanks very much.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT

Posted by Demi Marie Obenour 1 year, 4 months ago

On Fri, Dec 09, 2022 at 07:37:53AM +0000, Henry Wang wrote:
> Hi Julien,
> 
> > -----Original Message-----
> > From: Julien Grall <julien@xen.org>
> > Subject: Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT
> > 
> > Hi,
> > 
> > >>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > >>
> > >> Acked-by: Jan Beulich <jbeulich@suse.com>
> > >>
> > >>> Should this be included in 4.17?  It is a bug fix for a feature new to
> > >>> 4.17, so I suspect yes, but it is ultimately up to Henry Wang.  The code
> > >>> is identical to v2, but I have improved the commit message.
> > >>
> > >> It may be too late now, looking at the state of the tree. Henry, Julien?
> > >
> > > Like I said in v2, I don't object the change if you would like to include this
> > patch
> > > to 4.17, so if you are sure this patch is safe and want to commit it, feel free
> > to add:
> > >
> > > Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> > >
> > > Since we also need to commit:
> > > "[PATCH for-4.17] SUPPORT.md: Define support lifetime" so from my side
> > > I am no problem. Julien might have different opinion though, if Julien
> > object
> > > the change I would like to respect his opinion and leave this patch
> > uncommitted.
> > 
> > I have committed it after SUPPORT.md. So if for some reasons we are seen
> > any issues with Osstest, then I can tag the tree without this patch
> 
> This is a great solution :)
> 
> > (that said, I would rather prefer if we have staging-4.17 == stable-4.17).
> 
> Looks like now staging-4.17 == stable-4.17 now, with this patch pushed.
> So we are ready to tag.

And it turns out that I botched the initial patch, sorry.  (I forgot to
handle the multiboot2 case.)

I understand if it is too late for stable-4.17, but it ought to make
stable 4.17.1 as it was simply omitted from the initial patch series.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

RE: [PATCH v3] Use EfiACPIReclaimMemory for ESRT

Posted by Henry Wang 1 year, 4 months ago

Hi,

> -----Original Message-----
> From: Demi Marie Obenour <demi@invisiblethingslab.com>
> Subject: Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT
> 
> On Fri, Dec 09, 2022 at 07:37:53AM +0000, Henry Wang wrote:
> > Hi Julien,
> >
> > > -----Original Message-----
> > > From: Julien Grall <julien@xen.org>
> > > Subject: Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT
> > >
> > > Hi,
> > >
> > > >>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > >>
> > > >> Acked-by: Jan Beulich <jbeulich@suse.com>
> > > >>
> > > >>> Should this be included in 4.17?  It is a bug fix for a feature new to
> > > >>> 4.17, so I suspect yes, but it is ultimately up to Henry Wang.  The code
> > > >>> is identical to v2, but I have improved the commit message.
> > > >>
> > > >> It may be too late now, looking at the state of the tree. Henry, Julien?
> > > >
> > > > Like I said in v2, I don't object the change if you would like to include
> this
> > > patch
> > > > to 4.17, so if you are sure this patch is safe and want to commit it, feel
> free
> > > to add:
> > > >
> > > > Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> > > >
> > > > Since we also need to commit:
> > > > "[PATCH for-4.17] SUPPORT.md: Define support lifetime" so from my
> side
> > > > I am no problem. Julien might have different opinion though, if Julien
> > > object
> > > > the change I would like to respect his opinion and leave this patch
> > > uncommitted.
> > >
> > > I have committed it after SUPPORT.md. So if for some reasons we are
> seen
> > > any issues with Osstest, then I can tag the tree without this patch
> >
> > This is a great solution :)
> >
> > > (that said, I would rather prefer if we have staging-4.17 == stable-4.17).
> >
> > Looks like now staging-4.17 == stable-4.17 now, with this patch pushed.
> > So we are ready to tag.
> 
> And it turns out that I botched the initial patch, sorry.  (I forgot to
> handle the multiboot2 case.)
> 
> I understand if it is too late for stable-4.17, but it ought to make
> stable 4.17.1 as it was simply omitted from the initial patch series.

I don't think this patch will make it today so I would suggest we still follow
what Julien planned yesterday. Also I think this is also consistent with the
release management guideline.

Kind regards,
Henry


> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab

Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT

Posted by Demi Marie Obenour 1 year, 4 months ago

On Fri, Dec 09, 2022 at 02:54:15PM +0000, Henry Wang wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Demi Marie Obenour <demi@invisiblethingslab.com>
> > Subject: Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT
> > 
> > On Fri, Dec 09, 2022 at 07:37:53AM +0000, Henry Wang wrote:
> > > Hi Julien,
> > >
> > > > -----Original Message-----
> > > > From: Julien Grall <julien@xen.org>
> > > > Subject: Re: [PATCH v3] Use EfiACPIReclaimMemory for ESRT
> > > >
> > > > Hi,
> > > >
> > > > >>> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > >>
> > > > >> Acked-by: Jan Beulich <jbeulich@suse.com>
> > > > >>
> > > > >>> Should this be included in 4.17?  It is a bug fix for a feature new to
> > > > >>> 4.17, so I suspect yes, but it is ultimately up to Henry Wang.  The code
> > > > >>> is identical to v2, but I have improved the commit message.
> > > > >>
> > > > >> It may be too late now, looking at the state of the tree. Henry, Julien?
> > > > >
> > > > > Like I said in v2, I don't object the change if you would like to include
> > this
> > > > patch
> > > > > to 4.17, so if you are sure this patch is safe and want to commit it, feel
> > free
> > > > to add:
> > > > >
> > > > > Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> > > > >
> > > > > Since we also need to commit:
> > > > > "[PATCH for-4.17] SUPPORT.md: Define support lifetime" so from my
> > side
> > > > > I am no problem. Julien might have different opinion though, if Julien
> > > > object
> > > > > the change I would like to respect his opinion and leave this patch
> > > > uncommitted.
> > > >
> > > > I have committed it after SUPPORT.md. So if for some reasons we are
> > seen
> > > > any issues with Osstest, then I can tag the tree without this patch
> > >
> > > This is a great solution :)
> > >
> > > > (that said, I would rather prefer if we have staging-4.17 == stable-4.17).
> > >
> > > Looks like now staging-4.17 == stable-4.17 now, with this patch pushed.
> > > So we are ready to tag.
> > 
> > And it turns out that I botched the initial patch, sorry.  (I forgot to
> > handle the multiboot2 case.)
> > 
> > I understand if it is too late for stable-4.17, but it ought to make
> > stable 4.17.1 as it was simply omitted from the initial patch series.
> 
> I don't think this patch will make it today so I would suggest we still follow
> what Julien planned yesterday. Also I think this is also consistent with the
> release management guideline.

That’s okay.  Qubes can take this as an out of tree patch until it is
merged.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab