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:   Mon, 17 Apr 2023 13:04:11 -0700
From:   Drew Fustini <dfustini@baylibre.com>
To:     Conor Dooley <conor@kernel.org>
Cc:     linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Conor Dooley <conor.dooley@microchip.com>,
        Atish Patra <atishp@rivosinc.com>,
        =?iso-8859-1?Q?Bj=F6rn_T=F6pel?= <bjorn@rivosinc.com>,
        James Morse <james.morse@arm.com>,
        Kornel =?utf-8?Q?Dul=C4=99ba?= <mindal@semihalf.com>,
        Adrien Ricciardi <aricciardi@baylibre.com>
Subject: Re: [RFC PATCH 2/2] RISC-V: Add support for sqoscfg CSR
Message-ID: <ZD2mO/2BH9Zo3iqO@x1>
References: <20230410043646.3138446-1-dfustini@baylibre.com>
 <20230410043646.3138446-3-dfustini@baylibre.com>
 <20230417-culinary-capillary-4f1efd496691@spud>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230417-culinary-capillary-4f1efd496691@spud>
Precedence: bulk

On Mon, Apr 17, 2023 at 08:10:20PM +0100, Conor Dooley wrote:
> Hey Drew,
> 
> (Don't get your hopes up, I don't have anything really meaningful to
> contribute, sorry.)

Hi, thanks for the feedback...

> 
> 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@semihalf.com>
> > Signed-off-by: Kornel Dulęba <mindal@semihalf.com>
> > Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> > ---
> > 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/

Thanks, that is a good idea. It would make it easier for the person
using menuconfig as they won't have to access the help screen just to
see what the acronym stands for.

> > +
> > +	  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_`?

That's a fair point. No harm in adding the 'riscv_' to make the context
clearer.

> 
> > +{
> > +	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?

Thanks, I had not noticed those changes from the vector patch series [1]
until you pointed it out. The handling of sqoscfg could be converted to
that pattern too.

> 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.

The goal was so the inline qos_sched_in() would turn into a nop when
Ssqosid extensions not present. If Ssqosid was present, then the "real"
function __qos_sched_in() would be called.

However, having looked at the handling of fpu and vector in the vector
series, I think will redo the sqoscfg handling to follow that pattern.

thanks,
drew

[1] https://lore.kernel.org/linux-riscv/20230414155843.12963-1-andy.chiu@sifive.com/