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.
