On Mon, Jun 19, 2023 at 2:26 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxx> > > +: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!
