xen/common/efi/boot.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
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
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 ) > {
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
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.
© 2016 - 2024 Red Hat, Inc.