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

[Atish Patra]

On Tue, Feb 4, 2020 at 11:43 PM Ard Biesheuvel
<ard.biesheuvel at linaro.org> 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
> active hart via a property in the /chosen node?

Yes. RISC-V uses device tree. Technically, we can pass the active hart
by a DT property
but that means we have to modify the DT in OpenSBI (RISC-V specific
run time service provider).
We have been trying to avoid that if possible so that DT is not
bounced around. This also limits
U-Boot to have its own device tree.


> I'd assume the EFI 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.

Yes. EFI stub doesn't care about this information. However, it needs
to save the information somewhere
so that it can pass to the real kernel after exiting boot time services.

> - 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.
>

Sounds good to me. I will experiment with configuration table and send
the EFI stub patch series.

> 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.
>

Sure. There is no kexec implementation available now. We will keep this in mind
while implementing it. Thanks!

>
>
>
> > ---
> > 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
> >



-- 
Regards,
Atish