[PATCH v8 12/25] efi: Move exit_boot_services into a function

[Simon Glass]

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?

>
> > +     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