Re: [PATCH RFC v3 04/11] RISC-V: QoS: add CBQRI hardware interface

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

Hi Drew,

On 4/14/26 6:53 PM, Drew Fustini wrote:
> diff --git a/arch/riscv/kernel/qos/internal.h b/arch/riscv/kernel/qos/internal.h
> new file mode 100644
> index 000000000000..edbcbd9471b1
> --- /dev/null
> +++ b/arch/riscv/kernel/qos/internal.h

...

> +
> +struct cbqri_config {
> +	u64 cbm; /* capacity block mask */
> +	u64 rbwb; /* reserved bandwidth blocks */
> +};

Is this struct necessary? From what I can tell it is used to pass a parameter to
cbqri_apply_cache_config() and cbqri_apply_bw_config() where each just picks the
one member of interest. Could parameter of interest just be provided directly to
cbqri_apply_cache_config() and cbqri_apply_bw_config() without bouncing it
through this struct first?

> +
> +#endif /* _ASM_RISCV_QOS_INTERNAL_H */
> diff --git a/arch/riscv/kernel/qos/qos_resctrl.c b/arch/riscv/kernel/qos/qos_resctrl.c
> new file mode 100644
> index 000000000000..6d294f2f2504
> --- /dev/null
> +++ b/arch/riscv/kernel/qos/qos_resctrl.c
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#define pr_fmt(fmt) "qos: resctrl: " fmt
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/iopoll.h>
> +#include <linux/ioport.h>
> +#include <linux/resctrl.h>
> +#include <linux/riscv_qos.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <asm/csr.h>
> +#include <asm/qos.h>
> +#include "internal.h"
> +
> +static struct cbqri_resctrl_res cbqri_resctrl_resources[RDT_NUM_RESOURCES];
> +
> +static bool exposed_alloc_capable;
> +/* CDP (code data prioritization) on x86 is AT (access type) on RISC-V */
> +static bool exposed_cdp_l2_capable;
> +static bool exposed_cdp_l3_capable;
> +static bool is_cdp_l2_enabled;
> +static bool is_cdp_l3_enabled;

These CDP values could also be handled as part of struct cbqri_resctrl_res, similar
to how CDP is managed with struct rdt_hw_resource. There does seem to be a hidden
assumption in this implementation that if *any* cache controller at particular level
supports CDP (which will cause global exposed_cdp_l{2,3}_capable to be true) then all cache
controllers at that level are assumed to support CDP even though this property is
enumerated separately for every cache controller making it possible for this
support to not be uniform.

...

> +/* Perform capacity allocation control operation on capacity controller */
> +static int cbqri_cc_alloc_op(struct cbqri_controller *ctrl, int operation, int rcid,
> +			     enum resctrl_conf_type type)
> +{
> +	int reg_offset = CBQRI_CC_ALLOC_CTL_OFF;
> +	int status;
> +	u64 reg;
> +
> +	reg = ioread64(ctrl->base + reg_offset);
> +	reg &= ~CBQRI_CONTROL_REGISTERS_OP_MASK;
> +	reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_OP_MASK, operation);
> +	reg &= ~CBQRI_CONTROL_REGISTERS_RCID_MASK;
> +	reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_RCID_MASK, rcid);
> +
> +	/* CBQRI capacity AT is only supported on L2 and L3 caches for now */
> +	if (ctrl->type == CBQRI_CONTROLLER_TYPE_CAPACITY &&
> +	    ((ctrl->cache.cache_level == 2 && is_cdp_l2_enabled) ||
> +	    (ctrl->cache.cache_level == 3 && is_cdp_l3_enabled))) {
> +		reg &= ~CBQRI_CONTROL_REGISTERS_AT_MASK;
> +		switch (type) {
> +		case CDP_CODE:
> +			reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_AT_MASK,
> +					  CBQRI_CONTROL_REGISTERS_AT_CODE);
> +			break;
> +		case CDP_DATA:
> +		default:
> +			reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_AT_MASK,
> +					  CBQRI_CONTROL_REGISTERS_AT_DATA);
> +			break;
> +		}
> +	}

There does not seem to be any special enabling when resctrl enables CDP via
resctrl_arch_set_cdp_enabled() so I assume CDP is always enabled? Does that mean
that when CDP is disabled from resctrl perspective (thus, both is_cdp_l2_enabled
and is_cdp_l3_enabled are false, but exposed_cdp_l2_capable and exposed_cdp_l3_capable
are true) then the default behavior is that allocation applies to both code and data
and it is not necessary to set both CBQRI_CONTROL_REGISTERS_AT_CODE and
CBQRI_CONTROL_REGISTERS_AT_DATA?


...

> +static int cbqri_probe_controller(struct cbqri_controller *ctrl)
> +{
> +	int err;
> +
> +	pr_debug("controller info: type=%d addr=%pa size=%pa max-rcid=%u max-mcid=%u\n",
> +		 ctrl->type, &ctrl->addr, &ctrl->size,
> +		 ctrl->rcid_count, ctrl->mcid_count);
> +
> +	if (!ctrl->addr) {
> +		pr_warn("%s(): controller has invalid addr=0x0, skipping\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (!request_mem_region(ctrl->addr, ctrl->size, "cbqri_controller")) {
> +		pr_err("%s(): request_mem_region failed for %pa\n",
> +		       __func__, &ctrl->addr);
> +		return -EBUSY;
> +	}
> +
> +	ctrl->base = ioremap(ctrl->addr, ctrl->size);
> +	if (!ctrl->base) {
> +		pr_err("%s(): ioremap failed for %pa\n", __func__, &ctrl->addr);
> +		err = -ENOMEM;
> +		goto err_release;
> +	}
> +
> +	spin_lock_init(&ctrl->lock);
> +
> +	switch (ctrl->type) {
> +	case CBQRI_CONTROLLER_TYPE_CAPACITY:
> +		err = cbqri_probe_cc(ctrl);
> +		break;
> +	case CBQRI_CONTROLLER_TYPE_BANDWIDTH:
> +		err = cbqri_probe_bc(ctrl);
> +		break;
> +	default:
> +		pr_err("unknown controller type %d\n", ctrl->type);
> +		err = -ENODEV;
> +		break;
> +	}
> +
> +	if (err)
> +		goto err_iounmap;
> +
> +	/*
> +	 * max_rmid is used by resctrl_arch_system_num_rmid_idx()
> +	 * Find the smallest mcid_count amongst all controllers.
> +	 */
> +	max_rmid = min(max_rmid, ctrl->mcid_count);

This computation appears to include all controllers, whether they support monitoring or not.
What will ctrl->mcid_count be on a controller that does not support monitoring?

> +
> +	return 0;
> +
> +err_iounmap:
> +	iounmap(ctrl->base);
> +	ctrl->base = NULL;
> +err_release:
> +	release_mem_region(ctrl->addr, ctrl->size);
> +	return err;
> +}

Reinette