Re: [PATCH v3] xen/riscv: identify specific ISA supported by cpu

[Conor Dooley <conor@xxxxxxxxxx>]

On Thu, Feb 06, 2025 at 02:05:31PM +0100, Oleksii Kurochko wrote:
> On 2/5/25 8:07 PM, Conor Dooley wrote:
> > On Thu, Jan 23, 2025 at 03:46:36PM +0100, Oleksii Kurochko wrote:
> > > Supported ISA extensions are specified in the device tree within the CPU
> > > node, using two properties: `riscv,isa-extensions` and `riscv,isa`.
> > > 
> > > Currently, Xen does not support the `riscv,isa-extensions` property, as
> > > all available device tree source (DTS) files in the Linux kernel 
> > > (v6.12-rc3)
> > > and DTBs generated by QEMU use only the `riscv,isa` property.
> > That's not true? The riscv,isa-extensions property went into linux in
> > late 2023 (6.7 or so) and QEMU in v9.0.0 about a year ago, so all source
> > files in linux and blobs generated by QEMU should have both. OpenSBI &
> > U-Boot support both also. Might not affect your decision about what to
> > support here - but the rationale provided for implementing the deprecated
> > (per the binding at least) property isn't accurate.
> 
> I confused something with Linux kernel sources. Double-check and 
> riscv,isa-extensions
> is really supported.
> 
> Regarding QEMU, at the momemnt, Xen uses Debian bookworm and the following 
> version
> is used:
>   QEMU emulator version 7.2.11 (Debian 1:7.2+dfsg-7+deb12u6)
> 
> I will update the commit message.
> 
> Do you ( Conor and Jan ) think that it makes sense to support deprecated 
> property?
> Or it would be better start to use QEMU v9.0.0 and just drop support for 
> deprecated property?

I'm not entirely sure how Xen re-assembles the dtb that it passes to the
guess (cos you need to strip things you don't support yet, right?), but
to keep things simpler as you're doing bringup I think you could
continue on as you are. There's some guests that only support riscv,isa
like the BSDs (IIRC), so you'd get yourself a better testing base by
sticking to dealing with just that one property.
If I were you though, I'd probably try to match interpretation with
what guests are doing as that's less likely to cause problems.

> > > Therefore, only `riscv,isa` parsing is implemented.
> > > 
> > > The `riscv,isa` property is parsed for each CPU, and the common extensions
> > > are stored in the `host_riscv_isa` bitmap.
> > > This bitmap is then used by `riscv_isa_extension_available()` to check
> > > if a specific extension is supported.
> > > 
> > > The current implementation is based on Linux kernel v6.12-rc3
> > > implementation with the following changes:
> > >   - Drop unconditional setting of {RISCV_ISA_EXT_ZICSR,
> > >     RISCV_ISA_EXT_ZIFENCEI, RISCV_ISA_EXT_ZICNTR, RISCV_ISA_EXT_ZIHPM} as 
> > > they
> > >     are now part of the riscv,isa string.
> > Hmm, not sure I follow your logic here. Linux unconditionally sets these
> > extensions because the binding was written when these these were part of
> > the base instruction set*. That they can be put into the "riscv,isa"
> > string is actually the reason that the code setting them unconditionally
> > in linux exists! Linux cannot tell the difference between an "old" dtb
> > containing "rv64ima" meaning that Zicsr is present & a "new" one containing
> > "rv64ima" meaning that it is not! For backwards compatibility reasons,
> > linux is then forced to set its internal flags for Zicsr et al when it sees
> > "i" in riscv,isa. If you were to use the "new" definition of "i", and use
> > that to construct a dtb to pass to a linux guest, things would blow up.
> 
> My fault was that I didn't consider that someone will use "old" dtb and it 
> was the reason
> why "the binding was written when these these were part of the base 
> instruction set*" was
> interpreted as it isn't so any more and the mentioned extension should be 
> explicitly
> mentioned in riscv,isa.
> 
> > 
> > This is the whole reason that riscv,isa is marked deprecated in the binding
> > and replaced by riscv,isa-extensions - there are no concrete definitions
> > for what extensions mean using "riscv,isa".
> > 
> > I suppose you're free to decide to interpret the property in Xen
> > differently to linux - particularly because the hypervisor extension
> > requirement means that you're only going to run on hardware produced after
> > the aforementioned extensions were split out of "i" - but if you are
> > doing that, the rationale for a differing interpretation should be correct
> > IMO.
> 
> Agree, I will update the commit message with the wording to:
>   Drop unconditional setting of {...} because the hypervisor is going to run 
> on
>   hardware produced after the aforementioned extensions were split out of "i".
> 
> > 
> > Perhaps the wording of my comment in linux was not "strong" enough, and
> > the "can be set" should be changed to "must be set"?
> 
> It would be better.

Okay, I'll try to remember to get that changed.

> >             /*
> >              * These ones were as they were part of the base ISA when the
> >              * port & dt-bindings were upstreamed, and so can be set
> >              * unconditionally where `i` is in riscv,isa on DT systems.
> >              */
> >             if (acpi_disabled) {
> >                     set_bit(RISCV_ISA_EXT_ZICSR, source_isa);
> >                     set_bit(RISCV_ISA_EXT_ZIFENCEI, source_isa);
> >                     set_bit(RISCV_ISA_EXT_ZICNTR, source_isa);
> >                     set_bit(RISCV_ISA_EXT_ZIHPM, source_isa);
> >             }
> > 
> > 
> > 
> > * IIRC only 2 of them were part of i, the other two were assumed to be 
> > present
> >    by Linux etc and only became independent extensions later on.

Attachment: signature.asc
Description: PGP signature