[PATCH v2 1/1] efi_loader: architecture specific UEFI setup

[Daniel Kiper]

On Wed, Feb 05, 2020 at 12:37:03PM +0100, Heinrich Schuchardt wrote:
> Hello Daniel, hello Leif,
>
> what is the GRUB view on this discussion?

Alex, could you chime in on this as a GRUB RISC-V maintainer?

Daniel

> Best regards
>
> Heinrich
>
> On 2/5/20 12:32 PM, Heinrich Schuchardt wrote:
> > On 2/5/20 8:43 AM, Ard Biesheuvel wrote:
> > > On Wed, 5 Feb 2020 at 05:53, Heinrich Schuchardt <xypron.glpk at gmx.de>
> > > wrote:
> > > >
> > > > RISC-V booting currently is based on a per boot stage lottery to
> > > > determine
> > > > the active CPU. The Hart State Management (HSM) SBI extension replaces
> > > > this lottery by using a dedicated primary CPU.
> > > >
> > > > Cf. Hart State Management Extension, Extension ID: 0x48534D (HSM)
> > > > https://github.com/riscv/riscv-sbi-doc/blob/master/riscv-sbi.adoc
> > > >
> > > > In this scenario the id of the active hart has to be passed from boot
> > > > stage
> > > > to boot stage. Using a UEFI variable would provide an easy
> > > > implementation.
> > > >
> > > > This patch provides a weak function that is called at the end of the
> > > > setup
> > > > of U-Boot's UEFI sub-system. By overriding this function architecture
> > > > specific UEFI variables or configuration tables can be created.
> > > >
> > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> > > > Reviewed-by: Atish Patra <atish.patra at wdc.com>
> > >
> > > OK, so I have a couple of questions:
> > >
> > > - does RISC-V use device tree? if so, why are you not passing the
> >
> > In the Linux kernel tree you can find the SiFive HiFive Unleashed device
> > tree: arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> >
> > Some of the QEMU emulated RISC-V boards provide device trees, cf.
> > https://github.com/riscv/riscv-qemu/wiki#machines
> >
> > > active hart via a property in the /chosen node? I'd assume the EFI
> >
> > There is a hart (core) that calls the entry point of the next
> > boot-stage. Could this define the active hart?
> >
> > Best regards
> >
> > Heinrich
> >
> > > stub would not care at all about this information, and it would give
> > > you a Linux/RISC-V specific way to convey this information that is
> > > independent of EFI.
> > > - using variables to pass information from firmware to OS only is
> > > overkill, and config tables are preferred, given that they only
> > > require access to the system table. If required, a RISC-V specific
> > > data structure containing boot parameters could be installed as a
> > > configuration table, and the address passed to the startup code in the
> > > kernel proper [rather than just a hart id], allowing you to put any
> > > piece of information you like in there.
> > >
> > > Config tables work fine with kexec, btw. It is up to the first OS to
> > > memblock_reserve() the table to guarantee that it is still there at
> > > kexec time, but this applies equally to all other data structures
> > > passed as config tables. Alternatively, in this case, you can
> > > stipulate that it is passed as AcpiReclaim [ignore the 'Acpi' in the
> > > name] which is intended for firmware tables (and we never reclaim it
> > > in linux)
> > >
> > > I'd also recommend that RISC-V adopt the same principle as ARM does
> > > when it comes to EFI: call SetVirtualAddressMap in the stub, so that
> > > the kernel proper always sees the same handover state, regardless of
> > > kexec. Additionally, you shouldn't ever modify the EFI memory map
> > > provided by the firmware, so that the kexec kernel sees the exact same
> > > version.
> > >
> > >
> > >
> > >
> > > > ---
> > > > v2:
> > > > Â Â Â Â Â Â Â Â  reference the Hart State Management Extension in the commit
> > > > message
> > > > ---
> > > >   include/efi_loader.h       |  3 +++
> > > > Â  lib/efi_loader/efi_setup.c | 16 ++++++++++++++++
> > > > Â  2 files changed, 19 insertions(+)
> > > >
> > > > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > > > index d4c59b54c4..d87de85e83 100644
> > > > --- a/include/efi_loader.h
> > > > +++ b/include/efi_loader.h
> > > > @@ -116,6 +116,9 @@ extern efi_uintn_t efi_memory_map_key;
> > > > Â  extern struct efi_runtime_services efi_runtime_services;
> > > > Â  extern struct efi_system_table systab;
> > > >
> > > > +/* Architecture specific initialization of the UEFI system */
> > > > +efi_status_t efi_setup_arch_specific(void);
> > > > +
> > > > Â  extern struct efi_simple_text_output_protocol efi_con_out;
> > > > Â  extern struct efi_simple_text_input_protocol efi_con_in;
> > > > Â  extern struct efi_console_control_protocol efi_console_control;
> > > > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > > > index de7b616c6d..8469f0f43c 100644
> > > > --- a/lib/efi_loader/efi_setup.c
> > > > +++ b/lib/efi_loader/efi_setup.c
> > > > @@ -22,6 +22,17 @@ void __weak allow_unaligned(void)
> > > > Â  {
> > > > Â  }
> > > >
> > > > +/**
> > > > + * efi_setup_arch_specific() - architecture specific UEFI setup
> > > > + *
> > > > + * This routine can be used to define architecture specific variables
> > > > + * or configuration tables, e.g. HART id for RISC-V
> > > > + */
> > > > +efi_status_t __weak efi_setup_arch_specific(void)
> > > > +{
> > > > +Â Â Â Â Â Â  return EFI_SUCCESS;
> > > > +}
> > > > +
> > > > Â  /**
> > > > Â Â  * efi_init_platform_lang() - define supported languages
> > > > Â Â  *
> > > > @@ -179,6 +190,11 @@ efi_status_t efi_init_obj_list(void)
> > > > Â Â Â Â Â Â Â Â  if (ret != EFI_SUCCESS)
> > > > Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  goto out;
> > > >
> > > > +Â Â Â Â Â Â  /* Architecture specific setup */
> > > > +Â Â Â Â Â Â  ret = efi_setup_arch_specific();
> > > > +Â Â Â Â Â Â  if (ret != EFI_SUCCESS)
> > > > +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  goto out;
> > > > +
> > > > Â  out:
> > > > Â Â Â Â Â Â Â Â  efi_obj_list_initialized = ret;
> > > > Â Â Â Â Â Â Â Â  return ret;
> > > > --
> > > > 2.24.1