Re: [PATCH v4 2/7] x86,fs/resctrl: Make 'event_filter' files read only if they're not configurable

[Ben Horgan <ben.horgan@arm.com>]

Hi Reinette,

On 3/27/26 16:13, Reinette Chatre wrote:
> Hi Ben,
> 
> nit: Since there is some precedent for resctrl files changing permissions
> after creation I think it would support this work to make clear that the
> permissions are set before the files are created and as such they are
> created with correct permissions.
> 
> This could just be a simple: "Make" -> "Create" in subject.

Makes sense. I'll update to use "Create".

> 
> On 3/26/26 10:25 AM, Ben Horgan wrote:
>> When the counter assignment mode is mbm_event resctrl assumes the MBM
>> events are configurable and exposes the 'event_filter' files. These files
>> live at info/L3_MON/event_configs/<event>/event_filter and are used to
>> display and set the event configuration.
> 
> <split here into separata paragraphs>

Ack

> 
> The MPAM architecture has support
>> for configuring the memory bandwidth utilization (MBWU) counters to only
>> count reads or only count writes. However, In MPAM, this event filtering
>> support is optional in the hardware (and not yet implemented in the MPAM
>> driver) but MBM counter assignment is always possible for MPAM MBWU
>> counters.
>>
>> In order to support mbm_event mode with MPAM, make the 'event_filter' files
> 
> "make" -> "create"

Ack

> 
>> read only if the event configuration can't be changed. A user can still
>> chmod the file and so also return early with an error from
>> event_filter_write().
> 
> I went back-and-forth a few times on whether we should add a permissions
> check to rdtgroup_file_write() to not call rft->write() (and thus event_filter_write())
> at all if the file is not writable. In the end I think this patch is ok since
> there is the last_cmd_status help to give insight into why user is unable to
> write to the file that may be writable under other circumstances. Any opinions
> welcome here. 
> 
>>
>> Introduce a new monitor property, mbm_cntr_configurable, to indicate
>> whether or not assignable MBM counters are configurable. On x86, set this
>> to true whenever mbm_cntr_assignable is true to keep existing behaviour.
>>
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>> Changes since v2:
>> Use property, mbm_cntr_configurable, rather than arch hook
>> Change the event_filter mode to read only in res_common_files[]
>> Add resctrl_file_mode_init() and use in resctrl_l3_mon_resource_init()
>> set mbm_cntr_configurable for x86 ABMC and mention in commit message
>> ---
>>  arch/x86/kernel/cpu/resctrl/monitor.c |  1 +
>>  fs/resctrl/internal.h                 |  2 ++
>>  fs/resctrl/monitor.c                  |  7 +++++++
>>  fs/resctrl/rdtgroup.c                 | 11 ++++++++++-
>>  include/linux/resctrl.h               | 16 +++++++++-------
>>  5 files changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 9bd87bae4983..794a6fb175e4 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -454,6 +454,7 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r)
>>  	    (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL) ||
>>  	     rdt_cpu_has(X86_FEATURE_CQM_MBM_LOCAL))) {
>>  		r->mon.mbm_cntr_assignable = true;
>> +		r->mon.mbm_cntr_configurable = true;
>>  		cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
>>  		r->mon.num_mbm_cntrs = (ebx & GENMASK(15, 0)) + 1;
>>  		hw_res->mbm_cntr_assign_enabled = true;
>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index 1a9b29119f88..48af75b9dc85 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -408,6 +408,8 @@ void __check_limbo(struct rdt_l3_mon_domain *d, bool force_free);
>>  
>>  void resctrl_file_fflags_init(const char *config, unsigned long fflags);
>>  
>> +void resctrl_file_mode_init(const char *config, umode_t mode);
>> +
>>  void rdt_staged_configs_clear(void);
>>  
>>  bool closid_allocated(unsigned int closid);
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index 49f3f6b846b2..8fec3dea33c3 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -1420,6 +1420,11 @@ ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes
>>  		ret = -EINVAL;
>>  		goto out_unlock;
>>  	}
>> +	if (!r->mon.mbm_cntr_configurable) {
>> +		rdt_last_cmd_puts("event_filter is not configurable\n");
>> +		ret = -EPERM;
>> +		goto out_unlock;
>> +	}
>>  
>>  	ret = resctrl_parse_mem_transactions(buf, &evt_cfg);
>>  	if (!ret && mevt->evt_cfg != evt_cfg) {
>> @@ -1884,6 +1889,8 @@ int resctrl_l3_mon_resource_init(void)
>>  		resctrl_file_fflags_init("available_mbm_cntrs",
>>  					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
>>  		resctrl_file_fflags_init("event_filter", RFTYPE_ASSIGN_CONFIG);
>> +		if (r->mon.mbm_cntr_configurable)
>> +			resctrl_file_mode_init("event_filter", 0644);
>>  		resctrl_file_fflags_init("mbm_assign_on_mkdir", RFTYPE_MON_INFO |
>>  					 RFTYPE_RES_CACHE);
>>  		resctrl_file_fflags_init("mbm_L3_assignments", RFTYPE_MON_BASE);
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 4753841c2ca3..fa5712db3778 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -2020,7 +2020,7 @@ static struct rftype res_common_files[] = {
>>  	},
>>  	{
>>  		.name		= "event_filter",
>> -		.mode		= 0644,
>> +		.mode		= 0444,
>>  		.kf_ops		= &rdtgroup_kf_single_ops,
>>  		.seq_show	= event_filter_show,
>>  		.write		= event_filter_write,
>> @@ -2213,6 +2213,15 @@ void resctrl_file_fflags_init(const char *config, unsigned long fflags)
>>  		rft->fflags = fflags;
>>  }
>>  
>> +void resctrl_file_mode_init(const char *config, umode_t mode)
>> +{
>> +	struct rftype *rft;
>> +
>> +	rft = rdtgroup_get_rftype_by_name(config);
>> +	if (rft)
>> +		rft->mode = mode;
>> +}
>> +
>>  /**
>>   * rdtgroup_kn_mode_restrict - Restrict user access to named resctrl file
>>   * @r: The resource group with which the file is associated.
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index 006e57fd7ca5..06e8c72e8660 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -286,13 +286,14 @@ enum resctrl_schema_fmt {
>>  
>>  /**
>>   * struct resctrl_mon - Monitoring related data of a resctrl resource.
>> - * @num_rmid:		Number of RMIDs available.
>> - * @mbm_cfg_mask:	Memory transactions that can be tracked when bandwidth
>> - *			monitoring events can be configured.
>> - * @num_mbm_cntrs:	Number of assignable counters.
>> - * @mbm_cntr_assignable:Is system capable of supporting counter assignment?
>> - * @mbm_assign_on_mkdir:True if counters should automatically be assigned to MBM
>> - *			events of monitor groups created via mkdir.
>> + * @num_rmid:			Number of RMIDs available.
>> + * @mbm_cfg_mask:		Memory transactions that can be tracked when
>> + *				bandwidth monitoring events can be configured.
>> + * @num_mbm_cntrs:		Number of assignable counters.
>> + * @mbm_cntr_assignable:	Is system capable of supporting counter assignment?
>> + * @mbm_assign_on_mkdir:	True if counters should automatically be assigned to MBM
>> + *				events of monitor groups created via mkdir.
>> + * @mbm_cntr_configurable:	True if assignable counters are configurable.
>>   */
>>  struct resctrl_mon {
>>  	u32			num_rmid;
>> @@ -300,6 +301,7 @@ struct resctrl_mon {
>>  	int			num_mbm_cntrs;
>>  	bool			mbm_cntr_assignable;
>>  	bool			mbm_assign_on_mkdir;
>> +	bool			mbm_cntr_configurable;
>>  };
>>  
>>  /**
> 
> The above looks good to me. The documentation does still specifically state that "event_filter"
> is read/write so it needs a change to match. For example, like something below but please feel
> free to improve:
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 3ec6b3b1b603..70a4ae89d8e1 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -419,9 +419,9 @@ with the following files:
>  
>  	Two MBM events are supported by default: mbm_local_bytes and mbm_total_bytes.
>  	Each MBM event's sub-directory contains a file named "event_filter" that is
> -	used to view and modify which memory transactions the MBM event is configured
> -	with. The file is accessible only when "mbm_event" counter assignment mode is
> -	enabled.
> +	used to view and (if writable) modify which memory transactions the MBM
> +	event is configured with. The file is accessible only when "mbm_event" counter
> +	assignment mode is enabled.
>  
>  	List of memory transaction types supported:
>  
> @@ -446,9 +446,8 @@ with the following files:
>  	  # cat /sys/fs/resctrl/info/L3_MON/event_configs/mbm_local_bytes/event_filter
>  	  local_reads,local_non_temporal_writes,local_reads_slow_memory
>  
> -	Modify the event configuration by writing to the "event_filter" file within
> -	the "event_configs" directory. The read/write "event_filter" file contains the
> -	configuration of the event that reflects which memory transactions are counted by it.
> +	The memory transactions the MBM event is configured with can be changed
> +	if "event_filter" is writable.

Thanks for the wording. It reads well to me.

Ben

>  
>  	For example::
>  
> Reinette
> 
> 
> 
>