Messages in this thread | | | Date | Mon, 17 Apr 2023 13:04:11 -0700 | From | Drew Fustini <> | Subject | Re: [RFC PATCH 2/2] RISC-V: Add support for sqoscfg CSR |
| |
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/
| |