[PATCH v8 12/25] efi: Move exit_boot_services into a function
Heinrich Schuchardt
xypron.glpk at gmx.de
Wed Jan 5 09:01:12 CET 2022
On 1/4/22 11:52, Simon Glass wrote:
> Hi Heinrich,
>
> On Thu, 30 Dec 2021 at 22:41, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> On 12/29/21 19:57, Simon Glass wrote:
>>> At present this code is inline in the app and stub. But they do the same
>>> thing. The difference is that the stub does it immediately and the app
>>> doesn't want to do it until the end (when it boots a kernel) or not at
>>> all, if returning to UEFI.
>>>
>>> Move it into a function so it can be called as needed.
>>>
>>> Also store the memory map so that it can be accessed within the app if
>>> needed.
>>
>> The memory map is *not* a static object. It may change with any API call
>> that you make. You must read the memory map immediately before calling
>> ExitBootServices(). The valid value of MapKey typically will change with
>> any change of the memory map. Calling ExitBootServices() with the wrong
>> value of MapKey will lead to a failure. Storing these values except for
>> immediate use makes no sense.
>>
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>> (no changes since v6)
>>>
>>> Changes in v6:
>>> - Fix typo in function comment
>>>
>>> Changes in v2:
>>> - Add a sentence about what the patch does
>>>
>>> include/efi.h | 32 ++++++++++++++++++++++
>>> lib/efi/efi.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++
>>> lib/efi/efi_app.c | 3 ++
>>> lib/efi/efi_stub.c | 66 ++++++++------------------------------------
>>> 4 files changed, 114 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/include/efi.h b/include/efi.h
>>> index d4785478585..2a84223d235 100644
>>> --- a/include/efi.h
>>> +++ b/include/efi.h
>>> @@ -407,6 +407,12 @@ static inline struct efi_mem_desc *efi_get_next_mem_desc(
>>> * @sys_table: Pointer to system table
>>> * @boot: Pointer to boot-services table
>>> * @run: Pointer to runtime-services table
>>> + * @memmap_key: Key returned from get_memory_map()
>>> + * @memmap_desc: List of memory-map records
>>> + * @memmap_alloc: Amount of memory allocated for memory map list
>>> + * @memmap_size Size of memory-map list in bytes
>>> + * @memmap_desc_size: Size of an individual memory-map record, in bytes
>>> + * @memmap_version: Memory-map version
>>> *
>>> * @use_pool_for_malloc: true if all allocation should go through the EFI 'pool'
>>> * methods allocate_pool() and free_pool(); false to use 'pages' methods
>>> @@ -424,6 +430,12 @@ struct efi_priv {
>>> struct efi_system_table *sys_table;
>>> struct efi_boot_services *boot;
>>> struct efi_runtime_services *run;
>>> + efi_uintn_t memmap_key;
>>> + struct efi_mem_desc *memmap_desc;
>>> + efi_uintn_t memmap_alloc;
>>> + efi_uintn_t memmap_size;
>>> + efi_uintn_t memmap_desc_size;
>>> + u32 memmap_version;
>>>
>>> /* app: */
>>> bool use_pool_for_malloc;
>>> @@ -574,4 +586,24 @@ void efi_putc(struct efi_priv *priv, const char ch);
>>> */
>>> int efi_info_get(enum efi_entry_t type, void **datap, int *sizep);
>>>
>>> +/**
>>> + * efi_store_memory_map() - Collect the memory-map info from EFI
>>> + *
>>> + * Collect the memory info and store it for later use, e.g. in calling
>>> + * exit_boot_services()
>>> + *
>>> + * @priv: Pointer to private EFI structure
>>> + * @return 0 if OK, non-zero on error
>>> + */
>>> +int efi_store_memory_map(struct efi_priv *priv);
>>> +
>>> +/**
>>> + * efi_call_exit_boot_services() - Handle the exit-boot-service procedure
>>> + *
>>> + * Tell EFI we don't want their boot services anymore
>>> + *
>>> + * Return: 0 if OK, non-zero on error
>>> + */
>>> +int efi_call_exit_boot_services(void);
>>> +
>>> #endif /* _LINUX_EFI_H */
>>> diff --git a/lib/efi/efi.c b/lib/efi/efi.c
>>> index cd6bf47b180..20da88c9151 100644
>>> --- a/lib/efi/efi.c
>>> +++ b/lib/efi/efi.c
>>> @@ -135,3 +135,71 @@ void efi_free(struct efi_priv *priv, void *ptr)
>>>
>>> boot->free_pool(ptr);
>>> }
>>> +
>>> +int efi_store_memory_map(struct efi_priv *priv)
>>> +{
>>> + struct efi_boot_services *boot = priv->sys_table->boottime;
>>> + efi_uintn_t size, desc_size;
>>> + efi_status_t ret;
>>> +
>>> + /* Get the memory map so we can switch off EFI */
>>> + size = 0;
>>> + ret = boot->get_memory_map(&size, NULL, &priv->memmap_key,
>>> + &priv->memmap_desc_size,
>>> + &priv->memmap_version);
>>> + if (ret != EFI_BUFFER_TOO_SMALL) {
>>> + printhex2(EFI_BITS_PER_LONG);
>>> + putc(' ');
>>> + printhex2(ret);
>>> + puts(" No memory map\n");
>>> + return ret;
>>> + }
>>> + /*
>>> + * Since doing a malloc() may change the memory map and also we want to
>>> + * be able to read the memory map in efi_call_exit_boot_services()
>>> + * below, after more changes have happened
>>> + */
>>> + priv->memmap_alloc = size + 1024;
>>
>> GetMemoryMap() must be called immediately before calling ExitBootServices().
>
> Yes that's right. I remember reading that in the spec.
>
>>
>> If efi_malloc() invokes AllocatePages() or AllocatePohl(), you should
>> add DescriptorSize to MemoryMapSize and not some random value (e.g. 1024).
>
> This is copying existing code. If you want to change this, we can do
> it in a follow-on patch.
>
> For that discussion, How do you know it will be increased by
> sizeof(DescriptorSize) ? Is that in the spec somewhere?
>
> In one run of the stub with qemu it returned 0x1170 for the first call
> and only 0x1050 for the second. In a run of the app, I got 11d0 and
> then 10b0.
>
> Is it possible that some EFI implementations allow a margin already?
>
> Otherwise, what do you suggest?
The UEFI 2.9 spec states:
"The actual size of the buffer allocated for the consequent call to
GetMemoryMap() should be bigger then the value returned in
MemoryMapSize, since allocation of the new buffer may potentially
increase memory map size."
The additional size needed will be a multiple of DescriptorSize. In
U-Boot carving out a memory area will create exactly create one extra
descriptor. But the UEFI spec gives no guarantee for this.
EDK II uses the following algorithm in multiple places.
MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c:147
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c:788
MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskProtocol.c:163
MdePkg/Library/SmmMemLib/SmmMemLib.c:485
Status = gBS->GetMemoryMap (
&MemoryMapSize,
MemoryMap,
&MapKey,
&DescriptorSize,
&DescriptorVersion
);
ASSERT (Status == EFI_BUFFER_TOO_SMALL);
do {
MemoryMap = (EFI_MEMORY_DESCRIPTOR *) AllocatePool (MemoryMapSize);
ASSERT (MemoryMap != NULL);
Status = gBS->GetMemoryMap (
&MemoryMapSize,
MemoryMap,
&MapKey,
&DescriptorSize,
&DescriptorVersion
);
if (EFI_ERROR (Status)) {
FreePool (MemoryMap);
}
} while (Status == EFI_BUFFER_TOO_SMALL);
ASSERT_EFI_ERROR (Status);
This algorithm will only work if FreePool() reverts the memory map
changes of AllocatePool() which might not be the case for simplistic
implementations.
In the UEFI SCT I found:
uefi-sct/SctPkg/SCRT/SCRTApp/SCRTApp.c-307-
MemoryMapSize += 1024;
uefi-sct/SctPkg/TestCase/UEFI/EFI/BootServices/MemoryAllocationServices/BlackBoxTest/MemoryAllocationServicesBBTestMain.c-248
MemoryMapSizeNeeded += EFI_PAGE_SIZE;
So let's keep your code as it is.
Best regards
Heinrich
>
>>
>>> + priv->memmap_size = priv->memmap_alloc;
>>> + priv->memmap_desc = efi_malloc(priv, size, &ret);
>>> + if (!priv->memmap_desc) {
>>> + printhex2(ret);
>>> + puts(" No memory for memory descriptor\n");
>>> + return ret;
>>> + }
>>> +
>>> + ret = boot->get_memory_map(&priv->memmap_size, priv->memmap_desc,
>>> + &priv->memmap_key, &desc_size,
>>> + &priv->memmap_version);
>>> + if (ret) {
>>> + printhex2(ret);
>>> + puts(" Can't get memory map\n");
>>> + return ret;
>>
>> Please, use printf().
>
> It is not available in the stub. Even puts() is only available because
> there is a local version in efi_stub.c. I'll add a comment.
>
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +int efi_call_exit_boot_services(void)
>>> +{
>>> + struct efi_priv *priv = efi_get_priv();
>>> + const struct efi_boot_services *boot = priv->boot;
>>> + efi_uintn_t size;
>>> + u32 version;
>>> + efi_status_t ret;
>>> +
>>> + size = priv->memmap_alloc;
>>> + ret = boot->get_memory_map(&size, priv->memmap_desc,
>>> + &priv->memmap_key,
>>> + &priv->memmap_desc_size, &version);
>>> + if (ret) {
>>> + printhex2(ret);
>>> + puts(" Can't get memory map\n");
>>> + return ret;
>>
>> Please, use printf().
>
> See above.
>
> Regards,
> Simon
More information about the U-Boot
mailing list