[PATCH v2 01/28] efi: Define fields in struct efi_mem_desc

[Simon Glass]

Hi Heinrich,

On Thu, 28 Nov 2024 at 10:04, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 28.11.24 16:47, Simon Glass wrote:
> > There is quite a bit of confusion in the EFI code as to whether a field
> > contains an address or a pointer. As a first step towards resolving
> > this, document the memory-descriptor struct, indicating that it holds
> > pointers, not addresses.
> >
> > Dro the same for efi_add_memory_map() as it is widely used, as well as
> > efi_add_memory_map_pg() which is only used by lmb
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v2:
> > - Fix missing @
> > - Note that this holds pointers, not addresses
> >
> >   include/efi.h               | 15 +++++++++++++++
> >   include/efi_loader.h        |  6 ++++--
> >   lib/efi_loader/efi_memory.c |  4 +++-
> >   3 files changed, 22 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/efi.h b/include/efi.h
> > index c559fda3004..3fce281233c 100644
> > --- a/include/efi.h
> > +++ b/include/efi.h
> > @@ -273,6 +273,21 @@ enum efi_memory_type {
> >   #define EFI_PAGE_SIZE               (1ULL << EFI_PAGE_SHIFT)
> >   #define EFI_PAGE_MASK               (EFI_PAGE_SIZE - 1)
> >
> > +/**
> > + * struct efi_mem_desc - defines an EFI memory record
>
> Please, add here that this structure implements the
> EFI_MEMORY_DESCRIPTOR type of the UEFI specification.
>
> > + *
> > + * Note that this struct is for use outside U-Boot, so the two 'start' fields
> > + * are pointers, not addresses. Use map_to_sysmem() to convert to an address, so
> > + * that sandbox operates correctly.
> > + *
> > + * @type (enum efi_memory_type): EFI memory-type
> > + * @reserved: unused
> > + * @physical_start: Start address of region in physical memory
> > + * @virtual_start: Start address of region in physical memory
>
> %s/virtual memory/
>
> where both addresses will always be the same before
> SetVirtualMemoryMap() as the UEFI specification requires identity mapping.
>
> > + * @num_pages: Number of EFI pages this record covers (each is EFI_PAGE_SIZE
> > + *   bytes)
> > + * @attribute: Memory attributes (see EFI_MEMORY_...)
> > + */
> >   struct efi_mem_desc {
> >       u32 type;
> >       u32 reserved;
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 39809eac1bc..ee0cdd36500 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -788,8 +788,10 @@ efi_status_t efi_add_memory_map(u64 start, u64 size, int memory_type);
> >   /**
> >    * efi_add_memory_map_pg() - add pages to the memory map
> >    *
> > - * @start:                   start address, must be a multiple of
> > - *                           EFI_PAGE_SIZE
> > + * @start: start address, must be a multiple of EFI_PAGE_SIZE. Note that this
> > + * is actually a pointer provided as an integer. To pass in an address, pass
>
> %s/an address/a sandbox virtual address/
>
> > + * in (ulong)map_to_sysmem(addr)
> > + *
> >    * @pages:                  number of pages to add
> >    * @memory_type:            type of memory added
> >    * @overlap_conventional:   region may only overlap free(conventional)
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index e493934c713..f1154f73e05 100644
> > --- a/lib/efi_loader/efi_memory.c
> > +++ b/lib/efi_loader/efi_memory.c
> > @@ -384,7 +384,9 @@ efi_status_t efi_add_memory_map_pg(u64 start, u64 pages,
> >   /**
> >    * efi_add_memory_map() - add memory area to the memory map
> >    *
> > - * @start:           start address of the memory area
> > + * @start:           start address of the memory area. Note that this is
> > + *                   actually a pointer provided as an integer. To pass in
> > + *                   an address, pass in (ulong)map_to_sysmem(addr)
>
> %s/an address/a sandbox virtual address/

Sorry, no I don't want to introduce that terminology.
>
> >    * @size:           length in bytes of the memory area
> >    * @memory_type:    type of memory added
> >    *
>

Regards,
Simon