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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/3/25 5:03 PM, Jan Beulich wrote:

> On 03.02.2025 16:05, Oleksii Kurochko wrote:
> > On 1/27/25 3:47 PM, Jan Beulich wrote:
> > > > +static bool is_lowercase_extension_name(const char *str)
> > > > +{
> > > > +    /*
> > > > +     * `str` could contain full riscv,isa string from device tree so one
> > > > +     * of the stop condionitions is checking for '_' as extensions are
> > > > +     * separated by '_'.
> > > > +     */
> > > > +    for ( unsigned int i = 0; (str[i] != '\0') && (str[i] != '_'); i++ )
> > > > +        if ( !islower(str[i]) )
> > > > +            return false;
> > > > +
> > > > +    return true;
> > > > +}
> > > > +
> > > > +static void __init match_isa_ext(const char *name, const char *name_end,
> > > > +                                 unsigned long *bitmap)
> > > > +{
> > > > +    const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
> > > > +
> > > > +    for ( unsigned int i = 0; i < riscv_isa_ext_count; i++ )
> > > > +    {
> > > > +        const struct riscv_isa_ext_data *ext = &riscv_isa_ext[i];
> > > > +
> > > > +        /*
> > > > +         * `name` (according to device tree binding) and
> > > > +         * `ext->name` (according to initialization of riscv_isa_ext[]
> > > > +         * elements) must be all in lowercase.
> > > > +         *
> > > > +         * Just to be sure that it is true, ASSERT() is added.
> > > > +         */
> > > > +        ASSERT(is_lowercase_extension_name(name) &&
> > > > +               is_lowercase_extension_name(ext->name));
> > > >           
> > > More general remark: While asserting on ext->name is okay, for it being
> > > our own data, asserting on data coming from the outside is generally not
> > > correct. For now I'm not going to insist on this being changed, but
> > > sooner or later it will want revisiting
> > >         
> > IIUC it would be better to leave ASSERT(is_lowercase_extension_name(ext->name)) in match_isa_ext()
> > and put ASSERT(is_lowercase_extension_name(ext) in riscv_isa_parse_string() before match_isa_ext()
> > is called:
> >    static int __init riscv_isa_parse_string(const char *isa,
> >                                             unsigned long *out_bitmap)
> >    {
> >      ...
> >      while ( *isa )
> >      {
> >        const char *ext = isa++;
> >      ...
> >      ASSERT(is_lowercase_extension_name(ext));
> >      match_isa_ext(ext, ext_end, out_bitmap);
> >    }
> >
> > Is my understanding correct?
> >       
> That depends on the origin of the incoming "isa". Considering the function
> wants to parse it, I'd expect it still comes from DT. In which case
> asserting on it is wrong; anything may come from there, and nothing should
> cause assertion failures. Recall that assertions are checks of _our own
> internal state_ only.
>     
But based on the device tree binding ( https://elixir.bootlin.com/linux/v6.13.1/source/Documentation/devicetree/bindings/riscv/extensions.yaml#L47 ),
not anything should come from DT for the riscv,isa string; only lowercase letters are allowed.
I am not sure if it makes sense to double-check if riscv,isa is correct, as my expectation (which I haven’t checked yet) is that the DTS will
be validated during compilation.

Does it make sense to double check what was put in DT's riscv,isa?

As an option, I think I could simply convert the riscv,isa value obtained from the device tree to lowercase and then remove the ASSERT() for the DT’s
ISA property altogether. This way, it won’t really matter what is placed in the riscv,isa property. Even if riscv,isa mustn't have only lowercase letters
(according to the bindings) I would anyway to convert everything to lowercase to simplify parser.

> > > > +static int __init riscv_isa_parse_string(const char *isa,
> > > > +                                         unsigned long *out_bitmap)
> > > > +{
> > > > +    if ( (isa[0] != 'r') && (isa[1] != 'v') )
> > > > +        return -EINVAL;
> > > > +
> > > > +#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"
> > > >           
> > > Nit: We generally like to have the # in the first column, and - if
> > > so desired - blank padding afterwards.
> > >         
> > Should it be done only when "#if defined" used inside function or blank padding is needed only in
> > case when "#if defined" is used and, for example, for "#ifdef" such padding isn't needed?
> >       
> I fear I don't understand the question; I see no connection to #ifdef vs
> #if defined(). Any blanks after # are generally up to the author's taste
> (unless the result is really unwieldy), as we have no style rule for that.
> There are pros and cons towards the use of such padding.
>     
Got it. I just thought that sometimes padding is used and sometimes not, so decided that some "rule"
exist.

Thanks.

~ Oleksii