Re: [PATCH 1/2] Documentation: riscv: Add early boot document

[Conor Dooley]

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@xxxxxxxxxx> 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:
> > > > 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.
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.

> > 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.

> > > > > +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.

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature