Re: [RFC PATCH 2/2] RISC-V: Add support for sqoscfg CSR

[Conor Dooley]

Hey Drew,

(Don't get your hopes up, I don't have anything really meaningful to
contribute, sorry.)

On Sun, Apr 09, 2023 at 09:36:46PM -0700, Drew Fustini wrote:
> Add support for the sqoscfg CSR defined in the Ssqosid ISA extension
> (Supervisor-mode Quality of Service ID). The CSR contains two fields:
> 
>   - Resource Control ID (RCID) used determine resource allocation
>   - Monitoring Counter ID (MCID) used to track resource usage
> 
> Requests from a hart to shared resources like cache will be tagged with
> these IDs. This allows the usage of shared resources to be associated
> with the task currently running on the hart.
> 
> A sqoscfg field is added to thread_struct and has the same format as the
> sqoscfg CSR. This allows the scheduler to set the hart's sqoscfg CSR to
> contain the RCID and MCID for the task that is being scheduled in. The
> sqoscfg CSR is only written to if the thread_struct.sqoscfg is different
> from the current value of the CSR.
> 
> A per-cpu variable cpu_sqoscfg is used to mirror that state of the CSR.
> This is because access to L1D hot memory should be several times faster
> than a CSR read. Also, in the case of virtualization, accesses to this
> CSR are trapped in the hypervisor.
> 
> Link: https://github.com/riscv-non-isa/riscv-cmqri/blob/main/riscv-cbqri.pdf
> Co-developed-by: Kornel Dulęba <mindal@xxxxxxxxxxxx>
> Signed-off-by: Kornel Dulęba <mindal@xxxxxxxxxxxx>
> Signed-off-by: Drew Fustini <dfustini@xxxxxxxxxxxx>
> ---
> Note: the Ssqosid extension and CBQRI spec are still in a draft state.
> The CSR address of sqoscfg is not final.
> 
> Changes from the original patch [1]:
> - Rebase from 6.0 to 6.3
> - Simplify per-cpu variable from struct to u32 with just sqoscfg
> - Move qoscfg to thread_struct in arch/riscv/include/asm/processor.h
>   This avoids changing task_struct in /include/linux/sched.h
> - Reword commit description
> - Reword Kconfig description
> 
> [1] https://github.com/rivosinc/linux/commit/8454b793a62be21d39e5826ef5241dfa06198ba9
> 
>  arch/riscv/Kconfig                 | 19 ++++++++++++++
>  arch/riscv/include/asm/csr.h       |  8 ++++++
>  arch/riscv/include/asm/processor.h |  3 +++
>  arch/riscv/include/asm/qos.h       | 40 ++++++++++++++++++++++++++++++
>  arch/riscv/include/asm/switch_to.h |  2 ++
>  5 files changed, 72 insertions(+)
>  create mode 100644 arch/riscv/include/asm/qos.h
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index cc02eb9eee1f..03f22b7fe34b 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -418,6 +418,25 @@ config RISCV_ISA_SVNAPOT
>  
>  	  If you don't know what to do here, say Y.
>  
> +config RISCV_ISA_SSQOSID
> +	bool "Ssqosid extension support"
> +	default y
> +	help
> +	  Adds support for the Ssqosid ISA extension (Supervisor-mode
> +	  Quality of Service ID).

Could you add "long form" text in brackets here to the bool line, a la:
https://patchwork.kernel.org/project/linux-riscv/patch/20230405-pucker-cogwheel-3a999a94a2f2@wendy/

> +
> +	  Ssqosid defines the sqoscfg CSR which allows the system to tag
> +	  the running process with RCID (Resource Control ID) and MCID
> +	  (Monitoring Counter ID). The RCID is used determine resource
> +	  allocation. The MCID is used to track resource usage in event
> +	  counters.
> +
> +	  For example, a cache controller may use the RCID to apply a
> +	  cache partitioning scheme and use the MCID to track how much
> +	  cache a process, or a group of processes, is using.
> +
> +	  If you don't know what to do here, say Y.
> +
>  config RISCV_ISA_SVPBMT
>  	bool "SVPBMT extension support"
>  	depends on 64BIT && MMU
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index 7c2b8cdb7b77..17d04a0cacd6 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -59,6 +59,13 @@
>  #define SATP_ASID_MASK	_AC(0xFFFF, UL)
>  #endif
>  
> +/* SQOSCFG fields */
> +#define SQOSCFG_RCID_MASK	_AC(0x00000FFF, UL)
> +#define SQOSCFG_MCID_MASK	SQOSCFG_RCID_MASK
> +#define SQOSCFG_MCID_SHIFT	16
> +#define SQOSCFG_MASK		((SQOSCFG_MCID_MASK << SQOSCFG_MCID_SHIFT) | \
> +				  SQOSCFG_RCID_MASK)
> +
>  /* Exception cause high bit - is an interrupt if set */
>  #define CAUSE_IRQ_FLAG		(_AC(1, UL) << (__riscv_xlen - 1))
>  
> @@ -245,6 +252,7 @@
>  #define CSR_STVAL		0x143
>  #define CSR_SIP			0x144
>  #define CSR_SATP		0x180
> +#define CSR_SQOSCFG		0x181
>  
>  #define CSR_STIMECMP		0x14D
>  #define CSR_STIMECMPH		0x15D
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index 94a0590c6971..724b2aa2732d 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -39,6 +39,9 @@ struct thread_struct {
>  	unsigned long s[12];	/* s[0]: frame pointer */
>  	struct __riscv_d_ext_state fstate;
>  	unsigned long bad_cause;
> +#ifdef CONFIG_RISCV_ISA_SSQOSID
> +	u32 sqoscfg;
> +#endif
>  };
>  
>  /* Whitelist the fstate from the task_struct for hardened usercopy */
> diff --git a/arch/riscv/include/asm/qos.h b/arch/riscv/include/asm/qos.h
> new file mode 100644
> index 000000000000..297e7fb64d80
> --- /dev/null
> +++ b/arch/riscv/include/asm/qos.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_RISCV_QOS_H
> +#define _ASM_RISCV_QOS_H
> +
> +#ifdef CONFIG_RISCV_ISA_SSQOSID
> +
> +#include <linux/sched.h>
> +#include <linux/jump_label.h>
> +
> +#include <asm/barrier.h>
> +#include <asm/csr.h>
> +#include <asm/hwcap.h>
> +
> +/* cached value of sqoscfg csr for each cpu */
> +static DEFINE_PER_CPU(u32, cpu_sqoscfg);
> +
> +static void __qos_sched_in(struct task_struct *task)
> +{
> +	u32 *cpu_sqoscfg_ptr = this_cpu_ptr(&cpu_sqoscfg);
> +	u32 thread_sqoscfg;
> +
> +	thread_sqoscfg = READ_ONCE(task->thread.sqoscfg);
> +
> +	if (thread_sqoscfg != *cpu_sqoscfg_ptr) {
> +		*cpu_sqoscfg_ptr = thread_sqoscfg;
> +		csr_write(CSR_SQOSCFG, thread_sqoscfg);
> +	}
> +}
> +
> +static inline void qos_sched_in(struct task_struct *task)

"qos" is a pretty generic prefix, no? Do you think we'd be better off
prefixing this (and every other extension related thing) with `riscv_`?

> +{
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_SSQOSID))
> +		__qos_sched_in(task);
> +}
> +#else
> +
> +static inline void qos_sched_in(struct task_struct *task) {}
> +
> +#endif /* CONFIG_RISCV_ISA_SSQOSID */
> +#endif /* _ASM_RISCV_QOS_H */
> diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
> index 60f8ca01d36e..75d9bfd766af 100644
> --- a/arch/riscv/include/asm/switch_to.h
> +++ b/arch/riscv/include/asm/switch_to.h
> @@ -12,6 +12,7 @@
>  #include <asm/processor.h>
>  #include <asm/ptrace.h>
>  #include <asm/csr.h>
> +#include <asm/qos.h>
>  
>  #ifdef CONFIG_FPU
>  extern void __fstate_save(struct task_struct *save_to);
> @@ -79,6 +80,7 @@ do {							\
>  	if (has_fpu())					\
>  		__switch_to_aux(__prev, __next);	\
>  	((last) = __switch_to(__prev, __next));		\
> +	qos_sched_in(__next);				\

Both FPU and vector do:
|	if (has_fpu())					\
|		__switch_to_fpu(__prev, __next);	\
|	if (has_vector())				\
|		__switch_to_vector(__prev, __next);	\

Is it just my OCD that wants ssqosid to be the same?
It'd also do away with that seems a bit weird to me: having
qos_sched_in() and __qos_sched_in().
Even if you don't make them similar, what's the rationale behind not
inverting the extension check & returning early from a single function.

This is kinda above my pay grade, so let me know what I've inevitably
missed,
Conor.

>  
>  #endif /* _ASM_RISCV_SWITCH_TO_H */
> -- 
> 2.34.1
>

Attachment: signature.asc
Description: PGP signature