Re: [PATCH RFC v2 06/17] RISC-V: QoS: define CBQRI resctrl resources and domains

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Drew,

(caveat for all my comments)
I scanned this series for resctrl interactions and my comments are
focused around that instead of a thorough review of all the code
within. 

On 1/28/26 12:27 PM, Drew Fustini wrote:
> diff --git a/arch/riscv/kernel/qos/internal.h b/arch/riscv/kernel/qos/internal.h
> index ff2c7eff50be..c0402dd06cfa 100644
> --- a/arch/riscv/kernel/qos/internal.h
> +++ b/arch/riscv/kernel/qos/internal.h
> @@ -65,6 +65,11 @@
>  #define CBQRI_BC_ALLOC_CTL_OP_READ_LIMIT   2
>  #define CBQRI_BC_ALLOC_CTL_STATUS_SUCCESS  1
>  
> +int qos_resctrl_setup(void);
> +void qos_resctrl_exit(void);
> +int qos_resctrl_online_cpu(unsigned int cpu);
> +int qos_resctrl_offline_cpu(unsigned int cpu);
> +
>  /* Capacity Controller hardware capabilities */
>  struct riscv_cbqri_capacity_caps {
>  	u16 ncblks; /* number of capacity blocks */
> @@ -125,4 +130,26 @@ struct cbqri_controller {
>  	bool mon_capable;
>  };
>  
> +struct cbqri_resctrl_res {
> +	struct rdt_resource     resctrl_res;
> +	struct cbqri_controller controller;
> +	u32 max_rcid;
> +	u32 max_mcid;
> +};
> +
> +struct cbqri_resctrl_dom {
> +	struct rdt_domain_hdr       resctrl_dom_hdr;

The resctrl_dom_hdr above should not be needed and indeed does
not seem to be used in this series. The two members below it,
struct rdt_ctrl_domain and struct rdt_mon_domain (now named
rdt_l3_mon_domain), have the necessary struct rdt_domain_hdr as
their first member.

Without any comments or changelog that describes the other members
I am not able to really consider their inclusion here. From the
names it seems that most members are intended to support the
control domain usage so it is not clear to me why the cbqri domain
contains both a control and monitor domain while they are interacted
with separately by resctrl. From what I can tell resctrl_mon_dom
is not used by this implementation.

As hinted above, when you rebase you will find that struct rdt_mon_domain
is now named struct rdt_l3_mon_domain (see commit 4bc3ef46ff41
("x86,fs/resctrl: Rename struct rdt_mon_domain and rdt_hw_mon_domain")) to
make it obvious that it is specific to L3 monitoring. Having this
as a member for all cbqri domains will then become increasingly confusing
since the cbqri_resctrl_dom is also used for L2 cache allocation as well as the
memory bandwidth allocation that are not at L3 scope.

I think it may be simplest to drop the resctrl_mon_dom member until
monitoring is supported?


> +	struct rdt_ctrl_domain  resctrl_ctrl_dom;
> +	struct rdt_mon_domain   resctrl_mon_dom;
> +	u64 cbm;
> +	u64 rbwb;
> +	u64 *ctrl_val;
> +	struct cbqri_controller *hw_ctrl;
> +};
> +
> +struct cbqri_config {
> +	u64 cbm; /* capacity block mask */
> +	u64 rbwb; /* reserved bandwidth blocks */
> +};
> +
>  #endif /* _ASM_RISCV_QOS_INTERNAL_H */
> 

Reinette