Message-ID: <CAHVXubjV=0HNyc0-UMAQRQfi4ZUnwH8dmghV-BGogZsJiumtZA@mail.gmail.com>
Date:   Mon, 19 Jun 2023 16:04:52 +0200
From:   Alexandre Ghiti <alexghiti@...osinc.com>
To:     Conor Dooley <conor.dooley@...rochip.com>,
        Atish Kumar Patra <atishp@...osinc.com>
Cc:     Jonathan Corbet <corbet@....net>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>, linux-doc@...r.kernel.org,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] Documentation: riscv: Add early boot document

On Mon, Jun 19, 2023 at 2:26 PM Conor Dooley <conor.dooley@...rochip.com> wrote:
>
> Hey Alex,
>
> Thanks for working on this :) I've got a mix of suggestions and
> questions below. Hopefully it is not too disjoint, since I didn't write
> them in order.
>
> On Mon, Jun 19, 2023 at 11:47:04AM +0200, Alexandre Ghiti wrote:
> > This document describes the constraints and requirements of the early
> > boot process in a RISC-V kernel.
> >
> > Szigned-off-by: Alexandre Ghiti <alexghiti@...osinc.com>
> > ---
> >  Documentation/riscv/boot-image-header.rst |   3 -
> >  Documentation/riscv/boot.rst              | 181 ++++++++++++++++++++++
> >  Documentation/riscv/index.rst             |   1 +
> >  3 files changed, 182 insertions(+), 3 deletions(-)
> >  create mode 100644 Documentation/riscv/boot.rst
> >
> > diff --git a/Documentation/riscv/boot-image-header.rst b/Documentation/riscv/boot-image-header.rst
> > index d7752533865f..a4a45310c4c4 100644
> > --- a/Documentation/riscv/boot-image-header.rst
> > +++ b/Documentation/riscv/boot-image-header.rst
> > @@ -7,9 +7,6 @@ Boot image header in RISC-V Linux
> >
> >  This document only describes the boot image header details for RISC-V Linux.
> >
> > -TODO:
> > -  Write a complete booting guide.
> > -
> >  The following 64-byte header is present in decompressed Linux kernel image::
> >
> >       u32 code0;                /* Executable code */
> > diff --git a/Documentation/riscv/boot.rst b/Documentation/riscv/boot.rst
> > new file mode 100644
> > index 000000000000..b02230818b79
> > --- /dev/null
> > +++ b/Documentation/riscv/boot.rst
> > @@ -0,0 +1,181 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +=============================================
> > +Early boot requirements/constraints on RISC-V
> > +=============================================
>
> Please use "title case", here and elsewhere in the doc.

You mean using "title: " instead of "===="? Or using uppercase for the
first letter of each word? FYI I followed
https://docs.kernel.org/doc-guide/sphinx.html?highlight=title#specific-guidelines-for-the-kernel-documentation

> I'd also be inclined to drop the "Early" from here, as it permits more
> natural section headings. Perhaps "RISC-V Kernel Boot Requirements and
> Constraints"?

Good suggestion, I'll go with that, thanks

>
> > +
> > +:Author: Alexandre Ghiti <alexghiti@...osinc.com>
> > +:Date: 23 May 2023
> > +
> > +This document describes what the RISC-V kernel expects from the previous stages
>
> "the previous stages" is a bit vague IMO. You mean bootloader stages I
> assume, but I think it should be explicit. Perhaps:
> "...what a RISC-V kernel expects from bootloaders and firmware, and the
> constraints..."
>
> > +and the firmware, but also the constraints that any developer must have in mind
> > +when touching the early boot process, e.g. before the final virtual mapping is
> > +setup.
>
> s/setup./set up./
>
> Do you mean to have "For example" here? Or is "before the final virtual
> mapping is set up" the definition or "early boot"? If the latter, I
> would reword this as something like:
> "...when modifying the early boot process. For the purposes of this
> document, the 'early boot process' refers to any code that runs before
> the final virtual mapping is set up."

Thanks, that's what I meant.

>
> > +Pre-kernel boot (Expectations from firmware)
>
> Firmware or bootloaders? TBH, I would just drop the section in () and
> do something like:
>         Pre-kernel Requirements and Constraints
>         =======================================
>
>         The RISC-V kernel expects the following of bootloaders and platform
>         firmware:
>

Ok

> > +
> > +Registers state
>
> s/Registers state/Register State/

Ok

>
> > +---------------
> > +
> > +The RISC-V kernel expects:
> > +
> > +  * `$a0` to contain the hartid of the current core.
> > +  * `$a1` to contain the address of the device tree in memory.
> > +
> > +CSR state
> > +---------
> > +
> > +The RISC-V kernel expects:
> > +
> > +  * `$satp = 0`: the MMU must be disabled.
>
> "the MMU, if present, must be disabled." ;)

Ahah forgot the !mmu case, thanks :)

>
> > +
> > +Reserved memory for resident firmware
> > +-------------------------------------
> > +
> > +The RISC-V kernel expects the firmware to mark any resident memory with the
>
> Should this be
> "...resident memory, or memory it has protected with PMPs, with..."
> ?

I used "resident" memory instead of "PMP" memory because it was more
general. I mean you can have a region that is resident but not
protected by PMP, and I don't think the kernel should ask for this
resident memory to be protected with PMP right?

>
> > +`no-map` flag, thus the kernel won't map those regions in the direct mapping
>
> "no-map" is a DT specific term, should this section be moved down under
> DT, as a sub-section of that?

Maybe I can rephrase with something like that:

"The RISC-V kernel must not map any resident memory in the direct
mapping, so the firmware must correctly mark those regions as follows:
- when using a devicetree, using the `no-map` flag,
- when booting with UEFI without devicetree, either as
`EfiRuntimeServicesData/Code` or `EfiReserved`."

>
> > +(avoiding issues with hibernation, speculative accesses and probably other
> > +subsystems).
>
> I'm not sure that this () section is beneficial. To be honest, recent
> issues aside, this section here seems like a statement of the obvious...

But I made the mistake, so it was not that straightforward to
me...I'll remove the ().

>
> > +
> > +Kernel location
> > +---------------
> > +
> > +The RISC-V kernel expects to be placed at a PMD boundary (2MB for rv64 and 4MB
>
> Would that be better worded as "(2 MB aligned for rv64 and 4 MB aligned
> for rv32)"? It might be overly explicit, but I figure there's no harm...

Sure I can add that.

>
> > +for rv32). Note though that the EFI stub will physically relocate the kernel if
>
> s/though//

Ok

>
> > +that's not the case.
> > +
> > +Device-tree

Damn, missed this one, thanks!

>
> s/Device-tree/Devicetree/ and...
>
> > +-----------
> > +
> > +The RISC-V kernel always expects a device tree, it is:
>
> ...s/device tree/devicetree/ to match elsewhere in the kernel docs.
> Same applies to the other instances of "device tree" in this patch,
> please.

Ok I'll do that (but I'm happy to say that I thought about that and it
was intentional since "git grep "device tree" | wc -l" returns a
significant number of instances :)).

>
> > +
> > +- either passed directly to the kernel from the previous stage using the `$a1`
> > +  register,
> > +- or when booting with UEFI, the device tree will be retrieved by the EFI stub
> > +  using the EFI configuration table or it will be created.
>
> Can I suggest changing this around a little, pulling the "either" &
> dropping some boilerplate so that it reads (to me!) a little more
> naturally:
>         The RISC-V kernel always expects a devicetree, it is either:
>
>         - passed directly to the kernel from the previous stage using the `$a1`
>           register,
>         - retrieved by the EFI stub when booting with UEFI, using the EFI
>           configuration table or it will be created by ____.
>
> Also, please elaborate on what it will be created by.

Is it better this way:

"The RISC-V kernel always expects a devicetree, it is either:

- passed directly to the kernel from the previous stage using the
`$a1`
  register,
- retrieved by the EFI stub if present in the EFI configuration table,
- created by the EFI stub otherwise."

>
> > +
> > +Bootflow
>
> "Boot Flow", no?
> I am not sure that this is the "correct" heading for the content it
> describes, but I have nothing better to offer :/

Yes you're right, what about simply "Kernel Entrance"? Not sure this
is easily understandable though.

>
> > +--------
> > +
> > +There exist 2 methods to enter the kernel:
> > +
> > +- `RISCV_BOOT_SPINWAIT`: the firmware releases all harts in the kernel, one hart
> > +  wins a lottery and executes the early boot code while the other harts are
> > +  parked waiting for the initialization to finish. This method is now
>
> nit: s/now//

Ok

>
> What do you mean by deprecated? There's no requirement to implement the
> HSM extension, right?

I would say yes, you have to use a recent version of openSBI that
supports the HSM extension. @Atish Kumar Patra WDYT?

>
> > +  **deprecated**.
> > +- Ordered booting: the firmware releases only one hart that will execute the
> > +  initialization phase and then will start all other harts using the SBI HSM
> > +  extension.
> > +
> > +UEFI
> > +----
> > +
> > +UEFI memory map
> > +~~~~~~~~~~~~~~~
> > +
> > +When booting with UEFI, the RISC-V kernel will use only the EFI memory map to
> > +populate the system memory.
> > +
> > +The UEFI firmware must parse the subnodes of the `/reserved-memory` device tree
> > +node and abide by the device tree specification to convert the attributes of
> > +those subnodes (`no-map` and `reusable`) into their correct EFI equivalent
> > +(refer to section "3.5.4 /reserved-memory and UEFI" of the device tree
> > +specification).
> > +
> > +RISCV_EFI_BOOT_PROTOCOL
> > +~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +When booting with UEFI, the EFI stub requires the boot hartid in order to pass
> > +it to the RISC-V kernel in `$a1`. The EFI stub retrieves the boot hartid using
> > +one of the following methods:
> > +
> > +- `RISCV_EFI_BOOT_PROTOCOL` (**preferred**).
> > +- `boot-hartid` device tree subnode (**deprecated**).
> > +
> > +Any new firmware must implement `RISCV_EFI_BOOT_PROTOCOL` as the device tree
> > +based approach is deprecated now.
> > +
> > +During kernel boot: (Kernel internals)
>
> With the other section titles changed, this could be:
>         Early Boot Requirements and Constraints
>         =======================================
>
>         The RISC-V kernel's early boot process operates under the
>         following constraints:
>
> Thoughts?

I think it's better as you propose, I changed it, thanks.

>
> > +======================================
> > +
> > +EFI stub and device tree
>
> Same comments about "device tree" here etc.
>
> > +------------------------
> > +
> > +When booting with UEFI, the device tree is supplemented by the EFI stub with the
> > +following parameters (largely shared with arm64 in Documentation/arm/uefi.rst):
> > +
> > +==========================  ======   ===========================================
> > +Name                        Size     Description
> > +==========================  ======   ===========================================
> > +linux,uefi-system-table     64-bit   Physical address of the UEFI System Table.
>
> nit: Hmm, I think for all of these sizes s/-bit/ bits/.

That's a copy-paste from the link just above the table.

But maybe I should have pointed to the doc and added only the
"bootargs" stuff (maybe that's also present for arm64 actually).

>
> > +
> > +linux,uefi-mmap-start       64-bit   Physical address of the UEFI memory map,
> > +                                     populated by the UEFI GetMemoryMap() call.
> > +
> > +linux,uefi-mmap-size        32-bit   Size in bytes of the UEFI memory map
> > +                                     pointed to in previous entry.
> > +
> > +linux,uefi-mmap-desc-size   32-bit   Size in bytes of each entry in the UEFI
> > +                                     memory map.
> > +
> > +linux,uefi-mmap-desc-ver    32-bit   Version of the mmap descriptor format.
> > +
> > +kaslr-seed                  64-bit   Entropy used to randomize the kernel image
> > +                                     base address location.
> > +
> > +bootargs                             Kernel command line
> > +==========================  ======   ===========================================
> > +
> > +Virtual mapping setup
>
> nit: s/setup/Installation/

Ok

>
> > +---------------------
> > +
> > +The installation of the virtual mapping is done in 2 steps in the RISC-V kernel:
> > +
> > +1. :c:func:`setup_vm` installs a temporary kernel mapping in
> > +   :c:var:`early_pg_dir` which allows to discover the system memory: only the
>
> s/to discover/discovery of/

You mean "the discovery of" right?

> s/: only/. Only/

Ok

>
> > +   kernel text/data are mapped at this point. When establishing this mapping,
> > +   no allocation can be done (since the system memory is not known yet), so
> > +   :c:var:`early_pg_dir` page table is statically allocated (using only one
> > +   table for each level).
> > +
> > +2. :c:func:`setup_vm_final` creates the final kernel mapping in
> > +   :c:var:`swapper_pg_dir` and takes advantage of the discovered system memory
> > +   to create the linear mapping. When establishing this mapping, the kernel
> > +   can allocate memory but cannot access it directly (since the direct mapping
> > +   is not present yet), so it uses temporary mappings in the fixmap region to
> > +   be able to access the newly allocated page table levels.
> > +
> > +For :c:func:`virt_to_phys` and :c:func:`phys_to_virt` to be able to correctly
> > +convert direct mapping addresses to physical addresses, it needs to know the
>
> nit: s/it/they/

Ok

>
> > +start of the DRAM: this happens after 1, right before 2 installs the direct
>
> s/:/./

Ahah, you really don't like long sentences :) But of course ok :)

> Also how about s/1/step 1/ & s/2/step 2/?

Way better, thanks

>
> > +mapping (see :c:func:`setup_bootmem` function in arch/riscv/mm/init.c). So
>
> s/So//
>

Ok

> > +any usage of those macros before the final virtual mapping is installed must be
> > +carefully examined.
> > +
> > +Device-tree mapping via fixmap
> > +------------------------------
> > +
> > +The RISC-V kernel uses the fixmap region to map the device tree because the
> > +device tree virtual mapping must remain the same between :c:func:`setup_vm` and
> > +:c:func:`setup_vm_final` calls since :c:var:`reserved_mem` array is initialized
>
> Missing a "the" before reserved_mem.

Ok

>
> > +with virtual addresses established by :c:func:`setup_vm` and used with the
> > +mapping established by :c:func:`setup_vm_final`.
> > +
> > +Pre-MMU execution
> > +-----------------
> > +
> > +Any code that executes before even the first virtual mapping is established
> > +must be very carefully compiled as:
>
> Could you point out what the non-obvious examples of this code are?

I can do a list, yes

>
> > +- `-fno-pie`: This is needed for relocatable kernels which use `-fPIE`, since
>
> Is there a reason why the capitalisation is different for the two
> compiler flags?

No idea!

>
> > +  otherwise, any access to a global symbol would go through the GOT which is
> > +  only relocated virtually.
> > +- `-mcmodel=medany`: Any access to a global symbol must be PC-relative to avoid
> > +  any relocations to happen before the MMU is setup.
> > +- Also note that *all* instrumentation must also be disabled (that includes
>
> nit: s/Also note that//

Ok

>
> > +  KASAN, ftrace and others).
> > +
> > +As using a symbol from a different compilation unit requires this unit to be
> > +compiled with those flags, we advise, as much as possible, not to use external
> > +symbols.
>
> If the use of early alternatives grows, are we going to have to split
> the vendors early alternatives into a different compilation unit from
> their regular alternatives?

Indeed, that would relax this constraint for the non-early part of the
file, but as we already talked, the goal is to remove all those
constraints by moving all this code in kernel/pi (at least it is my
goal :)).

>
> Cheers,
> Conor.

Thanks for your thorough review!

Powered by blists - more mailing lists