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.
signature.asc
Description: PGP signature
