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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2025 18:11, Oleksii Kurochko wrote:
> On 1/8/25 4:21 PM, Jan Beulich wrote:
>> On 23.12.2024 13:55, Oleksii Kurochko wrote:
>>> +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;
>    };

Yes, this is what I'd have expected. I'm afraid ...

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

... I don't really follow all of this. It's clear though that the (char *)
cast is a Misra violation, and hence anything like this is out of question
anyway.

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

Still the doc has '"B" Extension for Bit Manipulation' as the title of the
chapter. And gas accepts B as an extension (e.g. ".option arch, +b").

>>> +            /*
>>> +             * 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 released30 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?

How could there be? At the time of building Xen we know what compiler
version is in use, but we clearly don't know under what qemu versions
it might later be run.

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

Well, you can of course make such an assumption. I don't think though that
it's technically impossible to have an extension-less environment. Xen
won't be able to run there, though (we'll require H at the very least aiui,
and I'm sure we really also require Zicsr).

Jan