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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.02.2025 17:24, Oleksii Kurochko wrote:
> 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
>  
> <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?

I think so. Just not by way of an assertion.

Jan