On Tue, Jun 20, 2023 at 9:41 AM Sunil V L <sunilvl@xxxxxxxxxxxxxxxx> wrote: > > On Mon, Jun 19, 2023 at 04:04:52PM +0200, Alexandre Ghiti wrote: > > 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`." > > > Hi Alex, > > I am not sure about the idea behind mentioning only UEFI boot without > DT since UEFI boot is supported with DT also. Should we just mention > that "when booting with UEFI, resident firmware ranges must be marked as > per UEFI specification" ? Converting reserved-memory node in DT to UEFI > memory map is anyway mentioned separately under UEFI memory map section > right? Right, let's make it simple: "The RISC-V kernel must not map any resident memory in the direct mapping, so the firmware must correctly mark those regions as per the devicetree specification and/or the UEFI specification." Feel free to comment if that's still not right to you, Thanks, > > Thanks, > Sunil
