[PATCH 06/13] efi_loader: Use the enum for memory type

[Simon Glass]

Hi Mark,

On Wed, 27 Nov 2024 at 06:23, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > From: Simon Glass <sjg at chromium.org>
> > Date: Wed, 27 Nov 2024 06:07:05 -0700
> >
> > Hi Heinrich,
> >
> > On Tue, 26 Nov 2024 at 08:37, Simon Glass <sjg at chromium.org> wrote:
> > >
> > > Hi Heinrich,
> > >
> > > On Tue, 26 Nov 2024 at 02:44, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > >
> > > > On 25.11.24 21:44, Simon Glass wrote:
> > > > > Rather than an integer, it is better to use the enum provided, when
> > > > > referring to an EFI memory-type. Update existing uses.
> > > >
> > > > The C standard provides no definition of the size of the integer used to
> > > > implement enums. It could use u8, u16, u32, or u64.
> > > >
> > > > As EFI applications may be compiled using a different compiler we need
> > > > to control the width used for passing parameters in the API interface.
> > > >
> > > > We should have used u32 instead of int here. We could use a typedef for
> > > > more verbosity though that is frowned upon in U-Boot.
> > >
> > > Yes, the ext function is debatable. The rest seem OK to me, though, as
> > > they are internal functions, See also [1].
> >
> > Hmm, but the spec uses enum, so arguably this patch is more correct?
>
> Only as a way to define the constants.  The actual interfaces that use
> the constants use UINT32, which would indeed be u32 in Linux land as
> Heinrich suggests.
>
> So no, the patch isn't correct.

Just to add a bit more detail...

I see the spec says this: "Element of a standard ANSI C enum type
declaration. Type INT32.or UINT32. ANSI C does
not define the size of sign of an enum so they should never be used in
structures. ANSI C
integer promotion rules make INT32 or UINT32 interchangeable when passed as an
argument to a function."

The compilers we use (at least gcc and clang) use int as the enum
type, and int is 32 bits on 32/64-bit machines.

So it looks safe to me. But since the spec goes out of its way to
mention this point, it seems best to keep the external interface using
int and just use the enum within the implementation code. That's what
I did on v2 onwards.

>
> > > >
> > > > Best regards
> > > >
> > > > Heinrich
> > > >
> > > > >
> > > > > Call the value 'mem_type' consistently. Fix up one instance of
> > > > > upper-case hex.
> > > > >
> > > > > Fix up the calls in struct efi_boot_services so that they use the same
> > > > > enum, adding the missing parameter names and enum efi_allocate_type.
> > > > >
> > > > > While we are here, rename the 'memory' parameter to 'memoryp' so that it
> > > > > is clear it is a return value.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > ---
> > > > >
> > > > >   include/efi_api.h                |  6 ++++--
> > > > >   include/efi_loader.h             | 28 ++++++++++++++-------------
> > > > >   lib/efi_loader/efi_boottime.c    | 13 +++++++------
> > > > >   lib/efi_loader/efi_device_path.c |  4 ++--
> > > > >   lib/efi_loader/efi_memory.c      | 33 ++++++++++++++++----------------
> > > > >   5 files changed, 45 insertions(+), 39 deletions(-)
> > > > >
> > >
> > > Regards,
> > > SImon
> > >
> > > [1] https://stackoverflow.com/questions/4879286/specifying-size-of-enum-type-in-c/4879350#4879350

Regards,
Simon