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