Received-SPF: pass (google.com: domain of linux-kernel+bounces-87276-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 29 Feb 2024 10:23:39 -0800 (PST)
Subject:     Re: [PATCH -fixes v4 2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR
In-Reply-To: <20240228-goldsmith-shrine-97fc4610e0bc@spud>
CC: samuel.holland@sifive.com, ajones@ventanamicro.com, linux-kernel@vger.kernel.org,
  alex@ghiti.fr, linux-riscv@lists.infradead.org, sorear@fastmail.com, stable@vger.kernel.org
From: Palmer Dabbelt <palmer@dabbelt.com>
To: Conor Dooley <conor@kernel.org>
Message-ID: <mhng-94e8034f-eda3-45df-bcf0-1bd5bd9cb869@palmer-ri-x1c9a>
Precedence: bulk
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

On Wed, 28 Feb 2024 02:12:14 PST (-0800), Conor Dooley wrote:
> On Tue, Feb 27, 2024 at 10:55:34PM -0800, Samuel Holland wrote:
>> The [ms]envcfg CSR was added in version 1.12 of the RISC-V privileged
>> ISA (aka S[ms]1p12). However, bits in this CSR are defined by several
>> other extensions which may be implemented separately from any particular
>> version of the privileged ISA (for example, some unrelated errata may
>> prevent an implementation from claiming conformance with Ss1p12). As a
>> result, Linux cannot simply use the privileged ISA version to determine
>> if the CSR is present. It must also check if any of these other
>> extensions are implemented. It also cannot probe the existence of the
>> CSR at runtime, because Linux does not require Sstrict, so (in the
>> absence of additional information) it cannot know if a CSR at that
>> address is [ms]envcfg or part of some non-conforming vendor extension.
>> 
>> Since there are several standard extensions that imply the existence of
>> the [ms]envcfg CSR, it becomes unwieldy to check for all of them
>> wherever the CSR is accessed. Instead, define a custom Xlinuxenvcfg ISA
>> extension bit that is implied by the other extensions and denotes that
>> the CSR exists as defined in the privileged ISA, containing at least one
>> of the fields common between menvcfg and senvcfg.
>
>> This extension does not need to be parsed from the devicetree or ISA
>> string because it can only be implemented as a subset of some other
>> standard extension.
>
> NGL, every time I look at the superset stuff I question whether or not
> it is a good implementation, but it is nice to see that it at least
> makes the creation of quasi-extension flags like this straightforward.

We can always add it to the DT list as a proper extension, but I think 
for this sort of stuff it's good enough for now -- we've already got a 
bunch of complexity for the proper ISA-defined extension dependencies, 
so it's not like we could really get away from it entirely.

> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> Cheers,
> Conor.
>
>
>> 
>> Cc: <stable@vger.kernel.org> # v6.7+
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>> 
>> Changes in v4:
>>  - New patch for v4
>> 
>>  arch/riscv/include/asm/hwcap.h |  2 ++
>>  arch/riscv/kernel/cpufeature.c | 14 ++++++++++++--
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>> index 5340f818746b..1f2d2599c655 100644
>> --- a/arch/riscv/include/asm/hwcap.h
>> +++ b/arch/riscv/include/asm/hwcap.h
>> @@ -81,6 +81,8 @@
>>  #define RISCV_ISA_EXT_ZTSO		72
>>  #define RISCV_ISA_EXT_ZACAS		73
>>  
>> +#define RISCV_ISA_EXT_XLINUXENVCFG	127
>> +
>>  #define RISCV_ISA_EXT_MAX		128
>>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>>  
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index c5b13f7dd482..dacffef68ce2 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -201,6 +201,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>>  	RISCV_ISA_EXT_ZVKB
>>  };
>>  
>> +/*
>> + * While the [ms]envcfg CSRs were not defined until version 1.12 of the RISC-V
>> + * privileged ISA, the existence of the CSRs is implied by any extension which
>> + * specifies [ms]envcfg bit(s). Hence, we define a custom ISA extension for the
>> + * existence of the CSR, and treat it as a subset of those other extensions.
>> + */
>> +static const unsigned int riscv_xlinuxenvcfg_exts[] = {
>> +	RISCV_ISA_EXT_XLINUXENVCFG
>> +};
>> +
>>  /*
>>   * The canonical order of ISA extension names in the ISA string is defined in
>>   * chapter 27 of the unprivileged specification.
>> @@ -250,8 +260,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>  	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>>  	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
>>  	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
>> -	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
>> -	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
>> +	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
>> +	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
>>  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
>>  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
>>  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
>> -- 
>> 2.43.1
>>