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
