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

[Heinrich Schuchardt]

Hello Daniel, hello Leif,

what is the GRUB view on this discussion?

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