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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/5/25 8:07 PM, Conor Dooley wrote:

> Yo,
>
> 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?

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

> 		/*
> 		 * 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.
>
>     
> >  - Remove saving of the ISA for each CPU, only the common available ISA is
> >    saved.
> >  - Remove ACPI-related code as ACPI is not supported by Xen.
> >  - Drop handling of elf_hwcap, since Xen does not provide hwcap to
> >    userspace.
> >  - Replace of_cpu_device_node_get() API, which is not available in Xen,
> >    with a combination of dt_for_each_child_node(), dt_device_type_is_equal(),
> >    and dt_get_cpuid_from_node() to retrieve cpuid and riscv,isa in
> >    riscv_fill_hwcap_from_isa_string().
> >  - Rename arguments of __RISCV_ISA_EXT_DATA() from _name to ext_name, and
> >    _id to ext_id for clarity.
> >  - Replace instances of __RISCV_ISA_EXT_DATA with RISCV_ISA_EXT_DATA.
> >  - Replace instances of __riscv_isa_extension_available with
> >    riscv_isa_extension_available for consistency. Also, update the type of
> >    `bit` argument of riscv_isa_extension_available().
> >  - Redefine RISCV_ISA_EXT_DATA() to work only with ext_name and ext_id,
> >    as other fields are not used in Xen currently.
> >  - Add check of first 4 letters of riscv,isa string to
> >    riscv_isa_parse_string() as Xen doesn't do this check before so it is
> >    necessary to check correctness of riscv,isa string. ( it should start with
> >    rv{32,64} with taking into account upper and lower case of "rv").
> >  - Drop an argument of riscv_fill_hwcap() and riscv_fill_hwcap_from_isa_string()
> >    as it isn't used, at the moment.
> >       
> >  - Update the comment message about QEMU workaround.
> >       
> Does Xen (for riscv) even work with QEMU 7.1?
>     
I haven't checked that. As mentioned above Xen's GitLab CI is using 7.2.11.

Thanks.

~ Oleksii