Re: [PATCH v1 2/3] RISC-V: resort all extensions in consistent orders

[Conor.Dooley]

On 01/12/2022 12:29, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Dec 01, 2022 at 10:00:41AM +0100, Andrew Jones wrote:
>> On Wed, Nov 30, 2022 at 11:41:25PM +0000, Conor Dooley wrote:
>>> From: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>>
>>> Ordering between each and every list of extensions is wildly
>>> inconsistent. Per discussion on the lists pick the following policy:
>>>
>>> - The array defining order in /proc/cpuinfo follows a narrow
>>>    interpretation of the ISA specifications, described in a comment
>>>    immediately presiding it.
>>>
>>> - All other lists of extensions are sorted alphabetically.
>>>
>>> This will hopefully allow for easier review & future additions, and
>>> reduce conflicts between patchsets as the number of extensions grows.
>>>
>>> Link: https://lore.kernel.org/all/20221129144742.2935581-2-conor.dooley@xxxxxxxxxxxxx/
>>> Suggested-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
>>> Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
>>> ---
> 
>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>> index 68b2bd0cc3bc..686d41b14206 100644
>>> --- a/arch/riscv/kernel/cpu.c
>>> +++ b/arch/riscv/kernel/cpu.c
>>> @@ -161,12 +161,12 @@ device_initcall(riscv_cpuinfo_init);
>>>    * New entries to this struct should follow the ordering rules described above.
>>>    */
>>>   static struct riscv_isa_ext_data isa_ext_arr[] = {
>>> +   __RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
>>> +   __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>>      __RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>>      __RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>>>      __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
>>>      __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
>>> -   __RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
>>> -   __RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
>>>      __RISCV_ISA_EXT_DATA("", RISCV_ISA_EXT_MAX),
>>>   };
>>
>> Technically we should have leave these in the wrong order if we want to be
>> strict about the ISA string published to userspace, but I'm in favor of
>> changing this array as necessary and hoping we teach userspace to use
>> flexible parsers. Actually, IMO, we shouldn't teach userspace to parse
>> this at all. We should instead create sysfs nodes:
>>
>>   .../isa/zicbom
>>   .../isa/zihintpause
>>   .../isa/sscofpmf
>>
>> and teach userspace to list .../isa/ to learn about extensions. That would
>> also allow us to publish extension version numbers which we are not
>> current doing with the proc isa string.
>>
>>   .../isa/zicbom/major
>>   .../isa/zicbom/minor
>>
>> and we could add other properties if necessary too, e.g.
>>
>>   .../isa/zicbom/block_size
> 
> Yah, this all kinda ties in with Palmer's RFC set that does the hwcap
> stuff. Kinda been holding off on any thoughts on the isa string as a
> valuable anything until that sees a proper respin.
> 
>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>> index 694267d1fe81..8a76a6ce70cf 100644
>>> --- a/arch/riscv/kernel/cpufeature.c
>>> +++ b/arch/riscv/kernel/cpufeature.c
>>> @@ -199,12 +199,13 @@ void __init riscv_fill_hwcap(void)
>>>                              this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
>>>                              set_bit(*ext - 'a', this_isa);
>>>                      } else {
>>> +                           /* sorted alphabetically */
>>>                              SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>>> +                           SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
>>> +                           SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
>>>                              SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
>>>                              SET_ISA_EXT_MAP("zicbom", RISCV_ISA_EXT_ZICBOM);
>>>                              SET_ISA_EXT_MAP("zihintpause", RISCV_ISA_EXT_ZIHINTPAUSE);
>>> -                           SET_ISA_EXT_MAP("sstc", RISCV_ISA_EXT_SSTC);
>>> -                           SET_ISA_EXT_MAP("svinval", RISCV_ISA_EXT_SVINVAL);
>>>                      }
>>>   #undef SET_ISA_EXT_MAP
>>>              }
>>> @@ -284,6 +285,7 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage)
>>>    * This code may also be executed before kernel relocation, so we cannot use
>>>    * addresses generated by the address-of operator as they won't be valid in
>>>    * this context.
>>> + * Tests, unless otherwise required, are to be added in alphabetical order.
>>>    */
>>>   static u32 __init_or_module cpufeature_probe(unsigned int stage)
>>>   {
>>> --
>>> 2.38.1
>>>
>>
>> I realize that I have a suggested-by tag in the commit message, but I
> 
> I did one thing as a "putting it out there" in the responses to another
> series and you suggested something different entirely. Ordinarily, I'd
> not put review comments in a suggested-by, but figured it was okay this
> time.
> 
>> don't really have a strong opinion on how we order extensions where the
>> order doesn't matter. A consistent policy of alphabetical or always at
>> the bottom both work for me. I personally prefer alphabetical when
>> reading the lists, but I realize we'll eventually merge stuff out of
>> order and then that'll generate some churn to reorder (but hopefully not
>> too frequently).
> 
> Think I said it at the yoke yesterday, but I don't think that this is
> much of a problem. If it gets out of order, we just get someone that's
> sending a patchset already to fix things up.
> 
>> My biggest concern is how much we need to care about the order of the
>> string in proc and whether or not we're allowed to fix its order like
>> we're doing with this patch. I hope we can, and I vote we do.
> 
> Being a bit hard-nosed about it:
> - the spec has said for years that this order is not correct
> 
> - their parser cannot assume any given extension is even present, so the
>    index at which the extension starts was only ever going to vary wildly
> 
> - to break a parser, it must expect to see extension Abcd before Efgh &
>    that order has to change for them
> 
> - expecting that a given pair of extensions that appeared one after
>    another would always do so is not something we should worry about
>    breaking as it was always noted in the comment (and by the specs?)
>    that new extensions would be added in alphabetical order (I'd like to
>    think that if a clairvoyant wrote a parser and knew that there'd be
>    nothing in the gap between the extensions we have now & what may be
>    produced they'd also account for this re-ordering...)
> 
> - the re-order of sstc is going to land for v6.1 & the addition of sstc
>    out of order landed in v6.0, so either that is an issue too or this is
>    fine
> 
> I guess I sent the patches, so my opinion is fairly obvious, but I think
> we change it & see if someone complains about an issue that something
> other than a re-jig would break.

typo: s/would/wouldn't/, that changes the meaning of my comment.
If a valid addition would break their parser, that's not really a
"uAPI breakage". It's only something that this re-order would break
but additions or valid change of the string based on cpu capability
would not that we need to worry about IMO.