Re: [PATCH for 4.21 v1 1/1] xen/riscv: identify specific ISA supported by cpu

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/8/25 4:21 PM, Jan Beulich wrote:
> On 23.12.2024 13:55, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/cpufeature.c
> > @@ -0,0 +1,466 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Taken for Linux kernel v6.12-rc3 and modified by
> > + * Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>:
> > + *
> > + * - 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.
> > + * - 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.
> > + * - 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.
> > + * - s/pr_info/printk.
> > + * - Apply Xen coding style.
> >       
> Having this in the patch description is sufficient imo.
>
> > + * Copyright (C) 2015 ARM Ltd.
> > + * Copyright (C) 2017 SiFive
> > + * Copyright (C) 2024 Vates
> > + */
> > +
> > +#include <xen/acpi.h>
> >       
> Didn't you say you dropped the ACPI pieces?
>     
Yes, they are dropped but my intention was to add "panic("%s should be updated correspondingly to support ACPI\n", __func__)"
for the places where ACPI changes should be done in case of ACPI support will be added. And these places are detected by
checking acpi_disabled variable which is in <xen/acpi.h>.

> > +#include <xen/bitmap.h>
> > +#include <xen/ctype.h>
> > +#include <xen/device_tree.h>
> > +#include <xen/errno.h>
> > +#include <xen/init.h>
> > +#include <xen/lib.h>
> > +#include <xen/sections.h>
> > +
> > +#include <asm/cpufeature.h>
> > +
> > +struct riscv_isa_ext_data {
> > +    const unsigned int id;
> > +    const char *name;
> > +};
> >       
> This is odd - why would the id be const, but not the name? Thus you
> require all instances of the struct to have an initializer. The more
> conventional approach is to apply the const on the instances of the
> structure (e.g. as you already have it for riscv_isa_ext[]).
>     
Agree, not too much sense to have const id, but not the name. Then it should be also "const char * const name".

Lets follow conventional approach and declare riscv_isa_ext_data structure as:
  struct riscv_isa_ext_data {
      unsigned int id;
      const char *name;
  };
name member of riscv_isa_ext_data structure still should be "const char *" to avoid compilation error:
  discarding of `const' qualifier from pointer target type.

An option could be to have a case in macros RISCV_ISA_EXT_DATA():
  #define RISCV_ISA_EXT_DATA(ext_name, ext_id)    \
  {                                               \
      .id = ext_id,                               \
      .name = (char *)#ext_name,                  \
  }
But IMO it is better just to declare riscv_isa_ext_data as suggested above.

> > +static int __init dt_get_cpuid_from_node(const struct dt_device_node *cpu)
> > +{
> > +    const __be32 *prop;
> > +    unsigned int reg_len;
> > +
> > +    if ( dt_n_size_cells(cpu) != 0 )
> > +        printk("cpu node `%s`: #size-cells %d\n",
> > +               dt_node_full_name(cpu), dt_n_size_cells(cpu));
> > +
> > +    prop = dt_get_property(cpu, "reg", &reg_len);
> > +    if ( !prop )
> > +    {
> > +        printk("cpu node `%s`: has no reg property\n", dt_node_full_name(cpu));
> > +        return -EINVAL;
> > +    }
> > +
> > +    if ( reg_len < dt_cells_to_size(dt_n_addr_cells(cpu)) )
> > +    {
> > +        printk("cpu node `%s`: reg property too short\n",
> > +               dt_node_full_name(cpu));
> > +        return -EINVAL;
> > +    }
> > +
> > +    return dt_read_paddr(prop, dt_n_addr_cells(cpu));
> > +}
> > +
> > +/*
> > + * The canonical order of ISA extension names in the ISA string is defined in
> > + * chapter 27 of the unprivileged specification.
> > + *
> > + * Ordinarily, for in-kernel data structures, this order is unimportant but
> > + * isa_ext_arr defines the order of the ISA string in /proc/cpuinfo.
> >       
> Inapplicable Linux detail? (If you want to keep it, you'll want to add
> "Linux'es" and avoid mentioning something that looks like a variable
> but then doesn't exist anywhere.)
>     
Agree, no need for this Linux detail, it is not really useful in our case. I will drop it.

> > +    RISCV_ISA_EXT_DATA(i, RISCV_ISA_EXT_i),
> > +    RISCV_ISA_EXT_DATA(m, RISCV_ISA_EXT_m),
> > +    RISCV_ISA_EXT_DATA(a, RISCV_ISA_EXT_a),
> > +    RISCV_ISA_EXT_DATA(f, RISCV_ISA_EXT_f),
> > +    RISCV_ISA_EXT_DATA(d, RISCV_ISA_EXT_d),
> > +    RISCV_ISA_EXT_DATA(q, RISCV_ISA_EXT_q),
> > +    RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
> > +    RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
> > +    RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> > +    RISCV_ISA_EXT_DATA(zifencei, RISCV_ISA_EXT_ZIFENCEI),
> > +    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> > +    RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
> > +    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> >       
> Isn't it kind of implied that with the presence of Zbb, B should also be
> present?
>     
My interpretation of the RISC-V Bitmanip Extension spec is that the 'B' extension is essentially a collection of
the Zba, Zbb, Zbs, and other extensions, but it isn't an extension by itself.
The following is mentioned in the spec:
  The bit-manipulation (bitmanip) extension collection is comprised of several component extensions to the base
  RISC-V architecture that are intended to provide some combination of code size reduction, performance
  improvement, and energy reduction. While the instructions are intended to have general use, some instructions
  are more useful in some domains than others. Hence, several smaller bitmanip extensions are provided, rather
  than one large extension. Each of these smaller extensions is grouped by common function and use case, and
  each of which has its own Zb*-extension name.
> > +    RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > +    RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > +};
> >       
> Wouldn't the Z and S prefixes better be recorded in upper case here?
>     
I decided that it is always mentioned in lower case in the spec, but it is wrong and Z and S are
used in upper case ( https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc#subset-naming-convention ),
and also there is another one reason mentioned in an answer to another your question below. ( it was marked as [1] )

> > +    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> > +    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> > +};
> > +
> > +static const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
> >       
> Is this variable really useful to have?
>     
Not really. ( at least, at the moment ) It could be dropped.

> > +    {
> > +        const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
> > +
> > +        if ( (name_end - name == strlen(ext->name)) &&
> > +             !strncasecmp(name, ext->name, name_end - name) )
> >       
> Does this really need to be a case-insensitive comparison?
>     
[1]:
Considering that we are discussing to use Z and S in upper case then we need to use strncasecmp()
because `name` comes from riscv, isa property of device tree and it means that letters in
the riscv,isa string must be all lowercase ( https://elixir.bootlin.com/linux/v6.12.6/source/Documentation/devicetree/bindings/riscv/extensions.yaml#L34 )
so we have to avoid comparison of extension names which could start in upper case with the same extension name which starts
from lower case:
      While the isa strings in ISA specification are case
      insensitive, letters in the riscv,isa string must be all
      lowercase.

And so to avoid comparison of a name from riscv_isa_ext[] which could be theoretically starts from upper case
we have to use strncasecmp() to transform everything to lower case before comparison. And it seems it is better
to be on a safe side for the cases when some accidentally will use upper case or in riscv_isa_ext[] or in riscv,isa property
of device-tree.

> > +            break;
> > +        }
> > +    }
> > +}
> > +
> > +static int __init riscv_isa_parse_string(const char *isa, unsigned long *out_bitmap)
> > +{
> > +    if ( isa[0] != 'r' && isa[0] != 'R' )
> > +        return -EINVAL;
> > +
> > +    if ( isa[1] != 'v' && isa[1] != 'V' )
> > +        return -EINVAL;
> > +
> > +    if ( isa[2] != '3' && isa[3] != '2' &&
> > +         isa[2] != '6' && isa[3] != '4' )
> > +        return -EINVAL;
> >       
> Any reason to accept the (respectively, depending on configuration) wrong
> bitness? Or to accept e.g. RV34?
>     
Good question. I think there is no any reason to accept the wrong bitness and
it is a bug. I'll re-write the last if-condition.

> > +    isa += 4;
> > +
> > +    while ( *isa )
> > +    {
> > +        const char *ext = isa++;
> > +        const char *ext_end = isa;
> > +        bool ext_err = false;
> > +
> > +        switch ( *ext )
> > +        {
> > +        case 'x':
> > +        case 'X':
> > +            if ( acpi_disabled )
> > +                printk_once("Vendor extensions are ignored in riscv,isa."
> > +                            "Use riscv,isa-extensions instead\n");
> >       
> How's this connected to ACPI? The more that you said there's nothing
> ACPI-ish left.
>     
The same as above just left that for the case if one day someone will add ACPI support.
( but, at the moment, we could drop it or I will update the comment when I wrote about
dropping of ACPI-connected things to something like: "ACPI is almost fully drop and leave
only places where a code should be updated to have ACPI support".
> > +            /*
> > +             * Workaround for invalid single-letter 's' & 'u' (QEMU).
> > +             * No need to set the bit in riscv_isa as 's' & 'u' are
> > +             * not valid ISA extensions. It works unless the first
> > +             * multi-letter extension in the ISA string begins with
> > +             * "Su" and is not prefixed with an underscore.
> > +             */
> > +            if ( ext[-1] != '_' && ext[1] == 'u' )
> > +            {
> > +                ++isa;
> > +                ext_err = true;
> > +                break;
> > +            }
> >       
> I'm afraid I don't understand this; the comment raises more questions
> than it answers.
>     
Some details could be found here about these QEMU workaround from LK view:
https://lore.kernel.org/linux-riscv/ae93358e-e117-b43d-faad-772c529f846c@xxxxxxxxxxxx/#t

This leads to the following fix in QEMU:
https://patchwork.kernel.org/project/qemu-devel/patch/dee09d708405075420b29115c1e9e87910b8da55.1648270894.git.research_trasio@xxxxxxxxxxxx/#24792587

Considering QEMU's patch, these workaround isn't needed anymore since QEMU 7.1 ( it has been released 30 Aug 2022 ) probably we could update the
QEMU version on our CI and just drop these changes.
Or, at least, update the comment with the links mentioned above and add a message that these changes are needed only for QEMU < 7.1.
Am I right that we don't have something like GCC_VERSION in Xen but for QEMU?
> > +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> > +            continue;
> > +
> > +        cpuid = dt_get_cpuid_from_node(cpu);
> > +        if ( cpuid < 0 )
> > +            continue;
> > +
> > +        if ( cpuid >= NR_CPUS )
> > +        {
> > +            printk_once("%s: dts has more cpu than NR_CPUs\n", __func__);
> > +            continue;
> > +        }
> > +
> > +        if ( dt_property_read_string(cpu, "riscv,isa-extensions", &isa) )
> > +        {
> > +            printk("Unable to find \"riscv,isa-extensions\" devicetree entry "
> > +                   "for cpu%d\n", cpuid);
> > +            continue;
> > +        }
> > +        else
> > +            printk("riscv,isa-extensions isnt supported\n");
> >       
> IOW no matter what, a message will be logged. Odd.
>     
I think it should be panic but I confused it with printk...

> Also: Preferably no "else" after an if() ending in "continue".
>     
If I understand you correctly, we really can drop here "else" and just
panic("riscv,isa-extensions isnt supported\n"), and it would be enough.
> > +        DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
> > +
> > +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> > +            continue;
> > +
> > +        cpuid = dt_get_cpuid_from_node(cpu);
> > +        if ( cpuid < 0 )
> > +            continue;
> > +
> > +        if ( cpuid >= NR_CPUS )
> > +        {
> > +            printk_once("%s: dts has more cpu than NR_CPUs\n", __func__);
> >       
> What's "dts"? Did the 's' mean to be in "cpus" instead? Also NR_CPUS
> to avoid confusion.
>     
DTS here means Device Tree Source. It would be more clear to write in the following way:
"CPU id is higher then maximum number of CPUs in Xen."
But I am not sure that his check is correct as it is not clearly defined that CPU ids are
from the range from 0 to NR_CPUS. The following is mentioned in the spec:
  Hart IDs might not necessarily be numbered contiguously in a multiprocessor system,
  but at least one hart must have a hart ID of zero. Hart IDs must be unique within the
  execution environment.
All that it guarantees it is that one of them should be zero and IDs are unique.

I think it would be better just to drop this check.
> > +            panic("there is no support for ACPI\n");
> > +
> > +        riscv_isa_parse_string(isa, this_isa);
> > +
> > +        if ( bitmap_empty(riscv_isa, RISCV_ISA_EXT_MAX) )
> > +            bitmap_copy(riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
> > +        else
> > +            bitmap_and(riscv_isa, riscv_isa, this_isa, RISCV_ISA_EXT_MAX);
> >       
> What if the first instance had no extensions at all? You'll then copy what
> the second instance say, ending up with extensions not supported by one of
> the CPUs. 
>     
I think that it's impossible that there is no extensions at all and it should be 
considered as a bug of provided riscv,isa property. Thereby it should be enough to
add BUG_ON(!bitmap_empty(this_isa, RISCV_ISA_EXT_MAX)) before if-condition.
> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/cpufeature.h
> > @@ -0,0 +1,63 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef ASM__RISCV__CPUFEATURE_H
> > +#define ASM__RISCV__CPUFEATURE_H
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +#define RISCV_ISA_EXT_a     ('a' - 'a')
> > +#define RISCV_ISA_EXT_c     ('c' - 'a')
> > +#define RISCV_ISA_EXT_d     ('d' - 'a')
> > +#define RISCV_ISA_EXT_f     ('f' - 'a')
> > +#define RISCV_ISA_EXT_h     ('h' - 'a')
> > +#define RISCV_ISA_EXT_i     ('i' - 'a')
> > +#define RISCV_ISA_EXT_m     ('m' - 'a')
> > +#define RISCV_ISA_EXT_q     ('q' - 'a')
> > +#define RISCV_ISA_EXT_v     ('v' - 'a')
> > +
> > +/*
> > + * Increse this to higher value as kernel support more ISA extensions.
> > + */
> > +#define RISCV_ISA_EXT_MAX   128
> >       
> What's this about? Why can't the last element of the enum below go without
> this, thus not needing manual bumping here?
>     
RISCV_ISA_EXT_ID_MAX could be used instead of RISCV_ISA_EXT_MAX. RISCV_ISA_EXT_MAX it
is rudiment from Linux kernel which initially I decided to leave to have more close to
Linux kernel code.

> > +#define RISCV_ISA_EXT_SxAIA     RISCV_ISA_EXT_SSAIA
> >       
> Why does this expand to RISCV_ISA_EXT_SSAIA and not RISCV_ISA_EXT_SMAIA?
> (Easiest way to address: remove the #define, as it's unused. Yet if it
> is to be kept, the question needs addressing, perhaps by way of a code
> comment.)
>     
I agree that it is better just to drop and I will do in that way.
Initial idea was close to Linux kernel. As Linux kernel could run in M mode
then these define should be *_EXT_SMAIA and if it is run in S mode then *_EXT_SSAIA:
#ifdef CONFIG_RISCV_M_MODE
#define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
#else
#define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
#endif

And I thought to something similar with hypervisor ( before decided that hypervisor mode is mandatory ).
> > +/*
> > + * These macros represent the logical IDs of each multi-letter RISC-V ISA
> > + * extension and are used in the ISA bitmap. The logical IDs start from
> > + * RISCV_ISA_EXT_BASE, which allows the 0-25 range to be reserved for single
> > + * letter extensions. The maximum, RISCV_ISA_EXT_MAX, is defined in order
> > + * to allocate the bitmap and may be increased when necessary.
> > + *
> > + * New extensions should just be added to the bottom, rather than added
> > + * alphabetically, in order to avoid unnecessary shuffling.
> > + */
> > +#define RISCV_ISA_EXT_BASE  26
> >       
> The comment living above this #define, it also wants wording to match
> this. Specifically the text starts with describing ...
>
> > +enum riscv_isa_ext_id {
> >       
> ... this enum instead (which doesn't consist of any macros).
>
> > +    RISCV_ISA_EXT_ZICNTR = RISCV_ISA_EXT_BASE,
> > +    RISCV_ISA_EXT_ZICSR,
> > +    RISCV_ISA_EXT_ZIFENCEI,
> > +    RISCV_ISA_EXT_ZIHINTPAUSE,
> > +    RISCV_ISA_EXT_ZIHPM,
> > +    RISCV_ISA_EXT_ZBB,
> > +    RISCV_ISA_EXT_SMAIA,
> > +    RISCV_ISA_EXT_SSAIA,
> > +    RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
> > +};
> >       
> Why can't the single-letter RISCV_ISA_EXT_? be part of this enum as well?
>     
Good point. It could and would be better. Thereby I will refactor that.

> > +void riscv_fill_hwcap(void);
> > +
> > +bool riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit);
> >       
> A signed bit position? Can negative values be passed in? Actually - can't
> this be enum riscv_isa_ext_id anyway?
>     
No, negative values can't be passed. Extension IDs are always positive.
If we will move everything ( single-letter extensions too ) to enum 
riscv_isa_ext_id then we can use enum riscv_isa_ext_id
instead of int here. I think that it would be really better so I will refactor that
accordingly.

Thanks for review!
~ Oleksii