Messages in this thread | | | Date | Thu, 11 Apr 2024 10:39:37 -0700 | Subject | Re: [PATCH v1 18/31] x86/resctrl: Allow resctrl_arch_mon_event_config_write() to return an error | From | Reinette Chatre <> |
| |
Hi Dave,
On 4/11/2024 7:17 AM, Dave Martin wrote: > On Mon, Apr 08, 2024 at 08:23:36PM -0700, Reinette Chatre wrote: >> Hi James, >> >> On 3/21/2024 9:50 AM, James Morse wrote: >>> resctrl_arch_mon_event_config_write() writes a bitmap of events provided >>> by user-space into the configuration register for the monitors. >>> >>> This assumes that all architectures support all the features each bit >>> corresponds to. >>> >>> MPAM can filter monitors based on read, write, or both, but there are >>> many more options in the existing bitmap. To allow this interface to >>> work for machines with MPAM, allow the architecture helper to return >>> an error if an incompatible bitmap is set. >>> >>> When valid values are provided, there is no change in behaviour. If >>> an invalid value is provided, currently it is silently ignored, but >>> last_cmd_status is updated. After this change, the parser will stop >>> at the first invalid value and return an error to user-space. This >>> matches the way changes to the schemata file are made. >>> >> >> Is this needed? With move of mbm_cfg_mask to rdt_resource I expect >> MPAM would use it to set what the valid values are. With that done, >> when user space provides a value, mon_config_write() compares user >> provided value against mbm_cfg_mask and will already return early >> (before attempting to write to hardware) with error >> if value is not supported. This seems to accomplish the goal of this >> patch? > > This sounds plausible. > > In a recent snapshot of James' MPAM code, it looks like we could be > initialising rdt_resource::mbm_cfg_mask when setting up the rdt_resource > struct for resctrl, though in fact this information is captured > differently right now. I'm sure why (though James may have a > reason). [1] > > I don't see an obvious reason though why we couldn't set mbm_cfg_mask > and detect bad config values globally in mon_config_write(), the same as > for the existing AMD BMEC case. > > Nothing in the MPAM architecture stops hardware vendors from randomly > implementing different capabilities in different components of the > system, but provided that we only expose the globally supported subset > of event filtering capabilities to resctrl this approach looks workable. > This consistent with the James' MPAM code deals with other feature > mismatches across the system today. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/mpam_resctrl.c?h=mpam/snapshot/v6.7-rc2#n730
My response was based on what I understood from the goal of this change as described by the changelog. The patch does not appear to match with the goals stated in changelog.
As I understand the patch it aims to detect when there is an invalid event id. It is not possible for this scenario to occur because this code is always called with a valid event id.
Reinette
| |