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.
Signed-off-by: James Morse <james.morse@arm.com>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 20 ++++++++++++++++----
include/linux/resctrl.h | 1 +
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3d3a839eba6b..56a0bfdc11f7 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1685,13 +1685,16 @@ void resctrl_arch_mon_event_config_write(void *info)
index = mon_event_config_index_get(mon_info->evtid);
if (index == INVALID_CONFIG_INDEX) {
pr_warn_once("Invalid event id %d\n", mon_info->evtid);
+ mon_info->err = -EINVAL;
return;
}
wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
+
+ mon_info->err = 0;
}
-static void mbm_config_write_domain(struct rdt_resource *r,
- struct rdt_domain *d, u32 evtid, u32 val)
+static int mbm_config_write_domain(struct rdt_resource *r,
+ struct rdt_domain *d, u32 evtid, u32 val)
{
struct resctrl_mon_config_info mon_info = {0};
@@ -1704,7 +1707,7 @@ static void mbm_config_write_domain(struct rdt_resource *r,
mon_info.evtid = evtid;
mondata_config_read(&mon_info);
if (mon_info.mon_config == val)
- return;
+ return 0;
mon_info.mon_config = val;
@@ -1716,6 +1719,10 @@ static void mbm_config_write_domain(struct rdt_resource *r,
*/
smp_call_function_any(&d->cpu_mask, resctrl_arch_mon_event_config_write,
&mon_info, 1);
+ if (mon_info.err) {
+ rdt_last_cmd_puts("Invalid event configuration\n");
+ return mon_info.err;
+ }
/*
* When an Event Configuration is changed, the bandwidth counters
@@ -1727,6 +1734,8 @@ static void mbm_config_write_domain(struct rdt_resource *r,
* mbm_local and mbm_total counts for all the RMIDs.
*/
resctrl_arch_reset_rmid_all(r, d);
+
+ return 0;
}
static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
@@ -1734,6 +1743,7 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
char *dom_str = NULL, *id_str;
unsigned long dom_id, val;
struct rdt_domain *d;
+ int err;
/* Walking r->domains, ensure it can't race with cpuhp */
lockdep_assert_cpus_held();
@@ -1765,7 +1775,9 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
list_for_each_entry(d, &r->domains, list) {
if (d->id == dom_id) {
- mbm_config_write_domain(r, d, evtid, val);
+ err = mbm_config_write_domain(r, d, evtid, val);
+ if (err)
+ return err;
goto next;
}
}
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 8a7367d1ce45..6705d7960dfd 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -200,6 +200,7 @@ struct resctrl_mon_config_info {
struct rdt_domain *d;
u32 evtid;
u32 mon_config;
+ int err;
};
/*
--
2.39.2Hi 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?
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
...
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 8a7367d1ce45..6705d7960dfd 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -200,6 +200,7 @@ struct resctrl_mon_config_info {
> struct rdt_domain *d;
> u32 evtid;
> u32 mon_config;
> + int err;
> };
Please take care to use consistent spacing.
Reinette
Hi Reinette, On 09/04/2024 04:23, Reinette Chatre wrote: > 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? Aha, this is done in mon_config_write(), not mbm_config_write_domain(). I'd missed that. I'll drop this patch. Thanks! James
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
>
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
>
> ..
> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index 8a7367d1ce45..6705d7960dfd 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -200,6 +200,7 @@ struct resctrl_mon_config_info {
> > struct rdt_domain *d;
> > u32 evtid;
> > u32 mon_config;
> > + int err;
> > };
>
> Please take care to use consistent spacing.
>
> Reinette
Noted FWIW (though I guess this code might change or go away).
Cheers
---Dave
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
Hi Rainette, On Thu, Apr 11, 2024 at 10:39:37AM -0700, Reinette Chatre wrote: > 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 I guess this will need discussion with James. FWIW, my impression was that the real goal of this patch was to allow a bad event config to be detected at cross-call time and reported asynchronously. Changes elsewhere look to be there just to make error reporting consistent for other existing paths too. Either way, I agree that we really ought be be able to detect and report "bad event config" errors synchronously, unless I'm missing something. I'll discuss with James whether we should be dropping this patch. (For MPAM, I suppose some componets grouped as a single resctrl resource could support event configuration and some not, but I'd hope that we can just not expose event configurability to resctrl at all in that case. A sane system design should not be affected.) Cheers ---Dave > > > > >
Hi Dave,
On 4/17/2024 7:42 AM, Dave Martin wrote:
> Hi Rainette,
>
> On Thu, Apr 11, 2024 at 10:39:37AM -0700, Reinette Chatre wrote:
>> 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
>
> I guess this will need discussion with James. FWIW, my impression was
> that the real goal of this patch was to allow a bad event config to be
> detected at cross-call time and reported asynchronously. Changes
> elsewhere look to be there just to make error reporting consistent for
> other existing paths too.
How do you interpret "bad event config"?
As I understand it, this patch only sets an error in one scenario:
index = mon_event_config_index_get(mon_info->evtid);
if (index == INVALID_CONFIG_INDEX) {
pr_warn_once("Invalid event id %d\n", mon_info->evtid);
mon_info->err = -EINVAL;
return;
}
When will mon_info->evtid be anything but QOS_L3_MBM_TOTAL_EVENT_ID or
QOS_L3_MBM_LOCAL_EVENT_ID?
Reinette
On Wed, Apr 17, 2024 at 10:19:31PM -0700, Reinette Chatre wrote:
> Hi Dave,
>
> On 4/17/2024 7:42 AM, Dave Martin wrote:
> > Hi Rainette,
> >
> > On Thu, Apr 11, 2024 at 10:39:37AM -0700, Reinette Chatre wrote:
> >> 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
> >
> > I guess this will need discussion with James. FWIW, my impression was
> > that the real goal of this patch was to allow a bad event config to be
> > detected at cross-call time and reported asynchronously. Changes
> > elsewhere look to be there just to make error reporting consistent for
> > other existing paths too.
>
> How do you interpret "bad event config"?
>
> As I understand it, this patch only sets an error in one scenario:
>
> index = mon_event_config_index_get(mon_info->evtid);
> if (index == INVALID_CONFIG_INDEX) {
> pr_warn_once("Invalid event id %d\n", mon_info->evtid);
> mon_info->err = -EINVAL;
> return;
> }
>
> When will mon_info->evtid be anything but QOS_L3_MBM_TOTAL_EVENT_ID or
> QOS_L3_MBM_LOCAL_EVENT_ID?
>
> Reinette
I don't know; my reading of this was that since there was a pr_warn()
already, and since James was adding the capability to return an error,
he figured that a suitable error ought to be returned in this case.
But the real reason for the error return mechanism seems to be
resctrl_arch_mon_event_config_write() in the MPAM code, here:
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/commit/?h=mpam/snapshot/v6.7-rc2&id=db0ac51f60675b6c4a54ccd24fa7198ec321c56d
I agree though that if we set mbm_cfg_mask in the rdt_resource at probe
time, the code in mon_config_write() ought to catch such cases cleanly
before making the cross-call. So maybe the new mechanism isn't needed.
I think I need to discuss this with James, to figure out if there's any
reason why that wouldn't work.
Cheers
---Dave
© 2016 - 2026 Red Hat, Inc.