xen/common/efi/boot.c | 108 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 106 insertions(+), 2 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>
---
Changes since v5:
- Use type and field names from EFI specification.
- Remove tags from EFI types that do not have them.
- Use PrintErr() instead of PrintStr() for error conditions.
Changes since v4:
- Do not call blexit() (via PrintErrMsg()) if the new ESRT cannot be
allocated or installed.
- Use the correct comment style.
- Remove leftover change from earlier version.
- Add a blank line between declarations and statements.
- Re-fetch the memory map after allocating the ESRT copy.
Changes since v3:
- Do not modify the memory map.
- Allocate a copy of the ESRT in EfiRuntimeServicesData() memory.
xen/common/efi/boot.c | 108 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 106 insertions(+), 2 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index a25e1d29f1..6a829b8278 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -39,6 +39,26 @@
{ 0x605dab50, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} }
#define APPLE_PROPERTIES_PROTOCOL_GUID \
{ 0x91bd12fe, 0xf6c3, 0x44fb, { 0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0} }
+#define EFI_SYSTEM_RESOURCE_TABLE_GUID \
+ { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }
+#define EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION 1
+
+typedef struct {
+ EFI_GUID FwClass;
+ UINT32 FwType;
+ UINT32 FwVersion;
+ UINT32 LowestSupportedFwVersion;
+ UINT32 CapsuleFlags;
+ UINT32 LastAttemptVersion;
+ UINT32 LastAttemptStatus;
+} EFI_SYSTEM_RESOURCE_ENTRY;
+
+typedef struct {
+ UINT32 FwResourceCount;
+ UINT32 FwResourceCountMax;
+ UINT64 FwResourceVersion;
+ EFI_SYSTEM_RESOURCE_ENTRY Entries[];
+} EFI_SYSTEM_RESOURCE_TABLE;
typedef EFI_STATUS
(/* _not_ EFIAPI */ *EFI_SHIM_LOCK_VERIFY) (
@@ -567,6 +587,39 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
}
#endif
+static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
+
+static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
+{
+ size_t available_len, len;
+ const UINTN physical_start = desc->PhysicalStart;
+ const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
+
+ len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+ if ( esrt == EFI_INVALID_TABLE_ADDR )
+ return 0;
+ if ( physical_start > esrt || esrt - physical_start >= len )
+ return 0;
+ /*
+ * The specification requires EfiBootServicesData, but accept
+ * EfiRuntimeServicesData, which is a more logical choice.
+ */
+ if ( (desc->Type != EfiRuntimeServicesData) &&
+ (desc->Type != EfiBootServicesData) )
+ return 0;
+ available_len = len - (esrt - physical_start);
+ if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
+ return 0;
+ available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
+ esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
+ if ( esrt_ptr->FwResourceVersion != EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION ||
+ !esrt_ptr->FwResourceCount )
+ return 0;
+ if ( esrt_ptr->FwResourceCount > available_len / sizeof(esrt_ptr->Entries[0]) )
+ return 0;
+ return esrt_ptr->FwResourceCount * sizeof(esrt_ptr->Entries[0]);
+}
+
/*
* Include architecture specific implementation here, which references the
* static globals defined above.
@@ -845,6 +898,8 @@ 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;
@@ -868,6 +923,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,9 +1113,7 @@ 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);
@@ -1067,6 +1122,46 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
if ( !efi_memmap )
blexit(L"Unable to allocate memory for EFI memory map");
+ efi_memmap_size = info_size;
+ status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
+ efi_memmap, &map_key,
+ &efi_mdesc_size,
+ &mdesc_ver);
+ if ( EFI_ERROR(status) )
+ PrintErrMesg(L"Cannot obtain memory map", status);
+
+ /* Try to obtain the ESRT. Errors are not fatal. */
+ for ( i = 0; i < efi_memmap_size; i += efi_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(efi_memmap + i);
+
+ if ( !esrt_size )
+ continue;
+ if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + 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;
+ }
+
for ( retry = false; ; retry = true )
{
efi_memmap_size = info_size;
@@ -1753,3 +1848,12 @@ void __init efi_init_memory(void)
unmap_domain_page(efi_l4t);
}
#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
On 18.05.2022 19:32, Demi Marie Obenour wrote:
> @@ -567,6 +587,39 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
> }
> #endif
>
> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
Just out of curiosity: It's an arbitrary choice to use this initializer,
i.e. no initializer (and hence zero) would do as well (with ...
> +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
> +{
> + size_t available_len, len;
> + const UINTN physical_start = desc->PhysicalStart;
> + const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
> +
> + len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> + if ( esrt == EFI_INVALID_TABLE_ADDR )
... an adjustment here, of course)?
> + return 0;
> + if ( physical_start > esrt || esrt - physical_start >= len )
> + return 0;
> + /*
> + * The specification requires EfiBootServicesData, but accept
> + * EfiRuntimeServicesData, which is a more logical choice.
> + */
> + if ( (desc->Type != EfiRuntimeServicesData) &&
> + (desc->Type != EfiBootServicesData) )
> + return 0;
> + available_len = len - (esrt - physical_start);
> + if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
> + return 0;
> + available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
> + esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
> + if ( esrt_ptr->FwResourceVersion != EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION ||
Nit (style): Overlong line.
> + !esrt_ptr->FwResourceCount )
> + return 0;
> + if ( esrt_ptr->FwResourceCount > available_len / sizeof(esrt_ptr->Entries[0]) )
> + return 0;
> + return esrt_ptr->FwResourceCount * sizeof(esrt_ptr->Entries[0]);
> +}
Nit (style again): We generally put a blank line ahead of a function's
main return statement.
> @@ -1067,6 +1122,46 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> if ( !efi_memmap )
> blexit(L"Unable to allocate memory for EFI memory map");
>
> + efi_memmap_size = info_size;
I don't think this global needs setting here, yet? The local will
do just fine here, likely yielding smaller code. But I realize that's
connected to how you did your change vs what I was expecting you to
do (see below).
> + status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
> + efi_memmap, &map_key,
> + &efi_mdesc_size,
> + &mdesc_ver);
> + if ( EFI_ERROR(status) )
> + PrintErrMesg(L"Cannot obtain memory map", status);
> +
> + /* Try to obtain the ESRT. Errors are not fatal. */
> + for ( i = 0; i < efi_memmap_size; i += efi_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(efi_memmap + i);
> +
> + if ( !esrt_size )
> + continue;
> + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type ==
> + EfiRuntimeServicesData )
> + break; /* ESRT already safe from reuse */
> + status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
> + &new_esrt);
I should have re-raised the earlier voiced concern when reviewing v5 (or
maybe already v4), and I'm sorry for not having paid close enough
attention: This may add up to two more entries in the memory map (if an
entry is split and its middle part is used; of course with an unusual
implementation there could be even more of a growth). Yet below your
addition, before obtaining the final memory map, you don't re- obtain
(and re-increase) the size needed. As to (re-)increase: In fact, prior
to the allocation you do there shouldn't be a need to bump the space by
8 extra entries. That's a safety measure only for possible allocations
happening across ExitBootServices().
And yes, in earlier versions you had
- info_size += 8 * efi_mdesc_size;
+ info_size += 8 * (efi_mdesc_size + 1);
there, but that's not what would be needed anyway (if trying to avoid
a 2nd pass of establishing the needed size). Instead in such an event
you need to bump 8 to 10 (or at least 9, when assuming that normally it
wouldn't be the middle part of a new range which would be used, but
rather the leading or trailing one).
While I'd be okay with addressing the two nits above while committing,
this allocation size aspect first wants settling on. Personally I'd
prefer the more involved solution, but I'd be okay with merely
bumping the 8 (plus the addition of a suitable comment, explaining
the now multiple [two] constituent parts of a seemingly arbitrary
number). If you want to go this easier route, I guess I could also
make that adjustment while committing (and adding my R-b).
Jan
On Thu, May 19, 2022 at 12:32:33PM +0200, Jan Beulich wrote:
> On 18.05.2022 19:32, Demi Marie Obenour wrote:
> > @@ -567,6 +587,39 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
> > }
> > #endif
> >
> > +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
>
> Just out of curiosity: It's an arbitrary choice to use this initializer,
> i.e. no initializer (and hence zero) would do as well (with ...
That is correct. I chose EFI_INVALID_TABLE_ADDR because it seemed meant
for this purpose.
> > +static size_t __init get_esrt_size(const EFI_MEMORY_DESCRIPTOR *desc)
> > +{
> > + size_t available_len, len;
> > + const UINTN physical_start = desc->PhysicalStart;
> > + const EFI_SYSTEM_RESOURCE_TABLE *esrt_ptr;
> > +
> > + len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> > + if ( esrt == EFI_INVALID_TABLE_ADDR )
>
> ... an adjustment here, of course)?
>
> > + return 0;
> > + if ( physical_start > esrt || esrt - physical_start >= len )
> > + return 0;
> > + /*
> > + * The specification requires EfiBootServicesData, but accept
> > + * EfiRuntimeServicesData, which is a more logical choice.
> > + */
> > + if ( (desc->Type != EfiRuntimeServicesData) &&
> > + (desc->Type != EfiBootServicesData) )
> > + return 0;
> > + available_len = len - (esrt - physical_start);
> > + if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
> > + return 0;
> > + available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
> > + esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
> > + if ( esrt_ptr->FwResourceVersion != EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION ||
>
> Nit (style): Overlong line.
Where is the best place to split this?
EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION is a rather long
identifier.
> > + !esrt_ptr->FwResourceCount )
> > + return 0;
> > + if ( esrt_ptr->FwResourceCount > available_len / sizeof(esrt_ptr->Entries[0]) )
> > + return 0;
> > + return esrt_ptr->FwResourceCount * sizeof(esrt_ptr->Entries[0]);
> > +}
>
> Nit (style again): We generally put a blank line ahead of a function's
> main return statement.
Will fix in v7.
> > @@ -1067,6 +1122,46 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> > if ( !efi_memmap )
> > blexit(L"Unable to allocate memory for EFI memory map");
> >
> > + efi_memmap_size = info_size;
>
> I don't think this global needs setting here, yet? The local will
> do just fine here, likely yielding smaller code. But I realize that's
> connected to how you did your change vs what I was expecting you to
> do (see below).
>
> > + status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
> > + efi_memmap, &map_key,
> > + &efi_mdesc_size,
> > + &mdesc_ver);
> > + if ( EFI_ERROR(status) )
> > + PrintErrMesg(L"Cannot obtain memory map", status);
> > +
> > + /* Try to obtain the ESRT. Errors are not fatal. */
> > + for ( i = 0; i < efi_memmap_size; i += efi_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(efi_memmap + i);
> > +
> > + if ( !esrt_size )
> > + continue;
> > + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type ==
> > + EfiRuntimeServicesData )
> > + break; /* ESRT already safe from reuse */
> > + status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
> > + &new_esrt);
>
> I should have re-raised the earlier voiced concern when reviewing v5 (or
> maybe already v4), and I'm sorry for not having paid close enough
> attention: This may add up to two more entries in the memory map (if an
> entry is split and its middle part is used; of course with an unusual
> implementation there could be even more of a growth). Yet below your
> addition, before obtaining the final memory map, you don't re- obtain
> (and re-increase) the size needed. As to (re-)increase: In fact, prior
> to the allocation you do there shouldn't be a need to bump the space by
> 8 extra entries. That's a safety measure only for possible allocations
> happening across ExitBootServices().
>
> And yes, in earlier versions you had
>
> - info_size += 8 * efi_mdesc_size;
> + info_size += 8 * (efi_mdesc_size + 1);
>
> there, but that's not what would be needed anyway (if trying to avoid
> a 2nd pass of establishing the needed size). Instead in such an event
> you need to bump 8 to 10 (or at least 9, when assuming that normally it
> wouldn't be the middle part of a new range which would be used, but
> rather the leading or trailing one).
>
> While I'd be okay with addressing the two nits above while committing,
> this allocation size aspect first wants settling on. Personally I'd
> prefer the more involved solution, but I'd be okay with merely
> bumping the 8 (plus the addition of a suitable comment, explaining
> the now multiple [two] constituent parts of a seemingly arbitrary
> number). If you want to go this easier route, I guess I could also
> make that adjustment while committing (and adding my R-b).
I would prefer the more involved solution too, but I am not quite sure
how to implement it. Should Xen call GetMemoryMap() in a loop, retrying
as long as it returns EFI_BUFFER_TOO_SMALL? If I do get
EFI_BUFFER_TOO_SMALL, how should I allocate memory for the new buffer?
Should I ask ebmalloc() to provide all remaining memory, and then tell
it how much was actually used?
Once I understand how to allocate the memory for the new memory map, and
where to split the long line mentioned above, I will send a v7.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
On 19.05.2022 16:45, Demi Marie Obenour wrote:
> On Thu, May 19, 2022 at 12:32:33PM +0200, Jan Beulich wrote:
>> On 18.05.2022 19:32, Demi Marie Obenour wrote:
>>> + /*
>>> + * The specification requires EfiBootServicesData, but accept
>>> + * EfiRuntimeServicesData, which is a more logical choice.
>>> + */
>>> + if ( (desc->Type != EfiRuntimeServicesData) &&
>>> + (desc->Type != EfiBootServicesData) )
>>> + return 0;
>>> + available_len = len - (esrt - physical_start);
>>> + if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
>>> + return 0;
>>> + available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
>>> + esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
>>> + if ( esrt_ptr->FwResourceVersion != EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION ||
>>
>> Nit (style): Overlong line.
>
> Where is the best place to split this?
> EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION is a rather long
> identifier.
There's no good place to split; the only possible (imo) place is after
the != .
>>> @@ -1067,6 +1122,46 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>>> if ( !efi_memmap )
>>> blexit(L"Unable to allocate memory for EFI memory map");
>>>
>>> + efi_memmap_size = info_size;
>>
>> I don't think this global needs setting here, yet? The local will
>> do just fine here, likely yielding smaller code. But I realize that's
>> connected to how you did your change vs what I was expecting you to
>> do (see below).
>>
>>> + status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
>>> + efi_memmap, &map_key,
>>> + &efi_mdesc_size,
>>> + &mdesc_ver);
>>> + if ( EFI_ERROR(status) )
>>> + PrintErrMesg(L"Cannot obtain memory map", status);
>>> +
>>> + /* Try to obtain the ESRT. Errors are not fatal. */
>>> + for ( i = 0; i < efi_memmap_size; i += efi_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(efi_memmap + i);
>>> +
>>> + if ( !esrt_size )
>>> + continue;
>>> + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type ==
>>> + EfiRuntimeServicesData )
>>> + break; /* ESRT already safe from reuse */
>>> + status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
>>> + &new_esrt);
>>
>> I should have re-raised the earlier voiced concern when reviewing v5 (or
>> maybe already v4), and I'm sorry for not having paid close enough
>> attention: This may add up to two more entries in the memory map (if an
>> entry is split and its middle part is used; of course with an unusual
>> implementation there could be even more of a growth). Yet below your
>> addition, before obtaining the final memory map, you don't re- obtain
>> (and re-increase) the size needed. As to (re-)increase: In fact, prior
>> to the allocation you do there shouldn't be a need to bump the space by
>> 8 extra entries. That's a safety measure only for possible allocations
>> happening across ExitBootServices().
>>
>> And yes, in earlier versions you had
>>
>> - info_size += 8 * efi_mdesc_size;
>> + info_size += 8 * (efi_mdesc_size + 1);
>>
>> there, but that's not what would be needed anyway (if trying to avoid
>> a 2nd pass of establishing the needed size). Instead in such an event
>> you need to bump 8 to 10 (or at least 9, when assuming that normally it
>> wouldn't be the middle part of a new range which would be used, but
>> rather the leading or trailing one).
>>
>> While I'd be okay with addressing the two nits above while committing,
>> this allocation size aspect first wants settling on. Personally I'd
>> prefer the more involved solution, but I'd be okay with merely
>> bumping the 8 (plus the addition of a suitable comment, explaining
>> the now multiple [two] constituent parts of a seemingly arbitrary
>> number). If you want to go this easier route, I guess I could also
>> make that adjustment while committing (and adding my R-b).
>
> I would prefer the more involved solution too, but I am not quite sure
> how to implement it. Should Xen call GetMemoryMap() in a loop, retrying
> as long as it returns EFI_BUFFER_TOO_SMALL? If I do get
> EFI_BUFFER_TOO_SMALL, how should I allocate memory for the new buffer?
> Should I ask ebmalloc() to provide all remaining memory, and then tell
> it how much was actually used?
Well, there are certainly multiple options. I was thinking that you'd
add a new call to size the memory map, add a few (again 8?) extra
entries there as well for the allocation, and leave the present sizing
call effectively alone (and sitting after all of your additions).
Jan
On Thu, May 19, 2022 at 04:55:10PM +0200, Jan Beulich wrote:
> On 19.05.2022 16:45, Demi Marie Obenour wrote:
> > On Thu, May 19, 2022 at 12:32:33PM +0200, Jan Beulich wrote:
> >> On 18.05.2022 19:32, Demi Marie Obenour wrote:
> >>> + /*
> >>> + * The specification requires EfiBootServicesData, but accept
> >>> + * EfiRuntimeServicesData, which is a more logical choice.
> >>> + */
> >>> + if ( (desc->Type != EfiRuntimeServicesData) &&
> >>> + (desc->Type != EfiBootServicesData) )
> >>> + return 0;
> >>> + available_len = len - (esrt - physical_start);
> >>> + if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
> >>> + return 0;
> >>> + available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
> >>> + esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
> >>> + if ( esrt_ptr->FwResourceVersion != EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION ||
> >>
> >> Nit (style): Overlong line.
> >
> > Where is the best place to split this?
> > EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION is a rather long
> > identifier.
>
> There's no good place to split; the only possible (imo) place is after
> the != .
Will do in v7, along with parentheses to avoid any visual confusion.
> >>> @@ -1067,6 +1122,46 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> >>> if ( !efi_memmap )
> >>> blexit(L"Unable to allocate memory for EFI memory map");
> >>>
> >>> + efi_memmap_size = info_size;
> >>
> >> I don't think this global needs setting here, yet? The local will
> >> do just fine here, likely yielding smaller code. But I realize that's
> >> connected to how you did your change vs what I was expecting you to
> >> do (see below).
> >>
> >>> + status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
> >>> + efi_memmap, &map_key,
> >>> + &efi_mdesc_size,
> >>> + &mdesc_ver);
> >>> + if ( EFI_ERROR(status) )
> >>> + PrintErrMesg(L"Cannot obtain memory map", status);
> >>> +
> >>> + /* Try to obtain the ESRT. Errors are not fatal. */
> >>> + for ( i = 0; i < efi_memmap_size; i += efi_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(efi_memmap + i);
> >>> +
> >>> + if ( !esrt_size )
> >>> + continue;
> >>> + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type ==
> >>> + EfiRuntimeServicesData )
> >>> + break; /* ESRT already safe from reuse */
> >>> + status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
> >>> + &new_esrt);
> >>
> >> I should have re-raised the earlier voiced concern when reviewing v5 (or
> >> maybe already v4), and I'm sorry for not having paid close enough
> >> attention: This may add up to two more entries in the memory map (if an
> >> entry is split and its middle part is used; of course with an unusual
> >> implementation there could be even more of a growth). Yet below your
> >> addition, before obtaining the final memory map, you don't re- obtain
> >> (and re-increase) the size needed. As to (re-)increase: In fact, prior
> >> to the allocation you do there shouldn't be a need to bump the space by
> >> 8 extra entries. That's a safety measure only for possible allocations
> >> happening across ExitBootServices().
> >>
> >> And yes, in earlier versions you had
> >>
> >> - info_size += 8 * efi_mdesc_size;
> >> + info_size += 8 * (efi_mdesc_size + 1);
> >>
> >> there, but that's not what would be needed anyway (if trying to avoid
> >> a 2nd pass of establishing the needed size). Instead in such an event
> >> you need to bump 8 to 10 (or at least 9, when assuming that normally it
> >> wouldn't be the middle part of a new range which would be used, but
> >> rather the leading or trailing one).
> >>
> >> While I'd be okay with addressing the two nits above while committing,
> >> this allocation size aspect first wants settling on. Personally I'd
> >> prefer the more involved solution, but I'd be okay with merely
> >> bumping the 8 (plus the addition of a suitable comment, explaining
> >> the now multiple [two] constituent parts of a seemingly arbitrary
> >> number). If you want to go this easier route, I guess I could also
> >> make that adjustment while committing (and adding my R-b).
> >
> > I would prefer the more involved solution too, but I am not quite sure
> > how to implement it. Should Xen call GetMemoryMap() in a loop, retrying
> > as long as it returns EFI_BUFFER_TOO_SMALL? If I do get
> > EFI_BUFFER_TOO_SMALL, how should I allocate memory for the new buffer?
> > Should I ask ebmalloc() to provide all remaining memory, and then tell
> > it how much was actually used?
>
> Well, there are certainly multiple options. I was thinking that you'd
> add a new call to size the memory map, add a few (again 8?) extra
> entries there as well for the allocation, and leave the present sizing
> call effectively alone (and sitting after all of your additions).
How should I allocate memory for the new memory map? Getting the size
is easy; allocating the memory is the tricky part. That’s where the
idea of calling AllocatePool() and GetMemoryMap() in a loop came from.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
On 19.05.2022 17:09, Demi Marie Obenour wrote:
> On Thu, May 19, 2022 at 04:55:10PM +0200, Jan Beulich wrote:
>> On 19.05.2022 16:45, Demi Marie Obenour wrote:
>>> On Thu, May 19, 2022 at 12:32:33PM +0200, Jan Beulich wrote:
>>>> On 18.05.2022 19:32, Demi Marie Obenour wrote:
>>>>> + /*
>>>>> + * The specification requires EfiBootServicesData, but accept
>>>>> + * EfiRuntimeServicesData, which is a more logical choice.
>>>>> + */
>>>>> + if ( (desc->Type != EfiRuntimeServicesData) &&
>>>>> + (desc->Type != EfiBootServicesData) )
>>>>> + return 0;
>>>>> + available_len = len - (esrt - physical_start);
>>>>> + if ( available_len <= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries) )
>>>>> + return 0;
>>>>> + available_len -= offsetof(EFI_SYSTEM_RESOURCE_TABLE, Entries);
>>>>> + esrt_ptr = (const EFI_SYSTEM_RESOURCE_TABLE *)esrt;
>>>>> + if ( esrt_ptr->FwResourceVersion != EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION ||
>>>>
>>>> Nit (style): Overlong line.
>>>
>>> Where is the best place to split this?
>>> EFI_SYSTEM_RESOURCE_TABLE_FIRMWARE_RESOURCE_VERSION is a rather long
>>> identifier.
>>
>> There's no good place to split; the only possible (imo) place is after
>> the != .
>
> Will do in v7, along with parentheses to avoid any visual confusion.
>
>>>>> @@ -1067,6 +1122,46 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
>>>>> if ( !efi_memmap )
>>>>> blexit(L"Unable to allocate memory for EFI memory map");
>>>>>
>>>>> + efi_memmap_size = info_size;
>>>>
>>>> I don't think this global needs setting here, yet? The local will
>>>> do just fine here, likely yielding smaller code. But I realize that's
>>>> connected to how you did your change vs what I was expecting you to
>>>> do (see below).
>>>>
>>>>> + status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
>>>>> + efi_memmap, &map_key,
>>>>> + &efi_mdesc_size,
>>>>> + &mdesc_ver);
>>>>> + if ( EFI_ERROR(status) )
>>>>> + PrintErrMesg(L"Cannot obtain memory map", status);
>>>>> +
>>>>> + /* Try to obtain the ESRT. Errors are not fatal. */
>>>>> + for ( i = 0; i < efi_memmap_size; i += efi_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(efi_memmap + i);
>>>>> +
>>>>> + if ( !esrt_size )
>>>>> + continue;
>>>>> + if ( ((EFI_MEMORY_DESCRIPTOR *)(efi_memmap + i))->Type ==
>>>>> + EfiRuntimeServicesData )
>>>>> + break; /* ESRT already safe from reuse */
>>>>> + status = efi_bs->AllocatePool(EfiRuntimeServicesData, esrt_size,
>>>>> + &new_esrt);
>>>>
>>>> I should have re-raised the earlier voiced concern when reviewing v5 (or
>>>> maybe already v4), and I'm sorry for not having paid close enough
>>>> attention: This may add up to two more entries in the memory map (if an
>>>> entry is split and its middle part is used; of course with an unusual
>>>> implementation there could be even more of a growth). Yet below your
>>>> addition, before obtaining the final memory map, you don't re- obtain
>>>> (and re-increase) the size needed. As to (re-)increase: In fact, prior
>>>> to the allocation you do there shouldn't be a need to bump the space by
>>>> 8 extra entries. That's a safety measure only for possible allocations
>>>> happening across ExitBootServices().
>>>>
>>>> And yes, in earlier versions you had
>>>>
>>>> - info_size += 8 * efi_mdesc_size;
>>>> + info_size += 8 * (efi_mdesc_size + 1);
>>>>
>>>> there, but that's not what would be needed anyway (if trying to avoid
>>>> a 2nd pass of establishing the needed size). Instead in such an event
>>>> you need to bump 8 to 10 (or at least 9, when assuming that normally it
>>>> wouldn't be the middle part of a new range which would be used, but
>>>> rather the leading or trailing one).
>>>>
>>>> While I'd be okay with addressing the two nits above while committing,
>>>> this allocation size aspect first wants settling on. Personally I'd
>>>> prefer the more involved solution, but I'd be okay with merely
>>>> bumping the 8 (plus the addition of a suitable comment, explaining
>>>> the now multiple [two] constituent parts of a seemingly arbitrary
>>>> number). If you want to go this easier route, I guess I could also
>>>> make that adjustment while committing (and adding my R-b).
>>>
>>> I would prefer the more involved solution too, but I am not quite sure
>>> how to implement it. Should Xen call GetMemoryMap() in a loop, retrying
>>> as long as it returns EFI_BUFFER_TOO_SMALL? If I do get
>>> EFI_BUFFER_TOO_SMALL, how should I allocate memory for the new buffer?
>>> Should I ask ebmalloc() to provide all remaining memory, and then tell
>>> it how much was actually used?
>>
>> Well, there are certainly multiple options. I was thinking that you'd
>> add a new call to size the memory map, add a few (again 8?) extra
>> entries there as well for the allocation, and leave the present sizing
>> call effectively alone (and sitting after all of your additions).
>
> How should I allocate memory for the new memory map? Getting the size
> is easy; allocating the memory is the tricky part. That’s where the
> idea of calling AllocatePool() and GetMemoryMap() in a loop came from.
Just like it's done now. GetMemoryMap(), AllocatePool(), GetMemoryMap().
I don't think you _need_ any loop for that, but you may well code it
that way if that looks neater to you. All I'm asking for is that you
leave the existing code largely undisturbed, which - as a consequence -
means that I don't think the logic strictly needs to live in
efi_exit_boot() (iirc I did hint at that before). But of course we
want to avoid relocating the blob and then exit for some trivial error
(like the kernel image not being readable), so it can't be done
arbitrarily early.
Jan
© 2016 - 2024 Red Hat, Inc.