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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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