Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 29 Nov 2022 17:35:19 +0100
From:   Andrew Jones <ajones@ventanamicro.com>
To:     Conor Dooley <conor.dooley@microchip.com>
Cc:     linux-riscv@lists.infradead.org, aou@eecs.berkeley.edu,
        conor@kernel.org, devicetree@vger.kernel.org, guoren@kernel.org,
        heiko@sntech.de, krzysztof.kozlowski+dt@linaro.org,
        linux-kernel@vger.kernel.org, palmer@dabbelt.com,
        paul.walmsley@sifive.com, robh+dt@kernel.org
Subject: Re: [RFC 2/2] RISC-V: resort all extensions in "canonical" order
Message-ID: <20221129163519.mzgn3nrfiguc2v2s@kamzik>
References: <Y4XvnHIPw8ZuBZEk@wendy>
 <20221129144742.2935581-3-conor.dooley@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221129144742.2935581-3-conor.dooley@microchip.com>
Precedence: bulk

On Tue, Nov 29, 2022 at 02:47:43PM +0000, Conor Dooley wrote:
> Per the comment in cpu.c, re-sort all lists/tables/enums/whatever in
> arch/riscv (apart from KVM) in the current edition of what the isa
> manual considers to be "canonical" order.
> 
> None of this is in uapi, so we are free to re-order it? I'm never sure
> when it comes to hwcap...
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  arch/riscv/include/asm/hwcap.h | 6 +++---
>  arch/riscv/kernel/cpu.c        | 4 ++--
>  arch/riscv/kernel/cpufeature.c | 4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index b22525290073..d7d5f27619ee 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -53,12 +53,12 @@ extern unsigned long elf_hwcap;
>   * available logical extension id.
>   */
>  enum riscv_isa_ext_id {
> -	RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
> -	RISCV_ISA_EXT_SVPBMT,
> -	RISCV_ISA_EXT_ZICBOM,
> +	RISCV_ISA_EXT_ZICBOM = RISCV_ISA_EXT_BASE,
>  	RISCV_ISA_EXT_ZIHINTPAUSE,
> +	RISCV_ISA_EXT_SSCOFPMF,
>  	RISCV_ISA_EXT_SSTC,
>  	RISCV_ISA_EXT_SVINVAL,
> +	RISCV_ISA_EXT_SVPBMT,
>  	RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,

Or,

@@ -48,17 +48,16 @@ extern unsigned long elf_hwcap;
 /*
  * This enum represent the logical ID for each multi-letter RISC-V ISA extension.
  * The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed
- * RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter
- * extensions while all the multi-letter extensions should define the next
- * available logical extension id.
+ * RISCV_ISA_EXT_MAX. While the order doesn't matter, we keep it sorted
+ * alphabetically for neatness.
  */
 enum riscv_isa_ext_id {
        RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
+       RISCV_ISA_EXT_SSTC,
+       RISCV_ISA_EXT_SVINVAL,
        RISCV_ISA_EXT_SVPBMT,
        RISCV_ISA_EXT_ZICBOM,
        RISCV_ISA_EXT_ZIHINTPAUSE,
-       RISCV_ISA_EXT_SSTC,
-       RISCV_ISA_EXT_SVINVAL,
        RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX,
 };


>  };
>  
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index 5e42c92a8456..1d0fa0ebf6a8 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -150,12 +150,12 @@ device_initcall(riscv_cpuinfo_init);
>   *    extensions by an underscore.
>   */
>  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),
>  };

Yes, this one should be put in canonical order (I guess).

>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 694267d1fe81..d3df72c4b94f 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -199,12 +199,12 @@ void __init riscv_fill_hwcap(void)
>  				this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
>  				set_bit(*ext - 'a', this_isa);
>  			} else {
> -				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
> -				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("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);

Or,

@@ -199,12 +199,16 @@ void __init riscv_fill_hwcap(void)
                                this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
                                set_bit(*ext - 'a', this_isa);
                        } else {
+                               /*
+                                * While the order doesn't matter here, we sort
+                                * alphabetically for neatness.
+                                */
                                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
                }

>  			}
>  #undef SET_ISA_EXT_MAP
>  		}
> -- 
> 2.38.1
>

Thanks,
drew