xen/common/efi/boot.c | 68 ++++++++++++++++++++++++++++++++++++++-- xen/common/efi/efi.h | 17 ++++++++++ xen/common/efi/runtime.c | 2 +- xen/include/efi/efiapi.h | 3 ++ 4 files changed, 86 insertions(+), 4 deletions(-)
The EFI System Resource Table (ESRT) is necessary for fwupd to identify
firmware updates to install. According to the UEFI specification §23.4,
the ESRT shall be stored in memory of type EfiBootServicesData. However,
memory of type EfiBootServicesData is considered general-purpose memory
by Xen, so the ESRT needs to be moved somewhere where Xen will not
overwrite it. Copy the ESRT to memory of type EfiRuntimeServicesData,
which Xen will not reuse. dom0 can use the ESRT if (and only if) it is
in memory of type EfiRuntimeServicesData.
Earlier versions of this patch reserved the memory in which the ESRT was
located. This created awkward alignment problems, and required either
splitting the E820 table or wasting memory. It also would have required
a new platform op for dom0 to use to indicate if the ESRT is reserved.
By copying the ESRT into EfiRuntimeServicesData memory, the E820 table
does not need to be modified, and dom0 can just check the type of the
memory region containing the ESRT. The copy is only done if the ESRT is
not already in EfiRuntimeServicesData memory, avoiding memory leaks on
repeated kexec.
See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/
for details.
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
xen/common/efi/boot.c | 68 ++++++++++++++++++++++++++++++++++++++--
xen/common/efi/efi.h | 17 ++++++++++
xen/common/efi/runtime.c | 2 +-
xen/include/efi/efiapi.h | 3 ++
4 files changed, 86 insertions(+), 4 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index a25e1d29f1..4b22dc1bb7 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -567,6 +567,37 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
}
#endif
+static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
+
+static bool __init is_esrt_valid(const EFI_MEMORY_DESCRIPTOR *desc)
+{
+ size_t available_len, len;
+ const UINTN physical_start = desc->PhysicalStart;
+ const ESRT *esrt_ptr;
+
+ len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+ if ( esrt == EFI_INVALID_TABLE_ADDR )
+ return false;
+ if ( physical_start > esrt || esrt - physical_start >= len )
+ return false;
+ /*
+ * The specification requires EfiBootServicesData, but accept
+ * EfiRuntimeServicesData, which is a more logical choice.
+ */
+ if ( (desc->Type != EfiRuntimeServicesData) &&
+ (desc->Type != EfiBootServicesData) )
+ return false;
+ available_len = len - (esrt - physical_start);
+ if ( available_len < sizeof(*esrt_ptr) )
+ return false;
+ esrt_ptr = (const ESRT *)esrt;
+ if ( esrt_ptr->Version != 1 || !esrt_ptr->Count )
+ return false;
+ return esrt_ptr->Count <=
+ (available_len - sizeof(*esrt_ptr)) /
+ sizeof(esrt_ptr->Entries[0]);
+}
+
/*
* Include architecture specific implementation here, which references the
* static globals defined above.
@@ -845,6 +876,8 @@ static UINTN __init efi_find_gop_mode(EFI_GRAPHICS_OUTPUT_PROTOCOL *gop,
return gop_mode;
}
+static EFI_GUID __initdata esrt_guid = ESRT_GUID;
+
static void __init efi_tables(void)
{
unsigned int i;
@@ -868,6 +901,8 @@ static void __init efi_tables(void)
efi.smbios = (unsigned long)efi_ct[i].VendorTable;
if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) )
efi.smbios3 = (unsigned long)efi_ct[i].VendorTable;
+ if ( match_guid(&esrt_guid, &efi_ct[i].VendorGuid) )
+ esrt = (UINTN)efi_ct[i].VendorTable;
}
#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
@@ -1056,13 +1091,11 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
EFI_STATUS status;
UINTN info_size = 0, map_key;
bool retry;
-#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
unsigned int i;
-#endif
efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
&efi_mdesc_size, &mdesc_ver);
- info_size += 8 * efi_mdesc_size;
+ info_size += 8 * (efi_mdesc_size + 1);
efi_memmap = efi_arch_allocate_mmap_buffer(info_size);
if ( !efi_memmap )
blexit(L"Unable to allocate memory for EFI memory map");
@@ -1077,6 +1110,35 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
if ( EFI_ERROR(status) )
PrintErrMesg(L"Cannot obtain memory map", status);
+ for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
+ {
+ if ( !is_esrt_valid(efi_memmap + i) )
+ continue;
+ if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type !=
+ EfiRuntimeServicesData )
+ {
+ /* ESRT needs to be moved to memory of type EfiRuntimeServicesData
+ * so that the memory it is in will not be used for other purposes */
+ size_t esrt_size = offsetof(ESRT, Entries) +
+ ((ESRT *)esrt)->Count * sizeof(ESRT_ENTRY);
+ void *new_esrt = NULL;
+ status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size, &new_esrt);
+ if ( status != EFI_SUCCESS )
+ {
+ PrintErrMesg(L"Cannot allocate memory for ESRT", status);
+ break;
+ }
+ memcpy(new_esrt, (void *)esrt, esrt_size);
+ status = efi_bs->InstallConfigurationTable(&esrt_guid, new_esrt);
+ if ( status != EFI_SUCCESS )
+ {
+ PrintErrMesg(L"Cannot install new ESRT", status);
+ efi_bs->FreePool(new_esrt);
+ }
+ }
+ break;
+ }
+
efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
efi_mdesc_size, mdesc_ver);
diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index c9aa65d506..bf94dfcdf6 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -10,6 +10,23 @@
#include <xen/spinlock.h>
#include <asm/page.h>
+typedef struct _ESRT_ENTRY {
+ EFI_GUID FwClass;
+ UINT32 FwType;
+ UINT32 FwVersion;
+ UINT32 FwLowestSupportedVersion;
+ UINT32 FwCapsuleFlags;
+ UINT32 FwLastAttemptVersion;
+ UINT32 FwLastAttemptStatus;
+} ESRT_ENTRY;
+
+typedef struct _ESRT {
+ UINT32 Count;
+ UINT32 Max;
+ UINT64 Version;
+ ESRT_ENTRY Entries[];
+} ESRT;
+
struct efi_pci_rom {
const struct efi_pci_rom *next;
u16 vendor, devid, segment;
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 13b0975866..64e9f04671 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -269,7 +269,7 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
case XEN_FW_EFI_MEM_INFO:
for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
{
- EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
+ const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
if ( info->mem.addr >= desc->PhysicalStart &&
diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
index a616d1238a..42ef3e1c8c 100644
--- a/xen/include/efi/efiapi.h
+++ b/xen/include/efi/efiapi.h
@@ -882,6 +882,9 @@ typedef struct _EFI_BOOT_SERVICES {
#define SAL_SYSTEM_TABLE_GUID \
{ 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d} }
+#define ESRT_GUID \
+ { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }
+
typedef struct _EFI_CONFIGURATION_TABLE {
EFI_GUID VendorGuid;
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
On 05.05.2022 07:38, Demi Marie Obenour wrote:
> @@ -1056,13 +1091,11 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> EFI_STATUS status;
> UINTN info_size = 0, map_key;
> bool retry;
> -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
> unsigned int i;
> -#endif
>
> efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
> &efi_mdesc_size, &mdesc_ver);
> - info_size += 8 * efi_mdesc_size;
> + info_size += 8 * (efi_mdesc_size + 1);
I think I did ask on an earlier version already why you're making this
change. It continues to look to me like a leftover which was needed by
an early version only.
> @@ -1077,6 +1110,35 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> if ( EFI_ERROR(status) )
> PrintErrMesg(L"Cannot obtain memory map", status);
>
> + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> + {
> + if ( !is_esrt_valid(efi_memmap + i) )
> + continue;
Instead of repeating the size calculation below, could you make the
function (with an altered name) simply return the size (and zero if
not [valid] ESRT), simplifying things below?
> + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type !=
> + EfiRuntimeServicesData )
> + {
> + /* ESRT needs to be moved to memory of type EfiRuntimeServicesData
> + * so that the memory it is in will not be used for other purposes */
Nit: Comment style.
> + size_t esrt_size = offsetof(ESRT, Entries) +
> + ((ESRT *)esrt)->Count * sizeof(ESRT_ENTRY);
> + void *new_esrt = NULL;
> + status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size, &new_esrt);
Nit: Please have a blank line between declaration(s) and statement(s).
> + if ( status != EFI_SUCCESS )
> + {
> + PrintErrMesg(L"Cannot allocate memory for ESRT", status);
Neither this nor ...
> + break;
> + }
> + memcpy(new_esrt, (void *)esrt, esrt_size);
> + status = efi_bs->InstallConfigurationTable(&esrt_guid, new_esrt);
> + if ( status != EFI_SUCCESS )
> + {
> + PrintErrMesg(L"Cannot install new ESRT", status);
> + efi_bs->FreePool(new_esrt);
... this ought to be fatal to the booting of Xen. Yet PrintErrMesg()
ends in blexit().
> + }
> + }
> + break;
> + }
> +
> efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
> efi_mdesc_size, mdesc_ver);
The allocation may have altered the memory map and hence invalidated what
was retrieved just before. You'd need to "continue;" without setting
"retry" to true, but then the question is why you make this allocation
after retrieving the memory map in the first place. It's not entirely
clear to me if it can be done _much_ earlier (if it can, doing it earlier
would of course be better), but since you need to do it before
ExitBootServices() anyway, and since you will need to call GetMemoryMap()
afterwards again, you could as well do it before calling GetMemoryMap().
> --- a/xen/common/efi/efi.h
> +++ b/xen/common/efi/efi.h
> @@ -10,6 +10,23 @@
> #include <xen/spinlock.h>
> #include <asm/page.h>
>
> +typedef struct _ESRT_ENTRY {
> + EFI_GUID FwClass;
> + UINT32 FwType;
> + UINT32 FwVersion;
> + UINT32 FwLowestSupportedVersion;
> + UINT32 FwCapsuleFlags;
> + UINT32 FwLastAttemptVersion;
> + UINT32 FwLastAttemptStatus;
> +} ESRT_ENTRY;
> +
> +typedef struct _ESRT {
> + UINT32 Count;
> + UINT32 Max;
> + UINT64 Version;
> + ESRT_ENTRY Entries[];
> +} ESRT;
I'm pretty sure I did indicate before that types used in just a single
source file should be put in that source file, unless we obtain them
by importing a header (e.g. the ones in include/efi/) from elsewhere.
> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -269,7 +269,7 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
> case XEN_FW_EFI_MEM_INFO:
> for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> {
> - EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> + const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
>
> if ( info->mem.addr >= desc->PhysicalStart &&
With the restructured approach I don't think this stray change should
be left in here anymore. Or am I overlooking anything requiring this
adjustment?
> --- a/xen/include/efi/efiapi.h
> +++ b/xen/include/efi/efiapi.h
> @@ -882,6 +882,9 @@ typedef struct _EFI_BOOT_SERVICES {
> #define SAL_SYSTEM_TABLE_GUID \
> { 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d} }
>
> +#define ESRT_GUID \
> + { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }
Like above I'm pretty sure I did ask that you do not alter this
imported header. If gnu-efi now has these definitions, we should
import them all in one go (i.e. then the two struct declarations
would also want to go into their appropriate place under include/efi/.
Otherwise this wants putting next to the other GUIDs defined in
boot.c.
Jan
On Fri, May 06, 2022 at 12:59:05PM +0200, Jan Beulich wrote:
> On 05.05.2022 07:38, Demi Marie Obenour wrote:
> > @@ -1056,13 +1091,11 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> > EFI_STATUS status;
> > UINTN info_size = 0, map_key;
> > bool retry;
> > -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
> > unsigned int i;
> > -#endif
> >
> > efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
> > &efi_mdesc_size, &mdesc_ver);
> > - info_size += 8 * efi_mdesc_size;
> > + info_size += 8 * (efi_mdesc_size + 1);
>
> I think I did ask on an earlier version already why you're making this
> change. It continues to look to me like a leftover which was needed by
> an early version only.
Will revert in v5.
> > @@ -1077,6 +1110,35 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> > if ( EFI_ERROR(status) )
> > PrintErrMesg(L"Cannot obtain memory map", status);
> >
> > + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> > + {
> > + if ( !is_esrt_valid(efi_memmap + i) )
> > + continue;
>
> Instead of repeating the size calculation below, could you make the
> function (with an altered name) simply return the size (and zero if
> not [valid] ESRT), simplifying things below?
Will fix in v5.
> > + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type !=
> > + EfiRuntimeServicesData )
> > + {
> > + /* ESRT needs to be moved to memory of type EfiRuntimeServicesData
> > + * so that the memory it is in will not be used for other purposes */
>
> Nit: Comment style.
Will fix in v5.
> > + size_t esrt_size = offsetof(ESRT, Entries) +
> > + ((ESRT *)esrt)->Count * sizeof(ESRT_ENTRY);
> > + void *new_esrt = NULL;
> > + status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size, &new_esrt);
>
> Nit: Please have a blank line between declaration(s) and statement(s).
Will fix in v5.
> > + if ( status != EFI_SUCCESS )
> > + {
> > + PrintErrMesg(L"Cannot allocate memory for ESRT", status);
>
> Neither this nor ...
>
> > + break;
> > + }
> > + memcpy(new_esrt, (void *)esrt, esrt_size);
> > + status = efi_bs->InstallConfigurationTable(&esrt_guid, new_esrt);
> > + if ( status != EFI_SUCCESS )
> > + {
> > + PrintErrMesg(L"Cannot install new ESRT", status);
> > + efi_bs->FreePool(new_esrt);
>
> ... this ought to be fatal to the booting of Xen. Yet PrintErrMesg()
> ends in blexit().
Whoops! I did not realized PrintErrMsg() was fatal.
> > + }
> > + }
> > + break;
> > + }
> > +
> > efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
> > efi_mdesc_size, mdesc_ver);
>
> The allocation may have altered the memory map and hence invalidated what
> was retrieved just before. You'd need to "continue;" without setting
> "retry" to true, but then the question is why you make this allocation
> after retrieving the memory map in the first place. It's not entirely
> clear to me if it can be done _much_ earlier (if it can, doing it earlier
> would of course be better), but since you need to do it before
> ExitBootServices() anyway, and since you will need to call GetMemoryMap()
> afterwards again, you could as well do it before calling GetMemoryMap().
This would mean the allocation would need to be unconditional. Right
now, I avoid the allocation if it is not necessary.
> > --- a/xen/common/efi/efi.h
> > +++ b/xen/common/efi/efi.h
> > @@ -10,6 +10,23 @@
> > #include <xen/spinlock.h>
> > #include <asm/page.h>
> >
> > +typedef struct _ESRT_ENTRY {
> > + EFI_GUID FwClass;
> > + UINT32 FwType;
> > + UINT32 FwVersion;
> > + UINT32 FwLowestSupportedVersion;
> > + UINT32 FwCapsuleFlags;
> > + UINT32 FwLastAttemptVersion;
> > + UINT32 FwLastAttemptStatus;
> > +} ESRT_ENTRY;
> > +
> > +typedef struct _ESRT {
> > + UINT32 Count;
> > + UINT32 Max;
> > + UINT64 Version;
> > + ESRT_ENTRY Entries[];
> > +} ESRT;
>
> I'm pretty sure I did indicate before that types used in just a single
> source file should be put in that source file, unless we obtain them
> by importing a header (e.g. the ones in include/efi/) from elsewhere.
Will fix in v5.
> > --- a/xen/common/efi/runtime.c
> > +++ b/xen/common/efi/runtime.c
> > @@ -269,7 +269,7 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
> > case XEN_FW_EFI_MEM_INFO:
> > for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> > {
> > - EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> > + const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> > u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> >
> > if ( info->mem.addr >= desc->PhysicalStart &&
>
> With the restructured approach I don't think this stray change should
> be left in here anymore. Or am I overlooking anything requiring this
> adjustment?
It’s a trivial cleanup but I can get rid of it.
> > --- a/xen/include/efi/efiapi.h
> > +++ b/xen/include/efi/efiapi.h
> > @@ -882,6 +882,9 @@ typedef struct _EFI_BOOT_SERVICES {
> > #define SAL_SYSTEM_TABLE_GUID \
> > { 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d} }
> >
> > +#define ESRT_GUID \
> > + { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }
>
> Like above I'm pretty sure I did ask that you do not alter this
> imported header. If gnu-efi now has these definitions, we should
> import them all in one go (i.e. then the two struct declarations
> would also want to go into their appropriate place under include/efi/.
> Otherwise this wants putting next to the other GUIDs defined in
> boot.c.
Will fix in v5.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
On 07.05.2022 06:26, Demi Marie Obenour wrote:
> On Fri, May 06, 2022 at 12:59:05PM +0200, Jan Beulich wrote:
>> On 05.05.2022 07:38, Demi Marie Obenour wrote:
>>> @@ -1077,6 +1110,35 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>>> if ( EFI_ERROR(status) )
>>> PrintErrMesg(L"Cannot obtain memory map", status);
>>>
>>> + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
>>> + {
>>> + if ( !is_esrt_valid(efi_memmap + i) )
>>> + continue;
>>
>> Instead of repeating the size calculation below, could you make the
>> function (with an altered name) simply return the size (and zero if
>> not [valid] ESRT), simplifying things below?
>
> Will fix in v5.
>
>>> + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type !=
>>> + EfiRuntimeServicesData )
>>> + {
>>> + /* ESRT needs to be moved to memory of type EfiRuntimeServicesData
>>> + * so that the memory it is in will not be used for other purposes */
>>
>> Nit: Comment style.
>
> Will fix in v5.
>
>>> + size_t esrt_size = offsetof(ESRT, Entries) +
>>> + ((ESRT *)esrt)->Count * sizeof(ESRT_ENTRY);
>>> + void *new_esrt = NULL;
>>> + status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size, &new_esrt);
>>
>> Nit: Please have a blank line between declaration(s) and statement(s).
>
> Will fix in v5.
>
>>> + if ( status != EFI_SUCCESS )
>>> + {
>>> + PrintErrMesg(L"Cannot allocate memory for ESRT", status);
>>
>> Neither this nor ...
>>
>>> + break;
>>> + }
>>> + memcpy(new_esrt, (void *)esrt, esrt_size);
>>> + status = efi_bs->InstallConfigurationTable(&esrt_guid, new_esrt);
>>> + if ( status != EFI_SUCCESS )
>>> + {
>>> + PrintErrMesg(L"Cannot install new ESRT", status);
>>> + efi_bs->FreePool(new_esrt);
>>
>> ... this ought to be fatal to the booting of Xen. Yet PrintErrMesg()
>> ends in blexit().
>
> Whoops! I did not realized PrintErrMsg() was fatal.
>
>>> + }
>>> + }
>>> + break;
>>> + }
>>> +
>>> efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
>>> efi_mdesc_size, mdesc_ver);
>>
>> The allocation may have altered the memory map and hence invalidated what
>> was retrieved just before. You'd need to "continue;" without setting
>> "retry" to true, but then the question is why you make this allocation
>> after retrieving the memory map in the first place. It's not entirely
>> clear to me if it can be done _much_ earlier (if it can, doing it earlier
>> would of course be better), but since you need to do it before
>> ExitBootServices() anyway, and since you will need to call GetMemoryMap()
>> afterwards again, you could as well do it before calling GetMemoryMap().
>
> This would mean the allocation would need to be unconditional. Right
> now, I avoid the allocation if it is not necessary.
Hmm, I guess I don't see (taking into account also my own reply to that
comment of mine) why it would need to become unconditional then.
Jan
On Tue, May 17, 2022 at 03:16:55PM +0200, Jan Beulich wrote: > On 07.05.2022 06:26, Demi Marie Obenour wrote: > > This would mean the allocation would need to be unconditional. Right > > now, I avoid the allocation if it is not necessary. > > Hmm, I guess I don't see (taking into account also my own reply to that > comment of mine) why it would need to become unconditional then. It did not need to become unconditional. See v5. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab
On 06.05.2022 12:59, Jan Beulich wrote:
> On 05.05.2022 07:38, Demi Marie Obenour wrote:
>> @@ -1077,6 +1110,35 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>> if ( EFI_ERROR(status) )
>> PrintErrMesg(L"Cannot obtain memory map", status);
>>
>> + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
>> + {
>> + if ( !is_esrt_valid(efi_memmap + i) )
>> + continue;
>
> Instead of repeating the size calculation below, could you make the
> function (with an altered name) simply return the size (and zero if
> not [valid] ESRT), simplifying things below?
>
>> + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type !=
>> + EfiRuntimeServicesData )
>> + {
>> + /* ESRT needs to be moved to memory of type EfiRuntimeServicesData
>> + * so that the memory it is in will not be used for other purposes */
>
> Nit: Comment style.
>
>> + size_t esrt_size = offsetof(ESRT, Entries) +
>> + ((ESRT *)esrt)->Count * sizeof(ESRT_ENTRY);
>> + void *new_esrt = NULL;
>> + status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size, &new_esrt);
>
> Nit: Please have a blank line between declaration(s) and statement(s).
>
>> + if ( status != EFI_SUCCESS )
>> + {
>> + PrintErrMesg(L"Cannot allocate memory for ESRT", status);
>
> Neither this nor ...
>
>> + break;
>> + }
>> + memcpy(new_esrt, (void *)esrt, esrt_size);
>> + status = efi_bs->InstallConfigurationTable(&esrt_guid, new_esrt);
>> + if ( status != EFI_SUCCESS )
>> + {
>> + PrintErrMesg(L"Cannot install new ESRT", status);
>> + efi_bs->FreePool(new_esrt);
>
> ... this ought to be fatal to the booting of Xen. Yet PrintErrMesg()
> ends in blexit().
>
>> + }
>> + }
>> + break;
>> + }
>> +
>> efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
>> efi_mdesc_size, mdesc_ver);
>
> The allocation may have altered the memory map and hence invalidated what
> was retrieved just before. You'd need to "continue;" without setting
> "retry" to true, but then the question is why you make this allocation
> after retrieving the memory map in the first place. It's not entirely
> clear to me if it can be done _much_ earlier (if it can, doing it earlier
> would of course be better), but since you need to do it before
> ExitBootServices() anyway, and since you will need to call GetMemoryMap()
> afterwards again, you could as well do it before calling GetMemoryMap().
Over lunch I figured that this was partly rubbish. Of course you need to
do the check after GetMemoryMap(). But I still think it would be better if
you moved this out of this function (or at the very least out of the loop)
and not piggy-back on the ExitBootServices() retry mechanism. I'd be
afraid this could end up in a single retry not being sufficient.
Jan
© 2016 - 2024 Red Hat, Inc.