Re: [PATCH 5/8] riscv_cbqri: resctrl: Add cache allocation via capacity block mask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Presence of an unsupported cache level causes complete resctrl initialization failure instead of skipping the unknown cache.
- [Low] Enabling CONFIG_RISCV_ISA_SSQOSID and CONFIG_RESCTRL_FS without CONFIG_RISCV_CBQRI_DRIVER results in a build failure due to missing resctrl_arch_* implementations.

Pre-existing issues:
- [High] Software fails to set the BUSY bit when initiating CBQRI control operations, causing the hardware to never execute the command.
--

commit 44771fe47dad2b92018db7b06dd244e6b25cc218
Author: Drew Fustini <fustini@kernel.org>

    riscv_cbqri: resctrl: Add cache allocation via capacity block mask
    
    This commit wires up the CBQRI capacity controllers into the resctrl
    subsystem as RDT_RESOURCE_L2 and RDT_RESOURCE_L3 schemata. It treats
    mismatched capacity controller capabilities at the same cache level as a
    fatal configuration error. It also lazily creates domains in the CPU
    hotplug online callback.

> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index ee586925f9722..9eb65d0eaa070 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -594,6 +594,7 @@ config RISCV_ISA_SSQOSID
>  	bool "Ssqosid extension support for supervisor mode Quality of Service ID"
>  	depends on 64BIT
>  	default n
> +	select ARCH_HAS_CPU_RESCTRL

[Severity: Low]
Does enabling CONFIG_RISCV_ISA_SSQOSID and CONFIG_RESCTRL_FS without
CONFIG_RISCV_CBQRI_DRIVER result in a build failure?

Selecting ARCH_HAS_CPU_RESCTRL here expects the architecture implementations
to be available. However, looking at drivers/resctrl/Kconfig,
RISCV_CBQRI_RESCTRL_FS is only selected if RISCV_CBQRI_DRIVER is enabled.

Could this configuration mismatch cause a link error during the kernel
build because the required resctrl_arch_* hooks are not compiled?

[ ... ]

> diff --git a/drivers/resctrl/cbqri_resctrl.c b/drivers/resctrl/cbqri_resctrl.c
> new file mode 100644
> index 0000000000000..d354129cc34fe
> --- /dev/null
> +++ b/drivers/resctrl/cbqri_resctrl.c
> @@ -0,0 +1,774 @@

[ ... ]

> +static int cbqri_apply_cache_config_dom(struct cbqri_resctrl_dom *hw_dom,
> +					struct rdt_resource *r,
> +					u32 closid, enum resctrl_conf_type t,
> +					u64 cbm)
> +{
> +	struct cbqri_resctrl_res *hw_res =
> +		container_of(r, struct cbqri_resctrl_res, resctrl_res);
> +	struct cbqri_cc_config cfg = {
> +		.cbm = cbm,
> +		.at = (t == CDP_CODE) ? CBQRI_CONTROL_REGISTERS_AT_CODE :
> +					CBQRI_CONTROL_REGISTERS_AT_DATA,
> +		.cdp_enabled = hw_res->cdp_enabled,
> +	};
> +
> +	return cbqri_apply_cache_config(hw_dom->hw_ctrl, closid, &cfg);
> +}

[Severity: High]
This is a pre-existing issue, but does the software fail to set the BUSY bit
when initiating CBQRI control operations in the underlying hardware driver?

This problem was introduced earlier in the same series. Looking at
cbqri_cc_alloc_op() and cbqri_probe_feature() in
drivers/resctrl/cbqri_devices.c, the driver modifies and writes the control
register but omits setting the CBQRI_CONTROL_REGISTERS_BUSY_MASK bit to 1
before issuing the MMIO write.

Because the hardware relies on the BUSY bit to trigger the state machine,
would the operations never start? It seems the subsequent wait-loop
incorrectly succeeds immediately because it polls for BUSY == 0, leading
the driver to silently believe the hardware executed the command.

[ ... ]

> +static int cbqri_resctrl_pick_caches(void)
> +{
> +	struct cbqri_controller *ctrl;
> +
> +	list_for_each_entry(ctrl, &cbqri_controllers, list) {
> +		struct cbqri_resctrl_res *cbqri_res;
> +		int rid;
> +
> +		if (ctrl->type != CBQRI_CONTROLLER_TYPE_CAPACITY)
> +			continue;
> +		if (!ctrl->alloc_capable)
> +			continue;
> +
> +		rid = cbqri_cache_level_to_rid(ctrl->cache.cache_level);
> +		if (rid < 0) {
> +			pr_err("unknown cache level %d\n",
> +			       ctrl->cache.cache_level);
> +			return rid;

[Severity: Medium]
If the device tree specifies a CBQRI controller for an unsupported cache
level like L4, does this entirely abort the setup of the resctrl filesystem?

Instead of returning -ENODEV and causing complete resctrl initialization
failure, would it be better to skip the unknown cache with a continue
statement? Otherwise, this prevents QoS from functioning on perfectly valid
and supported caches like L2 and L3.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619-dfustini-atl-sc-cbqri-dt-v1-0-e79a7723fab0@kernel.org?part=5