[RFC,0/2] Putting some basic order on isa extension stuff

Message

Conor Dooley Nov. 29, 2022, 2:47 p.m. UTC

RFC:
- I have not even tested this, I just did an allmodconfig
- I don't know if I re-ordered something that is sacrosanct
- I don't know if I changed all of the instances
- I didn't write a proper commit message for "patch" 2/2

With those caveats out of the way - all I did here was try to make
things consistent so that it'd be easier to point patch submitters at a
"do this order please".

I never know which of these can be moved without breaking stuff - but
they all seem to be internal use stuff since they're not in uapi?

@drew, I didn't touch the KVM ones - are they re-sortable too? My base
here is rc7 so if you did a reorder at any point there I'd not see it ;)

CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]
CC: [e-mail]

Conor Dooley (2):
  RISC-V: clarify ISA string ordering rules in cpu.c
  RISC-V: resort all extensions in "canonical" order

 arch/riscv/include/asm/hwcap.h |  6 +++---
 arch/riscv/kernel/cpu.c        | 26 +++++++++++++++++++-------
 arch/riscv/kernel/cpufeature.c |  4 ++--
 3 files changed, 24 insertions(+), 12 deletions(-)

Comments

Andrew Jones Nov. 29, 2022, 3:48 p.m. UTC | #1

On Tue, Nov 29, 2022 at 02:47:41PM +0000, Conor Dooley wrote:
> RFC:
> - I have not even tested this, I just did an allmodconfig
> - I don't know if I re-ordered something that is sacrosanct
> - I don't know if I changed all of the instances
> - I didn't write a proper commit message for "patch" 2/2
> 
> With those caveats out of the way - all I did here was try to make
> things consistent so that it'd be easier to point patch submitters at a
> "do this order please".
> 
> I never know which of these can be moved without breaking stuff - but
> they all seem to be internal use stuff since they're not in uapi?
> 
> @drew, I didn't touch the KVM ones - are they re-sortable too? My base
> here is rc7 so if you did a reorder at any point there I'd not see it ;)

Right, we can't touch enum KVM_RISCV_ISA_EXT_ID as that's UAPI. All new
extensions must be added at the bottom. We originally also had to keep
kvm_isa_ext_arr[] in that order, but commit 1b5cbb8733f9 ("RISC-V: KVM:
Make ISA ext mappings explicit") allows us to list its elements in any
order, which means we could sort them in canonical order, if we wanted
to. I think I'd rather have them in alphabetical order, though (they
nearly are at the moment, except for the bottom two...) The only other
place we have ISA extensions listed in KVM is in a switch statement,
which of course doesn't matter, and it's currently in alphabetical order.

Thanks,
drew

Conor Dooley Nov. 29, 2022, 4:50 p.m. UTC | #2

On Tue, Nov 29, 2022 at 04:48:32PM +0100, Andrew Jones wrote:
> On Tue, Nov 29, 2022 at 02:47:41PM +0000, Conor Dooley wrote:
> > RFC:
> > - I have not even tested this, I just did an allmodconfig
> > - I don't know if I re-ordered something that is sacrosanct
> > - I don't know if I changed all of the instances
> > - I didn't write a proper commit message for "patch" 2/2
> > 
> > With those caveats out of the way - all I did here was try to make
> > things consistent so that it'd be easier to point patch submitters at a
> > "do this order please".
> > 
> > I never know which of these can be moved without breaking stuff - but
> > they all seem to be internal use stuff since they're not in uapi?
> > 
> > @drew, I didn't touch the KVM ones - are they re-sortable too? My base
> > here is rc7 so if you did a reorder at any point there I'd not see it ;)
> 
> Right, we can't touch enum KVM_RISCV_ISA_EXT_ID as that's UAPI. All new
> extensions must be added at the bottom. We originally also had to keep
> kvm_isa_ext_arr[] in that order, but commit 1b5cbb8733f9 ("RISC-V: KVM:

Right, I knew that something had been changed in KVM land. It's probably
a good idea to say sort them all alphabetically apart from whichever
ones must be in other orders & explicitly note the reasons in-place.

> Make ISA ext mappings explicit") allows us to list its elements in any
> order, which means we could sort them in canonical order, if we wanted
> to. I think I'd rather have them in alphabetical order, though (they
> nearly are at the moment, except for the bottom two...) The only other
> place we have ISA extensions listed in KVM is in a switch statement,
> which of course doesn't matter, and it's currently in alphabetical order.

I did see the one in uAPI for KVM. Your idea in 2/2 of doing
alphabetical unless otherwise stated works for me as I just want
something concrete! If it works for the chief too, I'll resubmit and
drop the RFC...