Re: [PATCH v2 13/21] efi/libstub/x86: drop __efi_early() export of efi_config struct

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 1 Jan 2020 at 00:04, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Wed, Dec 18, 2019 at 07:01:31PM +0200, Ard Biesheuvel wrote:
> > The various pointers we stash in the efi_config struct which we
> > retrieve using __efi_early() are simply copies of the ones in
> > the EFI system table, which we have started accessing directly
> > in the previous patch. So drop all the __efi_early() related
> > plumbing, except for the access to a boolean which tells us
> > whether the firmware is 64-bit or not.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/x86/boot/compressed/eboot.c   | 36 ++++++++------------
> >  arch/x86/boot/compressed/head_32.S |  2 +-
> >  arch/x86/boot/compressed/head_64.S |  4 +--
> >  arch/x86/include/asm/efi.h         | 23 +++++--------
> >  4 files changed, 26 insertions(+), 39 deletions(-)
> >
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index 2bcab1ef5a56..53e67334c4c3 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -20,11 +20,17 @@
> >
> >  static efi_system_table_t *sys_table;
> >
> > -static struct efi_config *efi_early;
> > +struct efi_config {
> > +     u64 image_handle;
> > +     u64 table;
> > +     bool is64;
> > +} __packed;
> >
> > -__pure const struct efi_config *__efi_early(void)
> > +static bool is64;
> > +
> > +__pure bool __efi_early_is64(void)
> >  {
> > -     return efi_early;
> > +     return is64;
> >  }
> >
> >  __pure efi_system_table_t *efi_system_table(void)
>
> This predates your series, but looking at the generated code I noticed
> that pure isn't enough to let gcc optimize out repeated calls to is64
> and system_table. Declaring them as __attribute_const__ would allow
> optimization. They don't quite meet gcc's requirements for that as they
> access global non-const variables, but it seems like it ought to be
> safe, especially if we stop using the functions within eboot.c itself?

The GCC documentation mentions that it does not make sense for a
function annotated as const not to take any arguments, so I'd rather
avoid it here.