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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6057 bytes --]


On 1/14/25 8:33 AM, Jan Beulich wrote:
>>>> +    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").

I think it is fine.
B, in this case, just represents Zba, Zbb, Zbc, Zbs and that all of them are supported at the same time.
But I see chips that doesn't have B because it doesn't have one of those extensions.

>
>>>> +            /*
>>>> +             * 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@irq.a4lg.com/#t
>>
>> This leads to the following fix in QEMU:
>> https://patchwork.kernel.org/project/qemu-devel/patch/dee09d708405075420b29115c1e9e87910b8da55.1648270894.git.research_trasio@irq.a4lg.com/#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.

Agree with that, there is no any sense for having something similar as GCC_VERSIOB but
for QEMU. Then I will just update the comment around this workaround with some clarifications.

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

I would like to clarify some things. I think we are counting by word 'extension' different things.
I am including to this `Base ISA` ( and likely it is incorrect to do so ( or,at least, confusing )
and I will try not to do that in the future. I am using it in this manner because `Base ISA` is
included to the table 74. Standard ISA extension names in Unpriv spec ) then it is impossible to
have an extension-less environment because the spec mentions the following:

     A RISC-V ISA is defined as a base integer ISA, which must be present in any implementation, plus
     optional extensions to the base ISA.

So, at least, r{32,64,128}i should written in riscv,isa property of DTS file and that is the reason why
this_isa can't be empty, and thereby riscv_isa will be initialized with, at least, `i` ending up with only `i`
supported by all CPUs.

But if not count `I` as an extension and just as base ISA then it is really technically possible to come up with
extension-less environment. But anyway as you mentioned we still need for Xen some extensions. ( btw, thanks, I missed to
add Zicsr to required_extensions[] ).

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 8190 bytes --]