Message-ID: <CAHVXubjNHS1goZx=hiAC0Y5n2Ox_-6=+9wb+qphgn_AV0+hAHw@mail.gmail.com>
Date:   Tue, 20 Jun 2023 10:24:49 +0200
From:   Alexandre Ghiti <alexghiti@...osinc.com>
To:     Sunil V L <sunilvl@...tanamicro.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 Tue, Jun 20, 2023 at 9:41 AM Sunil V L <sunilvl@...tanamicro.com> 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@...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?

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

Powered by blists - more mailing lists