Re: [PATCH v10 2/6] riscv_cbqri: resctrl: Add cache allocation via capacity block mask

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] resctrl: cbqri: ZERO_SIZE_PTR deref on CDP mount when rcid_count is 1
- [Medium] resctrl: cbqri: stale cpu_srmcfg_default and CSR_SRMCFG across cpuhp
--

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

riscv_cbqri: resctrl: Add cache allocation via capacity block mask

This commit wires RISC-V CBQRI capacity controllers into resctrl as L2 and
L3 cache resources, managing control domains across CPU hotplug and exposing
capacity block mask configuration.

> diff --git a/drivers/resctrl/cbqri_resctrl.c b/drivers/resctrl/cbqri_resctrl.c
> new file mode 100644
> index 0000000000000..165b0027bbe0f
> --- /dev/null
> +++ b/drivers/resctrl/cbqri_resctrl.c

[ ... ]

> +int resctrl_arch_set_cdp_enabled(enum resctrl_res_level rid, bool enable)
> +{
> +	struct cbqri_resctrl_res *cbqri_res;
> +
> +	if (rid != RDT_RESOURCE_L2 && rid != RDT_RESOURCE_L3)
> +		return -ENODEV;
> +
> +	cbqri_res = &cbqri_resctrl_resources[rid];
> +	if (!cbqri_res->resctrl_res.cdp_capable)
> +		return -ENODEV;
> +
> +	cbqri_res->cdp_enabled = enable;
> +	return 0;
> +}

[ ... ]

> +/*
> + * Note about terminology between x86 (Intel RDT/AMD QoS) and RISC-V:
> + *   CLOSID on x86 is RCID on RISC-V
> + *     RMID on x86 is MCID on RISC-V
> + *
> + * schemata_list_add() halves this count when CDP is enabled because a
> + * CDP CLOSID takes two hardware IDs on x86. CBQRI selects the code or
> + * data allocation with the AT field next to the RCID, so every CLOSID
> + * resctrl hands out under CDP is already a distinct RCID and the RCIDs
> + * from rcid_count/2 upward sit idle. Skipping the halving would need a
> + * resctrl change, so it is left as is.
> + */
> +u32 resctrl_arch_get_num_closid(struct rdt_resource *res)
> +{
> +	struct cbqri_resctrl_res *hw_res;
> +
> +	hw_res = container_of(res, struct cbqri_resctrl_res, resctrl_res);
> +
> +	if (!hw_res->nr_ctrls)
> +		return 0;
> +
> +	return hw_res->ctrls[0]->rcid_count;
> +}

[ ... ]

> +static void cbqri_resctrl_control_init(struct cbqri_resctrl_res *cbqri_res)
> +{
> +	struct rdt_resource *res = &cbqri_res->resctrl_res;
> +	struct cbqri_controller *ctrl;
> +
> +	if (!cbqri_res->nr_ctrls)
> +		return;
> +	ctrl = cbqri_res->ctrls[0];
> +
> +	switch (res->rid) {
> +	case RDT_RESOURCE_L2:
> +	case RDT_RESOURCE_L3:
> +		res->name = (res->rid == RDT_RESOURCE_L2) ? "L2" : "L3";
> +		res->schema_fmt = RESCTRL_SCHEMA_BITMAP;
> +		res->ctrl_scope = (res->rid == RDT_RESOURCE_L2) ?
> +				    RESCTRL_L2_CACHE : RESCTRL_L3_CACHE;
> +		res->cache.cbm_len = ctrl->cc.ncblks;
> +		res->cache.shareable_bits = 0;
> +		res->cache.min_cbm_bits = 1;
> +		res->cache.arch_has_sparse_bitmasks = false;
> +		res->cdp_capable = ctrl->cc.supports_alloc_at_code;
> +		res->alloc_capable = ctrl->alloc_capable;
> +		INIT_LIST_HEAD(&res->ctrl_domains);
> +		INIT_LIST_HEAD(&res->mon_domains);
> +		break;
> +	default:
> +		break;
> +	}
> +}

[Severity: High]
What happens if a CBQRI capacity controller reports rcid_count == 1 and
supports_alloc_at_code == true, and userspace mounts resctrl with CDP
enabled (-o cdp, -o cdpl2, or -o cdpl3)?

In drivers/resctrl/cbqri_devices.c, riscv_cbqri_register_cc() only rejects
rcid_count == 0, so a controller with rcid_count == 1 is accepted. Here,
cbqri_resctrl_control_init() sets res->cdp_capable =
ctrl->cc.supports_alloc_at_code without requiring ctrl->rcid_count >= 2, and
resctrl_arch_set_cdp_enabled() allows enabling CDP whenever res->cdp_capable
is true.

When mounting with CDP enabled, rdt_get_tree() -> schemata_list_create() ->
schemata_list_add() halves num_closid:

    s->num_closid = resctrl_arch_get_num_closid(r);
    if (resctrl_arch_get_cdp_enabled(r->rid))
            s->num_closid /= 2;

With rcid_count == 1, s->num_closid becomes 0. Next, closid_init() computes
rdt_min_closid == 0:

    list_for_each_entry(s, &resctrl_schema_all, list)
            rdt_min_closid = min(rdt_min_closid, s->num_closid);

    closid_free_map = bitmap_alloc(rdt_min_closid, GFP_KERNEL);
    if (!closid_free_map)
            return -ENOMEM;
    bitmap_fill(closid_free_map, rdt_min_closid);

    /* RESCTRL_RESERVED_CLOSID is always reserved for the default group */
    __clear_bit(RESCTRL_RESERVED_CLOSID, closid_free_map);

Because bitmap_alloc(0, GFP_KERNEL) returns ZERO_SIZE_PTR, Could
__clear_bit() immediately fault on ZERO_SIZE_PTR? Should
cbqri_resctrl_control_init() or resctrl_arch_set_cdp_enabled() require
rcid_count >= 2 for CDP?

[ ... ]

> +static int cbqri_resctrl_online_cpu(unsigned int cpu)
> +{
> +	mutex_lock(&cbqri_domain_list_lock);
> +	cbqri_attach_cpu_to_all_ctrls(cpu);
> +	mutex_unlock(&cbqri_domain_list_lock);
> +
> +	/*
> +	 * Seed the per-CPU default RCID/MCID to the reserved (0, 0) pair and
> +	 * notify the resctrl core so it tracks this CPU in the default group.
> +	 */
> +	resctrl_arch_set_cpu_default_closid_rmid(cpu, 0, 0);
> +	resctrl_online_cpu(cpu);
> +	return 0;
> +}
> +
> +static int cbqri_resctrl_offline_cpu(unsigned int cpu)
> +{
> +	resctrl_offline_cpu(cpu);
> +
> +	mutex_lock(&cbqri_domain_list_lock);
> +	cbqri_detach_cpu_from_all_ctrls(cpu);
> +	mutex_unlock(&cbqri_domain_list_lock);
> +	return 0;
> +}

[Severity: Medium]
Can CSR_SRMCFG and per_cpu(cpu_srmcfg, cpu) become out of sync with
per_cpu(cpu_srmcfg_default, cpu) across CPU hotplug?

When a CPU assigned to a non-default resctrl group is taken offline,
cbqri_resctrl_offline_cpu() calls resctrl_offline_cpu(cpu), which removes the
CPU from the group's cpu_mask without resetting cpu_srmcfg_default to (0, 0).
Because the CPU is no longer in the group's cpu_mask or cpu_online_mask, a
subsequent rmdir of that group or unmount of resctrl will not clear
cpu_srmcfg_default on the offline CPU either.

In addition, riscv/srmcfg:online (registered at arch_initcall in
arch/riscv/kernel/qos.c) and cbqri:online (registered at late_initcall in
cbqri_arch_late_init()) both use CPUHP_AP_ONLINE_DYN, so
riscv_srmcfg_reset_cache() runs before cbqri_resctrl_online_cpu() on CPU
online and sets per_cpu(cpu_srmcfg, cpu) = U32_MAX while cpu_srmcfg_default
still holds the stale pre-offline RCID/MCID.

Inside cbqri_resctrl_online_cpu(), cbqri_attach_cpu_to_all_ctrls(cpu) runs
before resctrl_arch_set_cpu_default_closid_rmid(cpu, 0, 0) and performs
operations that can sleep (such as mutex_lock(), kzalloc_obj() with
GFP_KERNEL, and usleep_range() in cbqri_wait_busy_flag()). If a context
switch occurs on the onlining CPU during cbqri_attach_cpu_to_all_ctrls(),
__switch_to_srmcfg() sees cpu_srmcfg == U32_MAX, writes the stale
cpu_srmcfg_default into CSR_SRMCFG, and caches that stale value in
cpu_srmcfg.

When cbqri_resctrl_online_cpu() then calls
resctrl_arch_set_cpu_default_closid_rmid(cpu, 0, 0), it only updates
cpu_srmcfg_default in memory and never calls resctrl_arch_sched_in(current)
to refresh CSR_SRMCFG and cpu_srmcfg on the running CPU.

Should cbqri_resctrl_offline_cpu() reset the CPU's default CLOSID/RMID and
refresh CSR_SRMCFG, and should cbqri_resctrl_online_cpu() set the default to
(0, 0) and call resctrl_arch_sched_in(current) so CSR_SRMCFG and cpu_srmcfg
stay synchronized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-dfustini-atl-sc-cbqri-dt-v10-0-1636a003a3c0@kernel.org?part=2