[PATCH] efi_loader: Avoid emitting efi_var_buf to .GOT

[Ilias Apalodimas]

Hi Heinrich, 

[...]
> > Atish can you give it a spin and let me know if this fixes the issue for you?
> > The objdump seems to be correct now, but I am not familiar with RISC-V.
> > No regressions on Arm with TEE or memory backed variables.
> >  include/efi_variable.h            | 12 ++++++++++++
> >  lib/efi_loader/efi_var_mem.c      | 12 +++++++++++-
> >  lib/efi_loader/efi_variable_tee.c |  2 +-
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/efi_variable.h b/include/efi_variable.h
> > index 4704a3c16e65..b2317eb7bf1c 100644
> > --- a/include/efi_variable.h
> > +++ b/include/efi_variable.h
> > @@ -306,4 +306,16 @@ efi_status_t __efi_runtime EFIAPI
> >  efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> >  				   u16 *variable_name, efi_guid_t *guid);
> >
> > +/**
> > + * efi_var_buf_update() - Update the value of efi_var_buf in efi_var_mem.c
> 
> Dear Ilias,
> 
> thank you for addressing this problem. The code looks fine to me. Just
> some ideas concerning comment lines:

Well, I broke it to begin with so ...

> 
> The value of efi_var_buf is the address it is pointing to. So i would
> prefer:
> 
> efi_var_buf_update() - udpate memory buffer for variables
> 
> > + *
> > + * @var_buf:	Source buffer
> 
> %s/Source/source/
> 

Ok

> > + *
> > + * efi_var_buf is special since we use it on Runtime Services. We need
> > + * to keep it static in efi_var_mem.c and avoid having it pulled into
> > + * .GOT. Since it has to be static this function must be used to update
> 
> You already place a comment about .GOT where the declaration is. Here
> describing how the function is used would be of interest. E.g.
> 
> "This function copies to the memory buffer for UEFI variables. Call this
> function in ExitBootServices() if memory backed variables are only used
> at runtime to fill the buffer."
> 

I'll replace it. Guess I was too concerned pointing out "Don't touch efi_var_buf"

> > + * it
> > + */
> > +void efi_var_buf_update(struct efi_var_file *var_buf);
> > +
> >  #endif
> > diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> > index d155f25f60e6..fcf0043b5d3b 100644
> > --- a/lib/efi_loader/efi_var_mem.c
> > +++ b/lib/efi_loader/efi_var_mem.c
> > @@ -10,7 +10,12 @@
> >  #include <efi_variable.h>
> >  #include <u-boot/crc.h>
> >
> > -struct efi_var_file __efi_runtime_data *efi_var_buf;
> > +/*
> > + * keep efi_var_buf as static , moving it out might move it to .got
> > + * which is not mapped in virtual address for Linux. Whenever
> > + * we try to invoke get_variable service, it will panic.
> 
> Not everybody will know the abbreviation .got. How about:
> 
> "The variables efi_var_file and efi_var_entry must be static to avoid
> that they are referenced via the global offset table (section .got). The
> GOT is neither mapped as EfiRuntimeServicesData nor do we support its
> relocation during SetVirtualAddressMap()."
> 

Sure

> Otherwise

I'll wait for Atish to verify it fixes RISC-V, because it makes no difference
whatsoever in arm and send a v2. 

Thanks
/Ilias
> 
> Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

[...]