Re: [PATCH v6 03/18] x86/resctrl: Add PLZA configuration definitions and data structures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] x86/resctrl: silent truncation of closid in msr_pqr_plza_assoc
--

commit 850d244de0af8ce075ea53a011e816d9227fff52
Author: Babu Moger <babu.moger@amd.com>

x86/resctrl: Add PLZA configuration definitions and data structures

Privilege Level Zero Association (PLZA) enables the kernel to switch to
a different CLOSID and/or RMID than those used in user mode when entering
kernel mode. CLOSID and RMID selection are independent and can be
configured separately.

> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index e3cfa0c10e92d..44ecde20b1b43 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h

[ ... ]

> @@ -222,6 +222,55 @@ union l3_qos_abmc_cfg {
>  	unsigned long full;
>  };
>  
> +/*
> + * Privilege Level Zero Association (PLZA) is configured via
> + * MSR_IA32_PQR_PLZA_ASSOC.

[ ... ]

> + * This allows PLZA to override only RMID, only CLOSID, or both. If both
> + * rmid_en and closid_en are clear, the effective behavior is the same as
> + * when PLZA is disabled, with RMID and CLOSID used from MSR_IA32_PQR_ASSOC
> + * irrespective of privilege level.
> + */
> +union msr_pqr_plza_assoc {
> +	struct {
> +		u64	rmid		:12,
> +			reserved1	:19,
> +			rmid_en		: 1,
> +			closid		: 4,
> +			reserved2	:11,

[Severity: High]
Does this correctly size the closid field?

The closid is defined as a 4-bit field, which could cause a regression
through silent data truncation when assigned a value >= 16.

resctrl dynamically discovers the maximum number of CLOSIDs supported by the
hardware via CPUID. If the hardware reports support for more than 16 CLOSIDs,
resctrl will allocate closid values >= 16.

When a resource group with closid >= 16 is assigned to PLZA via
resctrl_arch_configure_kmode_global(), the u32 closid is assigned to the
4-bit split.closid bitfield, silently truncating the upper bits. This
programs an incorrect CLOSID into the MSR, breaking the requested
cache/bandwidth isolation for kernel mode operations without any warning
or error.

If the AMD specification allows a 15-bit CLOSID (like standard
MSR_IA32_PQR_ASSOC), does the struct bitfield incorrectly split 15 bits
into 4 bits for closid and 11 bits for reserved2?

If the hardware is strictly limited to 4 bits, should there be a bounds
check (if (closid > 15)) or a cap on num_closids when PLZA is enabled?

> +			closid_en	: 1,
> +			reserved3	:15,
> +			plza_en		: 1;
> +	} split;
> +	u64	full;
> +};
> +
>  void rdt_ctrl_update(void *arg);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790867441.git.babu.moger@amd.com?part=3