Message-ID: <CAHVXubgaJ7m88_nh-pNrNGPG43P18Qv+6DmGJ6Gcb2DORJxqAA@mail.gmail.com>
Date:   Tue, 20 Jun 2023 14:00:53 +0200
From:   Alexandre Ghiti <alexghiti@...osinc.com>
To:     Conor Dooley <conor.dooley@...rochip.com>
Cc:     Conor Dooley <conor@...nel.org>,
        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 12:33 PM Conor Dooley
<conor.dooley@...rochip.com> wrote:
>
> On Tue, Jun 20, 2023 at 11:09:47AM +0200, Alexandre Ghiti wrote:
> > On Mon, Jun 19, 2023 at 6:00 PM Conor Dooley <conor@...nel.org> 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:
> > > > > On Mon, Jun 19, 2023 at 11:47:04AM +0200, Alexandre Ghiti wrote:
>
> > > > > > +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.
> > >
> > > "Non-boot Hart Initialisation"?
> >
> > Hmmm not that great either (sorry for being direct here)
>
> lol, no need to apologise.
>
> > > > > > +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?
> > >
> > > Uh, you don't need to use OpenSBI in the first place, let alone use a
> > > recent version of it, right? What am I missing?
> >
> > You need a M-Mode firmware which follows the SBI specification and
> > that implements the HSM extension.
>
> Firstly, and maybe I am showing my ignorance here, but we do support
> m-mode in Linux, and SMP is not disabled for m-mode kernels where it is
> required to use the spinwait method.

You're right.

> Secondly, I don't think that we've actually noted that non-HSM booting
> is deprecated before now - at least not somewhere obviously. Things like
> the platform spec on github might require it & it may be deprecated in
> SBI implementations etc, but in the Kconfig option it is not described
> as deprecated. The Kconfig option only says that it "should be only
> enabled for M-mode Linux or platforms relying on older firmware without
> SBI HSM extension".
> I think marking it as deprecated here is not accurate, and instead we
> would be better off pointing out what the limitations of the method are
> and noting the limited situations when it should be used.

You're right again, before this is officially deprecated, I'll point
out the limitations of this method only as follows:

Kernel entrance
---------------

On SMP systems, there are 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 mostly used to
  support older firmwares without SBI HSM extension and M-mode RISC-V
kernel.
- `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. The ordered booting method is the preferred booting
method for
  booting the RISC-V kernel because it can support cpu hotplug and kexec.

>
> > > Also, what about !SMP systems? Although my suggested new section title
> > > kinda addresses that!
> >
> > I'll add "On SMP systems, there exist 2 methods to enter the
>
> nit: s/exist/are/
>
> > kernel:....", I don't think we need to detail the !SMP case as it is
> > obvious.
>
> That's fine. Maybe I am just a pedant, but I think it is good to be a
> bit over precise.
>
> > > > > > +  **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.
>
> > > > > > +---------------------
> > > > > > +
> > > > > > +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?
> > >
> > > No? The "the" there would not be required.
> >
> > That was a genuine question, thanks
>
> Sorry if the "No?" came across as aggressive, I meant it inquisitively.
> Adding the "the" changes the meaning slightly, but not in a way that
> that is relevant here.

No worries :)

>
> > > > > > +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
> > >
> > > Not even a list, just something like "...established, for example early
> > > alternatives and foo, must be very..."
> >
> > Done as follows:
> >
> > "A few pieces of code need to run before even the first virtual mapping is
> > established, that comprises the installation of the first virtual mapping, the
> > early alternatives and the early parsing of the kernel command line. That code
> > must be very carefully compiled as:..."
>
> Changed slightly:
> "A few pieces of code need to run before even the first virtual mapping is
> established. These are the installation of the first virtual mapping itself,
> patching of early alternatives and the early parsing of the kernel command line.
> That code must be very carefully compiled as:..."
>
> Two minor suggestions there, one to make the it more obvious what the first
> section inside commas refers to & one to note what it is that we do with
> the alternatives.
>

Thanks

> Cheers,
> Conor.

Powered by blists - more mailing lists