Re: [PATCH v2 01/19] riscv: hwprobe: factorize hwprobe ISA extension reporting

[Clément Léger]

On 19/10/2023 12:22, Conor Dooley wrote:
> On Thu, Oct 19, 2023 at 09:26:31AM +0200, Clément Léger wrote:
>>
>>
>> On 18/10/2023 19:36, Conor Dooley wrote:
>>> On Wed, Oct 18, 2023 at 06:33:34PM +0100, Conor Dooley wrote:
>>>> On Wed, Oct 18, 2023 at 10:24:15AM -0700, Evan Green wrote:
>>>>> On Tue, Oct 17, 2023 at 6:15 AM Clément Léger <cleger@xxxxxxxxxxxx> wrote:
>>>>>>
>>>>>> Factorize ISA extension reporting by using a macro rather than
>>>>>> copy/pasting extension names. This will allow adding new extensions more
>>>>>> easily.
>>>>>>
>>>>>> Signed-off-by: Clément Léger <cleger@xxxxxxxxxxxx>
>>>>>> ---
>>>>>>  arch/riscv/kernel/sys_riscv.c | 32 ++++++++++++++++++--------------
>>>>>>  1 file changed, 18 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
>>>>>> index 473159b5f303..e207874e686e 100644
>>>>>> --- a/arch/riscv/kernel/sys_riscv.c
>>>>>> +++ b/arch/riscv/kernel/sys_riscv.c
>>>>>> @@ -145,20 +145,24 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
>>>>>>         for_each_cpu(cpu, cpus) {
>>>>>>                 struct riscv_isainfo *isainfo = &hart_isa[cpu];
>>>>>>
>>>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBA))
>>>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBA;
>>>>>> -               else
>>>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBA;
>>>>>> -
>>>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBB))
>>>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBB;
>>>>>> -               else
>>>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBB;
>>>>>> -
>>>>>> -               if (riscv_isa_extension_available(isainfo->isa, ZBS))
>>>>>> -                       pair->value |= RISCV_HWPROBE_EXT_ZBS;
>>>>>> -               else
>>>>>> -                       missing |= RISCV_HWPROBE_EXT_ZBS;
>>>>>> +#define CHECK_ISA_EXT(__ext)                                                   \
>>>>>> +               do {                                                            \
>>>>>> +                       if (riscv_isa_extension_available(isainfo->isa, __ext)) \
>>>>>> +                               pair->value |= RISCV_HWPROBE_EXT_##__ext;       \
>>>>>> +                       else                                                    \
>>>>>> +                               missing |= RISCV_HWPROBE_EXT_##__ext;           \
>>>>>> +               } while (false)
>>>>>> +
>>>>>> +               /*
>>>>>> +                * Only use CHECK_ISA_EXT() for extensions which can be exposed
>>>>>> +                * to userspace, regardless of the kernel's configuration, as no
>>>>>> +                * other checks, besides presence in the hart_isa bitmap, are
>>>>>> +                * made.
>>>>>
>>>>> This comment alludes to a dangerous trap, but I'm having trouble
>>>>> understanding what it is.
>>>>
>>>> You cannot, for example, use this for communicating the presence of F or
>>>> D, since they require a config option to be set before their use is
>>>> safe.
>>>
>>> Funnily enough, this comment is immediately contradicted by the vector
>>> subset extensions, where these CHECK_ISA_EXT() macros are used wrapped
>>> in has_vector(). The code looks valid to me, since has_vector() contains
>>> the Kconfig check, but does fly in the face of this comment.
> 
>> Yes, the KConfig checks are already done by the headers, adding #ifdef
>> would be redundant even if more coherent with the comment
> 
> I don't really understand what the first part of this means, or why using
> avoidable ifdeffery here would be desirable.

Sorry, I was not clear enough. What I meant is that the has_fpu() and
has_vector() functions are already ifdef'd in headers based on the
KConfig options for their support (CONFIG_FPU/CONFIG_RISCV_ISA_V) So in
the end, using ifdef here in hwprobe_isa_ext0() would be redundant.

> 
>> BTW, wouldn't
>> it make more sense to get rid out of the unsupported extensions directly
>> at ISA string parsing ? ie, if kernel is compiled without V support,
>> then do not set the bits corresponding to these in the riscv_isa_ext[]
>> array ? But the initial intent was probably to be able to report the
>> full string through cpuinfo.
> 
> Yeah, hysterical raisins I guess, it's always been that way. I don't
> think anyone originally thought about such configurations and that is
> how the cpuinfo stuff behaves. I strongly dislike the
> riscv_isa_extension_available() interface, but one of Drew's patches
> does at least improve things a bit. Kinda waiting for some of the
> patches in flight to settle down before deciding if I want to refactor
> stuff to be less of a potential for shooting oneself in the foot.

Make sense.

Clément