Received-SPF: pass (google.com: domain of linux-kernel+bounces-213706-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d8575593-588d-406e-bcd3-eaec93e43e2c@sifive.com>
Date: Thu, 13 Jun 2024 11:59:55 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/3] riscv: Add support for per-thread envcfg CSR values
To: Deepak Gupta <debug@rivosinc.com>
Cc: linux-riscv@lists.infradead.org, Palmer Dabbelt <palmer@dabbelt.com>,
 Andrew Jones <ajones@ventanamicro.com>, Conor Dooley <conor@kernel.org>,
 linux-kernel@vger.kernel.org
References: <20240605205658.184399-1-samuel.holland@sifive.com>
 <20240605205658.184399-3-samuel.holland@sifive.com>
 <ZmOCpF5ACdQiSfcm@debug.ba.rivosinc.com>
Content-Language: en-US
From: Samuel Holland <samuel.holland@sifive.com>
In-Reply-To: <ZmOCpF5ACdQiSfcm@debug.ba.rivosinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi Deepak,

On 2024-06-07 4:59 PM, Deepak Gupta wrote:
> On Wed, Jun 05, 2024 at 01:56:46PM -0700, Samuel Holland wrote:
>> Some bits in the [ms]envcfg CSR, such as the CFI state and pointer
>> masking mode, need to be controlled on a per-thread basis. Support this
>> by keeping a copy of the CSR value in struct thread_struct and writing
>> it during context switches. It is safe to discard the old CSR value
>> during the context switch because the CSR is modified only by software,
>> so the CSR will remain in sync with the copy in thread_struct.
>>
>> Use ALTERNATIVE directly instead of riscv_has_extension_unlikely() to
>> minimize branchiness in the context switching code.
>>
>> Since thread_struct is copied during fork(), setting the value for the
>> init task sets the default value for all other threads.
>>
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>>
>> arch/riscv/include/asm/processor.h | 1 +
>> arch/riscv/include/asm/switch_to.h | 8 ++++++++
>> arch/riscv/kernel/cpufeature.c     | 2 +-
>> 3 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/include/asm/processor.h
>> b/arch/riscv/include/asm/processor.h
>> index 68c3432dc6ea..0838922bd1c8 100644
>> --- a/arch/riscv/include/asm/processor.h
>> +++ b/arch/riscv/include/asm/processor.h
>> @@ -118,6 +118,7 @@ struct thread_struct {
>>     unsigned long s[12];    /* s[0]: frame pointer */
>>     struct __riscv_d_ext_state fstate;
>>     unsigned long bad_cause;
>> +    unsigned long envcfg;
>>     u32 riscv_v_flags;
>>     u32 vstate_ctrl;
>>     struct __riscv_v_ext_state vstate;
>> diff --git a/arch/riscv/include/asm/switch_to.h
>> b/arch/riscv/include/asm/switch_to.h
>> index 7594df37cc9f..9685cd85e57c 100644
>> --- a/arch/riscv/include/asm/switch_to.h
>> +++ b/arch/riscv/include/asm/switch_to.h
>> @@ -70,6 +70,13 @@ static __always_inline bool has_fpu(void) { return false; }
>> #define __switch_to_fpu(__prev, __next) do { } while (0)
>> #endif
>>
>> +static inline void __switch_to_envcfg(struct task_struct *next)
>> +{
>> +    asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0",
>> +                  0, RISCV_ISA_EXT_XLINUXENVCFG, 1)
>> +            :: "r" (next->thread.envcfg) : "memory");
>> +}
>> +
>> extern struct task_struct *__switch_to(struct task_struct *,
>>                        struct task_struct *);
>>
>> @@ -103,6 +110,7 @@ do {                            \
>>         __switch_to_vector(__prev, __next);    \
>>     if (switch_to_should_flush_icache(__next))    \
>>         local_flush_icache_all();        \
>> +    __switch_to_envcfg(__next);            \
>>     ((last) = __switch_to(__prev, __next));        \
>> } while (0)
> 
> Suggestion:
> Probably make this patch 1
> 
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 2879e26dbcd8..1153b96346ae 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -728,7 +728,7 @@ unsigned long riscv_get_elf_hwcap(void)
>> void riscv_user_isa_enable(void)
>> {
>>     if (riscv_has_extension_unlikely(RISCV_ISA_EXT_ZICBOZ))
>> -        csr_set(CSR_ENVCFG, ENVCFG_CBZE);
>> +        current->thread.envcfg |= ENVCFG_CBZE;
> 
> Suggestion:
> Squash this with current patch 1 and call it patch 2.

This reorganization doesn't work. If I add __switch_to_envcfg() first without
this change, then the CSR would get zeroed out during the first context switch,
so userspace cbo.zero would be broken after the first patch.

Regards,
Samuel

>>     else if (any_cpu_has_zicboz)
>>         pr_warn_once("Zicboz disabled as it is unavailable on some harts\n");
>> }
>> -- 
>> 2.44.1
>>