Message-ID: <ZJFYE+Ss/OgMIjda@sunil-laptop>
Date:   Tue, 20 Jun 2023 13:11:07 +0530
From:   Sunil V L <sunilvl@...tanamicro.com>
To:     Alexandre Ghiti <alexghiti@...osinc.com>
Cc:     Conor Dooley <conor.dooley@...rochip.com>,
        Atish Kumar Patra <atishp@...osinc.com>,
        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 04:04:52PM +0200, Alexandre Ghiti wrote:
> 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`."
> 
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?

Thanks,
Sunil

Powered by blists - more mailing lists