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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/20/25 4:13 PM, Jan Beulich wrote:

> On 15.01.2025 17:36, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/cpufeature.c
> > @@ -0,0 +1,506 @@
> > +/* 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. 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 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.
> > + * - s/pr_info/printk.
> >       
> As said before - having this in the commit message is likely enough. Putting
> it here as well is only risking for this to go stale (perhaps rather sooner
> than later).
>     
I misunderstood you last time. I will remove this comment entirely to avoid dealing
with potential staleness in the future.

> > + * Copyright (C) 2015 ARM Ltd.
> > + * Copyright (C) 2017 SiFive
> > + * Copyright (C) 2024 Vates
> > + */
> > +
> > +#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>
> > +
> > +#ifdef CONFIG_ACPI
> > +#error "cpufeature.c functions should be updated to support ACPI"
> > +#endif
> > +
> > +struct riscv_isa_ext_data {
> > +    unsigned int id;
> > +    const char *name;
> > +};
> > +
> > +#define RISCV_ISA_EXT_DATA(ext_name, ext_id)    \
> > +{                                               \
> > +    .id = ext_id,                               \
> > +    .name = #ext_name,                          \
> > +}
> > +
> > +/* Host ISA bitmap */
> > +static __ro_after_init DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX);
> > +
> > +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));
> >       
> The call to dt_n_size_cells() is here really just for the printk()?
>     
Yes, it is only used for debug purposes.

Based on DT's bindings ( https://www.kernel.org/doc/Documentation/devicetree/bindings/riscv/cpus.txt )
and RISC-V's DTS files in kernel #size-cells for cpu node is expected to be 0.
> > +    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));
> >       
> How come it is okay to (silently) truncate here from paddr_t to int?
>     
Specifically in this case it is okay as it is low chance that cpuid will be higher then the size of `int`.
But based on RISC-V spec. hartid could be from 0 to ((1ULL << (MXLEN)) - 1) [MXLEN is the size of register]
so it would be better to use `long` instead of `int` as return value and use `long` for cpuid variable in
the callers of dt_get_cpuid_from_node().

Probably it would be even better to return error code as `int` to have ability to verify if something wrong
happens during dt_get_cpuid_from_node() and add another one argument to dt_get_cpuid_from_node() with paddr_t
type ( or `unsigned long` as type `paddr_t` looks confusing a little bit for describing of cpu id ) and set this
argument before return.
> > +}
> > +
> > +/*
> > + * The canonical order of ISA extension names in the ISA string is defined in
> > + * chapter 27 of the unprivileged specification.
> > + *
> > + * The specification uses vague wording, such as should, when it comes to
> > + * ordering, so for our purposes the following rules apply:
> > + *
> > + * 1. All multi-letter extensions must be separated from other extensions by an
> > + *    underscore.
> > + *
> > + * 2. Additional standard extensions (starting with 'Z') must be sorted after
> > + *    single-letter extensions and before any higher-privileged extensions.
> > + *
> > + * 3. The first letter following the 'Z' conventionally indicates the most
> > + *    closely related alphabetical extension category, IMAFDQLCBKJTPVH.
> > + *    If multiple 'Z' extensions are named, they must be ordered first by
> > + *    category, then alphabetically within a category.
> > + *
> > + * 4. Standard supervisor-level extensions (starting with 'S') must be listed
> > + *    after standard unprivileged extensions.  If multiple supervisor-level
> > + *    extensions are listed, they must be ordered alphabetically.
> > + *
> > + * 5. Standard machine-level extensions (starting with 'Zxm') must be listed
> > + *    after any lower-privileged, standard extensions.  If multiple
> > + *    machine-level extensions are listed, they must be ordered
> > + *    alphabetically.
> > + *
> > + * 6. Non-standard extensions (starting with 'X') must be listed after all
> > + *    standard extensions. If multiple non-standard extensions are listed, they
> > + *    must be ordered alphabetically.
> > + *
> > + * An example string following the order is:
> > + *    rv64imadc_zifoo_zigoo_zafoo_sbar_scar_zxmbaz_xqux_xrux
> > + *
> > + * New entries to this struct should follow the ordering rules described above.
> > + *
> > + * Extension name must be all lowercase ( according to device-tree binding )
> > + * and strncmp() is used in match_isa_ext() to compare extension names instead
> > + * of strncasecmp().
> > + */
> > +const struct riscv_isa_ext_data __initconst riscv_isa_ext[] = {
> > +    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),
> > +    RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> > +    RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> > +};
> > +
> > +static const struct riscv_isa_ext_data __initconst required_extensions[] = {
> > +    RISCV_ISA_EXT_DATA(zbb, RISCV_ISA_EXT_ZBB),
> > +    RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
> > +};
> >       
> No Zicsr here?
>     
Agree, it should be added.


> > +
> > +static int __init riscv_isa_parse_string(const char *isa,
> > +                                         unsigned long *out_bitmap)
> > +{
> > +    /*
> > +     * According to RISC-V device tree binding isa string must be all
> > +     * lowercase.
> > +     * To be sure that this is true, ASSERT below is added.
> > +     */
> > +    ASSERT(islower(isa[0]) && islower(isa[1]));
> >       
> This looks a little odd to me when you have ...
>
> > +    if ( (isa[0] != 'r') && (isa[1] != 'v') )
> > +        return -EINVAL;
> >       
> ... this anyway.
>     
Agree, there is a little sense in having ASSERT() as, actually, if-condition does the same. I'll
drop ASSERT().

> > +#if defined(CONFIG_RISCV_32)
> > +    if ( isa[2] != '3' && isa[3] != '2' )
> > +        return -EINVAL;
> > +#elif defined(CONFIG_RISCV_64)
> > +    if ( isa[2] != '6' && isa[3] != '4' )
> > +        return -EINVAL;
> > +#else
> > +    #error "unsupported RISC-V bitness"
> > +#endif
> > +
> > +    isa += 4;
> > +
> > +    while ( *isa )
> > +    {
> > +        const char *ext = isa++;
> > +        const char *ext_end = isa;
> > +        bool ext_err = false;
> > +
> > +        switch ( *ext )
> > +        {
> > +        case 'x':
> > +        case 'X':
> > +            printk_once("Vendor extensions are ignored in riscv,isa."
> > +                        "Use riscv,isa-extensions instead\n");
> >       
> Interesting suggestion considering that doing so will end in a panic().
>     
Do you think that "Use riscv,isa-extensions instead\n" would be better to add when "riscv,isa-extensions" handling
will be ready?

> > +static int __init riscv_fill_hwcap_from_ext_list(void)
> > +{
> > +    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> > +    const struct dt_device_node *cpu;
> > +    int res = -EINVAL;
> > +
> > +    if ( !cpus )
> > +    {
> > +        printk("Missing /cpus node in the device tree?\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    dt_for_each_child_node(cpus, cpu)
> > +    {
> > +        const char *isa;
> > +        int cpuid;
> > +
> > +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> > +            continue;
> > +
> > +        cpuid = dt_get_cpuid_from_node(cpu);
> > +        if ( cpuid < 0 )
> > +            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);
> >       
> This is DT's number space for CPUs, isn't it? Any use of cpu%d (or CPU%d) or
> alike generally suggests the number is Xen's. This may want disambiguating
> here.
>     
Yeah, the cpuid in this context is from the DT's namespace.

I'm not sure if we can get Xen's number at this stage, as only one CPU is used. We can only get
the DT's cpuid for Xen's CPU0 as set_cpuid_to_hartid(0, bootcpuid) has been already called.
For other CPUs, it depends on the order in which they are booted and the call to set_cpuid_to_hartid().

I think that the best thing we can do is re-wording. I have two options:
1. "for DT's cpu%d node\n", cpuid);
2. "for hartid%d\n", cpuid); -> as based on the function name set_cpuid_to_hartid() we are using cpuid
   for Xen's cpu id and hart id - for real cpu id.
I prefer the first one option as it is more explicit and it doesn't require to know RISC-V specific terminology.

> > +static void __init riscv_fill_hwcap_from_isa_string(void)
> > +{
> > +    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> > +    const struct dt_device_node *cpu;
> > +
> > +    if ( !cpus )
> > +    {
> > +        printk("Missing /cpus node in the device tree?\n");
> > +        return;
> > +    }
> > +
> > +    dt_for_each_child_node(cpus, cpu)
> > +    {
> > +        DECLARE_BITMAP(this_isa, RISCV_ISA_EXT_MAX);
> > +        const char *isa;
> > +        int cpuid;
> > +
> > +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> > +            continue;
> > +
> > +        cpuid = dt_get_cpuid_from_node(cpu);
> > +        if ( cpuid < 0 )
> > +            continue;
> > +
> > +        if ( dt_property_read_string(cpu, "riscv,isa", &isa) )
> > +        {
> > +            printk("Unable to find \"riscv,isa\" devicetree entry\n");
> > +            continue;
> > +        }
> >       
> Interestingly you don't log the CPU number here. What's the deal?
>     
Missed to do that, I will add CPU number to printk().

> > +bool riscv_isa_extension_available(const unsigned long *isa_bitmap,
> > +                                   enum riscv_isa_ext_id bit)
> > +{
> > +    const unsigned long *bmap = (isa_bitmap) ? isa_bitmap : riscv_isa;
> >       
> Since it helps readability, may I suggest
>
>     const unsigned long *bmap = isa_bitmap ?: riscv_isa;
>
> or even getting away without the local var altogether:
>
>     if ( !isa_bitmap )
>         isa_bitmap = riscv_isa;
>
> ?
>     
I think it would be better then go without local variable. I will proceed in that way.

> > +    if ( ret )
> > +    {
> > +        printk("Falling back to deprecated \"riscv,isa\"\n");
> > +        riscv_fill_hwcap_from_isa_string();
> > +    }
> >       
> I continue to find it irritating that you first try a function you
> know won't succeed (and will panic() if the DT item is actually there),
> in order to the log yet another message about using something that's
> deprecated. If this is deprecated, why don't we prefer (and hence
> support) the mor modern approach?
>     
Even though it is considered deprecated, I haven't seen any DTS files in the Linux kernel using riscv,isa-extension.
Currently, Xen relies on the DTB generated by QEMU, which still uses riscv,isa. This is why, unfortunately, we still
need to support the deprecated riscv,isa. (Although I would much prefer using riscv,isa-extension since it's easier
to parse.)
Based on the fact that every source I checked doesn't use riscv,isa-extension, I decided to postpone adding support
for it. However, this task is still on my TODO list.
I completely agree that this is a frustrating approach. But at the time of porting the code, it seemed like the
best option. After you pointed it out, I think we could improve this part of the code in the following way:
  -    int ret = riscv_fill_hwcap_from_ext_list();
  -
  -    if ( ret )
  -    {
  -        printk("Falling back to deprecated \"riscv,isa\"\n");
  -        riscv_fill_hwcap_from_isa_string();
  -    }
  +    if ( riscv_fill_hwcap_from_isa_string() )
  +        panic("HW capabilities parsing fro isa string failed\n");
( with dropping of riscv_fill_hwcap_from_ext_list() and adding of return value for riscv_fill_hwcap_from_isa_string() )

> > +    for ( i = 0; i < req_extns_amount; i++ )
> > +    {
> > +        const struct riscv_isa_ext_data ext = required_extensions[i];
> > +
> > +        if ( !riscv_isa_extension_available(NULL, ext.id) )
> > +        {
> > +            printk("Xen requires extension: %s\n", ext.name);
> > +            all_extns_available = false;
> > +        }
> > +    }
> > +
> > +    if ( !all_extns_available )
> > +        panic("Look why the extensions above are needed in booting.txt\n");
> >       
> Where's this booting.txt? I don't think people should be made hunt it
> down ...
>     
I will add ("...docs/misc/riscv/booting.txt\n").

As an other option I could paste here a link to booting.txt ( it will violate code string length but I think it is
fine in the current case ):
 panic("Look why the extensions above are needed in https://gitlab.com/xen-project/xen/-/blob/staging/docs/misc/riscv/booting.txt?ref_type=heads \n");

Thanks.

~ Oleksii