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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.01.2025 13:37, Oleksii Kurochko wrote:
> On 1/20/25 4:13 PM, Jan Beulich wrote:
>> On 15.01.2025 17:36, Oleksii Kurochko wrote:
>>> +#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?

Possibly (see also below). At the very least the code should be self-consistent.

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

I'd be fine with either; all I care about is absence of ambiguity.

>>> +    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() )

That's probably best indeed. The panic() message may then want conditionalizing
upon there actually being "riscv,isa-extension" in DT. If there is, you may want
to say that support for it needs implementing, in place of the above.

>>> +    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 
> inhttps://gitlab.com/xen-project/xen/-/blob/staging/docs/misc/riscv/booting.txt?ref_type=heads
>  \n");

String length is of no real concern. What I'd recommend against though
is to reference anything other than the canonical tree (which lives on
xenbits, not at gitlab). Yet better might be to refer to the produced
documentation
(https://xenbits.xenproject.org/docs/unstable/misc/riscv/booting.txt)
rather than a file in a source tree / git repo.

Jan