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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan