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

[Atish Patra]

On Thu, Feb 13, 2020 at 2:11 PM Ard Biesheuvel
<ard.biesheuvel at linaro.org> wrote:
>
> On Thu, 13 Feb 2020 at 19:59, Atish Patra <atishp at atishpatra.org> wrote:
> >
> > On Tue, Feb 11, 2020 at 11:28 PM Ard Biesheuvel
> > <ard.biesheuvel at linaro.org> wrote:
> > >
> > > On Wed, 12 Feb 2020 at 06:49, Chang, Abner (HPS SW/FW Technologist)
> > > <abner.chang at hpe.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Heinrich Schuchardt [mailto:xypron.glpk at gmx.de]
> > > > > Sent: Wednesday, February 12, 2020 2:26 AM
> > > > > To: Chang, Abner (HPS SW/FW Technologist) <abner.chang at hpe.com>;
> > > > > Atish Patra <atishp at atishpatra.org>; Ard Biesheuvel
> > > > > <ard.biesheuvel at linaro.org>
> > > > > Cc: Alexander Graf <agraf at csgraf.de>; U-Boot Mailing List <u-
> > > > > boot at lists.denx.de>; Atish Patra <atish.patra at wdc.com>;
> > > > > leif at nuviainc.com
> > > > > Subject: Re: [PATCH v2 1/1] efi_loader: architecture specific UEFI setup
> > > > >
> > > > > On 2/7/20 4:13 AM, Chang, Abner (HPS SW/FW Technologist) wrote:
> > > > > >
> > > > > >
> > > > > >> -----Original Message-----
> > > > > >> From: Atish Patra [mailto:atishp at atishpatra.org]
> > > > > >> Sent: Friday, February 7, 2020 6:56 AM
> > > > > >> To: Ard Biesheuvel <ard.biesheuvel at linaro.org>; Chang, Abner (HPS
> > > > > >> SW/FW
> > > > > >> Technologist) <abner.chang at hpe.com>
> > > > > >> Cc: Alexander Graf <agraf at csgraf.de>; Heinrich Schuchardt
> > > > > >> <xypron.glpk at gmx.de>; U-Boot Mailing List <u-boot at lists.denx.de>;
> > > > > >> Atish Patra <atish.patra at wdc.com>; leif at nuviainc.com
> > > > > >> Subject: Re: [PATCH v2 1/1] efi_loader: architecture specific UEFI
> > > > > >> setup
> > > > > >>
> > > > > >> On Thu, Feb 6, 2020 at 2:07 PM Ard Biesheuvel
> > > > > >> <ard.biesheuvel at linaro.org>
> > > > > >> wrote:
> > > > > >>>
> > > > > >>> On Thu, 6 Feb 2020 at 21:06, Atish Patra <atishp at atishpatra.org> wrote:
> > > > > >>>>
> > > > > >>>> On Thu, Feb 6, 2020 at 12:10 PM Alexander Graf <agraf at csgraf.de>
> > > > > wrote:
> > > > > >>>>>
> > > > > >>>>>
> > > > > >>>>> On 06.02.20 19:28, Atish Patra wrote:
> > > > > >>>>>> 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.a
> > > > > >>>>>>>> doc
> > > > > >>>>>>>>
> > > > > >>>>>>>> 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 don't understand how this is different from the UEFI variable
> > > > > >>>>> scheme proposed here? If you want to create an SBI interface to
> > > > > >>>>> propagate the active HART that U-Boot then uses to populate the
> > > > > >>>>> /chosen property, that's probably fine as well.
> > > > > >>>>>
> > > > > >>>>
> > > > > >>>> We don't want to create SBI interface to pass this information.
> > > > > >>>>
> > > > > >>>>> We already have hooks that allow you to modify the DT right before
> > > > > >>>>> it gets delivered to the payload. Just add it there?
> > > > > >>>>>
> > > > > >>>>
> > > > > >>>> Hmm. I guess it is true if the DT is loaded from MMC or network as well.
> > > > > >>>> How about EDK2 ? If we go DT route, it also has to modify the DT to
> > > > > >>>> pass the boot hart.
> > > > > >>>>
> > > > > >>>> As it requires DT modification in multiple projects, why not use
> > > > > >>>> efi configuration tables as suggested by Ard ?
> > > > > >>>>
> > > > > >>>
> > > > > >>> Configuration tables are preferred over variables, but putting it in
> > > > > >>> the DT makes even more sense, since in that case, nothing that runs
> > > > > >>> in the UEFI context has to care about any of this.
> > > > > >>>
> > > > > >>>>>>
> > > > > >>>>>>
> > > > > >>>>>>> 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.
> > > > > >>>>>
> > > > > >>>>>
> > > > > >>>>> DT sounds like a pretty natural choice for that to me :).
> > > > > >>>>>
> > > > > >>>
> > > > > >>> Indeed. DT has a /chosen node that is set aside for this purpose. It
> > > > > >>> does depend on how early you need the value (i.e., before or after
> > > > > >>> you can run C code), but since you are passing the DT address to the
> > > > > >>> core kernel, it makes way more sense to drop any additional
> > > > > >>> information that you need to pass in there.
> > > > > >>
> > > > > >> We don't need boot hart id until real kernel boots and parse DT. So
> > > > > >> that should be okay.
> > > > > >> I just looked at the efi stub code once more and realized that it is
> > > > > >> already parsing the DT to setup uefi memory maps from /chosen node.
> > > > > >> Adding boot hart id to the chosen node does seem much cleaner to me
> > > > > >> :). Thanks for all the explanations.
> > > > > >>
> > > > > >> I have not looked at EDK2 code. But I am assuming modifying the DT
> > > > > >> just before jumping to the payload won't be too hard for EDK2 as well.
> > > > > > We don’t use DT in edk2 RISC-V port and we pass boot HART ID in SMBIOS
> > > > > > type 44h as it is spec out in below link,
> > > > > > https://github.com/riscv/riscv-smbios/blob/master/RISCV-SMBIOS.md
> > > > >
> > > > > Thanks for the link.
> > > > >
> > > > > For 'RISC-V SMBIOS Type 44 Processor Additional Information' I find entry
> > > > > 0x13h 1: This is boot hart to boot system .
> > > > >
> > > > > But is '44' a hexadecimal number? The document does not indicate this.
> > > > Type '44' is decimal format as it mentioned in SMBIOS spec, I had typo in above which said '44h'. However, that's good to mention this in RISCV_SMBIOS.md. Thanks for the recommendation.
> > > > >
> > >
> > > SMBIOS data is intended to describe the hardware to system
> > > administrators, not to the OS loader, and I don't think it makes sense
> > > to rely on it for booting. I'd assume that SMBIOS tables are not
> > > mandatory to begin with.
> > >
> > > For EFI boot, it is acceptable if the stub loader in Linux itself
> > > needs to obtain the value from something like a device tree and pass
> > > it in a CPU register at handover time,
> >
> > That's what I am planning to do for now. We can add SMBIOS parsing as well
> > if required in future.
> >
> > although I would still prefer
> > > it if the kernel simply gets it from the device tree directly if one
> > > is guaranteed to be available.
> > >
> >
> > That would break current booting protocol in RISC-V where register "a0" should
> > contain the booting hartid. If we have to move away for that method,
> > changes need to
> > be in multiple places (to modify the DT) and it has to be done in a
> > backward compatible
> > way.
> >
>
> How do you pass the device tree address?

in register "a1"

> > > Adding a new ABI between the firmware and the stub loader in Linux to
> > > use EFI specific conduits like config tables or EFI variables should
> > > really be avoided, though, as it affects every EFI loader while the
> > > code that runs in the EFI context doesn't even care (note that beyond
> > > u-boot and GRUB, there are other EFI loaders such as systemd-boot that
> > > need to be taken into account).
> >
> > Which booting stage should be responsible for changing the DT for
> > those EFI loaders ?
> >
>
> If the EFI stub for RISC-V needs to read the hart id from somewhere
> and pass it in a register when it enters the startup code of the core
> kernel, that is fine.
>
> Since DT is mandatory on your systems, and EFI is not, defining some
> EFI specific way of conveying this information seems like a bad idea
> to me.
>
> I'd say the firmware stage that incorporates the DT stuffs the hartid
> in /chosen so that any later stage can find it there if it needs to,
> without the software having to be aware of this. That way, you can use
> intermediate loaders like GRUB or systemd-boot without any changes.
> (This would actually be true when using a EFI variable for the same
> purpose, but I still prefer DT for this)

I am fine with this. For now, U-Boot can append the chosen node.

@Abner: I might have missed something. But I couldn't find anything
other than boot hartid in SMBIOS table that
EFI stub need to parse in order to boot kernel. How difficult is to
modify the DT in EDK2 ?

-- 
Regards,
Atish