If the BMEC (Bandwidth Monitoring Event Configuration) feature is
supported, the bandwidth events can be configured. Currently, the maximum
supported event bitmask is hard-coded. This information can be detected by
the CPUID_Fn80000020_ECX_x03.
CPUID_Fn80000020_ECX_x03 [Platform QoS Monitoring Bandwidth Event
Configuration] Read-only. Reset: 0000_007Fh.
Bits Description
31:7 Reserved
6:0 Identifies the bandwidth sources that can be tracked.
Remove the hardcoded value and detect using CPUID command.
The CPUID details are documentation in the PPR listed below [1].
[1] Processor Programming Reference (PPR) Vol 1.1 for AMD Family 19h Model
11h B1 - 55901 Rev 0.25.
Fixes: dc2a3e857981 ("x86/resctrl: Add interface to read mbm_total_bytes_config")
Signed-off-by: Babu Moger <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
---
arch/x86/kernel/cpu/resctrl/internal.h | 4 +---
arch/x86/kernel/cpu/resctrl/monitor.c | 11 +++++++++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 4 ++--
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index d2979748fae4..524d8bec1439 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -50,9 +50,6 @@
/* Dirty Victims to All Types of Memory */
#define DIRTY_VICTIMS_TO_ALL_MEM BIT(6)
-/* Max event bits supported */
-#define MAX_EVT_CONFIG_BITS GENMASK(6, 0)
-
struct rdt_fs_context {
struct kernfs_fs_context kfc;
bool enable_cdpl2;
@@ -117,6 +114,7 @@ extern bool rdt_alloc_capable;
extern bool rdt_mon_capable;
extern unsigned int rdt_mon_features;
extern struct list_head resctrl_schema_all;
+extern unsigned int resctrl_max_evt_bitmask;
enum rdt_group_type {
RDTCTRL_GROUP = 0,
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index f136ac046851..c611b16ba259 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -127,6 +127,11 @@ static const struct mbm_correction_factor_table {
static u32 mbm_cf_rmidthreshold __read_mostly = UINT_MAX;
static u64 mbm_cf __read_mostly;
+/*
+ * Identifies the list of QoS Bandwidth Sources to track
+ */
+unsigned int resctrl_max_evt_bitmask;
+
static inline u64 get_corrected_mbm_count(u32 rmid, unsigned long val)
{
/* Correct MBM value. */
@@ -813,6 +818,12 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
return ret;
if (rdt_cpu_has(X86_FEATURE_BMEC)) {
+ u32 eax, ebx, ecx, edx;
+
+ /* Detect list of bandwidth sources that can be tracked */
+ cpuid_count(0x80000020, 3, &eax, &ebx, &ecx, &edx);
+ resctrl_max_evt_bitmask = ecx;
+
if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
mbm_total_event.configurable = true;
mbm_config_rftype_init("mbm_total_bytes_config");
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 69a1de92384a..6c22718dbaa2 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1547,7 +1547,7 @@ static void mon_event_config_read(void *info)
rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
/* Report only the valid event configuration bits */
- mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
+ mon_info->mon_config = msrval & resctrl_max_evt_bitmask;
}
static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
@@ -1621,7 +1621,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
int ret = 0;
/* mon_config cannot be more than the supported set of events */
- if (val > MAX_EVT_CONFIG_BITS) {
+ if (val > resctrl_max_evt_bitmask) {
rdt_last_cmd_puts("Invalid event configuration\n");
return -EINVAL;
}
--
2.34.1
Hi Babu,
On 11/30/2023 4:57 PM, Babu Moger wrote:
> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
> supported, the bandwidth events can be configured. Currently, the maximum
> supported event bitmask is hard-coded. This information can be detected by
> the CPUID_Fn80000020_ECX_x03.
Be careful here ... the original meaning is the maximum length of the bitmask
and the new meaning is the maximum valid bitmask. Looking at the AMD spec
it looks to me that the original meaning is still valid, the number of
bits available in register to configure the bandwidth types is still the same.
There is additional information about which of those bits are actually supported
by the hardware.
>
> CPUID_Fn80000020_ECX_x03 [Platform QoS Monitoring Bandwidth Event
> Configuration] Read-only. Reset: 0000_007Fh.
> Bits Description
> 31:7 Reserved
> 6:0 Identifies the bandwidth sources that can be tracked.
So above means that only bits 0 to 6 can be used for config of event type? This
is done in current implementation and I do not think this should change.
Learning and using the supported events from hardware is new.
>
> Remove the hardcoded value and detect using CPUID command.
>
> The CPUID details are documentation in the PPR listed below [1].
> [1] Processor Programming Reference (PPR) Vol 1.1 for AMD Family 19h Model
> 11h B1 - 55901 Rev 0.25.
>
> Fixes: dc2a3e857981 ("x86/resctrl: Add interface to read mbm_total_bytes_config")
Please highlight the impact here to understand if this is
stable material. This also does not seem part of this series.
> Signed-off-by: Babu Moger <[email protected]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> ---
> arch/x86/kernel/cpu/resctrl/internal.h | 4 +---
> arch/x86/kernel/cpu/resctrl/monitor.c | 11 +++++++++++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 4 ++--
> 3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index d2979748fae4..524d8bec1439 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -50,9 +50,6 @@
> /* Dirty Victims to All Types of Memory */
> #define DIRTY_VICTIMS_TO_ALL_MEM BIT(6)
>
> -/* Max event bits supported */
> -#define MAX_EVT_CONFIG_BITS GENMASK(6, 0)
> -
> struct rdt_fs_context {
> struct kernfs_fs_context kfc;
> bool enable_cdpl2;
> @@ -117,6 +114,7 @@ extern bool rdt_alloc_capable;
> extern bool rdt_mon_capable;
> extern unsigned int rdt_mon_features;
> extern struct list_head resctrl_schema_all;
> +extern unsigned int resctrl_max_evt_bitmask;
>
Why is this global and not resource specific like other monitoring
properties?
> enum rdt_group_type {
> RDTCTRL_GROUP = 0,
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index f136ac046851..c611b16ba259 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -127,6 +127,11 @@ static const struct mbm_correction_factor_table {
> static u32 mbm_cf_rmidthreshold __read_mostly = UINT_MAX;
> static u64 mbm_cf __read_mostly;
>
> +/*
> + * Identifies the list of QoS Bandwidth Sources to track
> + */
> +unsigned int resctrl_max_evt_bitmask;
> +
> static inline u64 get_corrected_mbm_count(u32 rmid, unsigned long val)
> {
> /* Correct MBM value. */
> @@ -813,6 +818,12 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
> return ret;
>
> if (rdt_cpu_has(X86_FEATURE_BMEC)) {
> + u32 eax, ebx, ecx, edx;
> +
> + /* Detect list of bandwidth sources that can be tracked */
> + cpuid_count(0x80000020, 3, &eax, &ebx, &ecx, &edx);
> + resctrl_max_evt_bitmask = ecx;
> +
> if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
> mbm_total_event.configurable = true;
> mbm_config_rftype_init("mbm_total_bytes_config");
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 69a1de92384a..6c22718dbaa2 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1547,7 +1547,7 @@ static void mon_event_config_read(void *info)
> rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>
> /* Report only the valid event configuration bits */
> - mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
> + mon_info->mon_config = msrval & resctrl_max_evt_bitmask;
Original code still looks correct to me. Just like before the first seven
bits of MSR_IA32_EVT_CFG_BASE contains the event configuration.
Comparing with supported bits would be an additional check, but what does
that imply? Would it be possible for hardware to have a bit set that is
not supported? Would that mean it is actually supported or a hardware bug?
> }
>
> static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
> @@ -1621,7 +1621,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
> int ret = 0;
>
> /* mon_config cannot be more than the supported set of events */
> - if (val > MAX_EVT_CONFIG_BITS) {
> + if (val > resctrl_max_evt_bitmask) {
> rdt_last_cmd_puts("Invalid event configuration\n");
> return -EINVAL;
> }
This does not look right. resctrl_max_evt_bitmask contains the supported
types. A user may set a value that is less than resctrl_max_evt_bitmask but
yet have an unsupported bit set, no?
Reinette
Hi Reinette,
On 12/5/23 17:21, Reinette Chatre wrote:
> Hi Babu,
>
> On 11/30/2023 4:57 PM, Babu Moger wrote:
>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>> supported, the bandwidth events can be configured. Currently, the maximum
>> supported event bitmask is hard-coded. This information can be detected by
>> the CPUID_Fn80000020_ECX_x03.
>
> Be careful here ... the original meaning is the maximum length of the bitmask
> and the new meaning is the maximum valid bitmask. Looking at the AMD spec
> it looks to me that the original meaning is still valid, the number of
> bits available in register to configure the bandwidth types is still the same.
>
> There is additional information about which of those bits are actually supported
> by the hardware.
There is no change in the definition. Definition is still the same. I just
wanted to remove hard-coding.
Will make that clear.
>
>>
>> CPUID_Fn80000020_ECX_x03 [Platform QoS Monitoring Bandwidth Event
>> Configuration] Read-only. Reset: 0000_007Fh.
>> Bits Description
>> 31:7 Reserved
>> 6:0 Identifies the bandwidth sources that can be tracked.
>
> So above means that only bits 0 to 6 can be used for config of event type? This
> is done in current implementation and I do not think this should change.
> Learning and using the supported events from hardware is new.
That is correct. Right now bits 0 to 6 are only used. New hardware can add
more bits here to count new type of event.
>
>>
>> Remove the hardcoded value and detect using CPUID command.
>>
>> The CPUID details are documentation in the PPR listed below [1].
>> [1] Processor Programming Reference (PPR) Vol 1.1 for AMD Family 19h Model
>> 11h B1 - 55901 Rev 0.25.
>>
>> Fixes: dc2a3e857981 ("x86/resctrl: Add interface to read mbm_total_bytes_config")
>
> Please highlight the impact here to understand if this is
This is just to make sure not to change code for new hardware adds new
event. There is not much impact currently.
> stable material. This also does not seem part of this series.
Agree. This is not specific to this series.
>
>> Signed-off-by: Babu Moger <[email protected]>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 4 +---
>> arch/x86/kernel/cpu/resctrl/monitor.c | 11 +++++++++++
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 4 ++--
>> 3 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index d2979748fae4..524d8bec1439 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -50,9 +50,6 @@
>> /* Dirty Victims to All Types of Memory */
>> #define DIRTY_VICTIMS_TO_ALL_MEM BIT(6)
>>
>> -/* Max event bits supported */
>> -#define MAX_EVT_CONFIG_BITS GENMASK(6, 0)
>> -
>> struct rdt_fs_context {
>> struct kernfs_fs_context kfc;
>> bool enable_cdpl2;
>> @@ -117,6 +114,7 @@ extern bool rdt_alloc_capable;
>> extern bool rdt_mon_capable;
>> extern unsigned int rdt_mon_features;
>> extern struct list_head resctrl_schema_all;
>> +extern unsigned int resctrl_max_evt_bitmask;
>>
>
> Why is this global and not resource specific like other monitoring
> properties?
It does not have to be global. I can add it part rdt_hw_resource.
>
>> enum rdt_group_type {
>> RDTCTRL_GROUP = 0,
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index f136ac046851..c611b16ba259 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -127,6 +127,11 @@ static const struct mbm_correction_factor_table {
>> static u32 mbm_cf_rmidthreshold __read_mostly = UINT_MAX;
>> static u64 mbm_cf __read_mostly;
>>
>> +/*
>> + * Identifies the list of QoS Bandwidth Sources to track
>> + */
>> +unsigned int resctrl_max_evt_bitmask;
>> +
>> static inline u64 get_corrected_mbm_count(u32 rmid, unsigned long val)
>> {
>> /* Correct MBM value. */
>> @@ -813,6 +818,12 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
>> return ret;
>>
>> if (rdt_cpu_has(X86_FEATURE_BMEC)) {
>> + u32 eax, ebx, ecx, edx;
>> +
>> + /* Detect list of bandwidth sources that can be tracked */
>> + cpuid_count(0x80000020, 3, &eax, &ebx, &ecx, &edx);
>> + resctrl_max_evt_bitmask = ecx;
>> +
>> if (rdt_cpu_has(X86_FEATURE_CQM_MBM_TOTAL)) {
>> mbm_total_event.configurable = true;
>> mbm_config_rftype_init("mbm_total_bytes_config");
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 69a1de92384a..6c22718dbaa2 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1547,7 +1547,7 @@ static void mon_event_config_read(void *info)
>> rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>>
>> /* Report only the valid event configuration bits */
>> - mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
>> + mon_info->mon_config = msrval & resctrl_max_evt_bitmask;
>
> Original code still looks correct to me. Just like before the first seven
> bits of MSR_IA32_EVT_CFG_BASE contains the event configuration.
Original code still works.
>
> Comparing with supported bits would be an additional check, but what does
> that imply? Would it be possible for hardware to have a bit set that is
> not supported? Would that mean it is actually supported or a hardware bug?
No. Hardware supports all the bits reported here. Like i said before
wanted to remove the hard-coded value.
>
>> }
>>
>> static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
>> @@ -1621,7 +1621,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
>> int ret = 0;
>>
>> /* mon_config cannot be more than the supported set of events */
>> - if (val > MAX_EVT_CONFIG_BITS) {
>> + if (val > resctrl_max_evt_bitmask) {
>> rdt_last_cmd_puts("Invalid event configuration\n");
>> return -EINVAL;
>> }
>
> This does not look right. resctrl_max_evt_bitmask contains the supported
> types. A user may set a value that is less than resctrl_max_evt_bitmask but
> yet have an unsupported bit set, no?
I think I have to make this clear in the patch. There is no difference in
the definition. Hardware supports all the events reported by the cpuid.
--
Thanks
Babu Moger
Hi Babu,
On 12/6/2023 9:17 AM, Moger, Babu wrote:
> On 12/5/23 17:21, Reinette Chatre wrote:
>> On 11/30/2023 4:57 PM, Babu Moger wrote:
...
>> Comparing with supported bits would be an additional check, but what does
>> that imply? Would it be possible for hardware to have a bit set that is
>> not supported? Would that mean it is actually supported or a hardware bug?
>
> No. Hardware supports all the bits reported here. Like i said before
> wanted to remove the hard-coded value.
The size of the field in the register is different information from what
the value of that field may be.
>
>>
>>> }
>>>
>>> static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
>>> @@ -1621,7 +1621,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
>>> int ret = 0;
>>>
>>> /* mon_config cannot be more than the supported set of events */
>>> - if (val > MAX_EVT_CONFIG_BITS) {
>>> + if (val > resctrl_max_evt_bitmask) {
>>> rdt_last_cmd_puts("Invalid event configuration\n");
>>> return -EINVAL;
>>> }
>>
>> This does not look right. resctrl_max_evt_bitmask contains the supported
>> types. A user may set a value that is less than resctrl_max_evt_bitmask but
>> yet have an unsupported bit set, no?
>
> I think I have to make this clear in the patch. There is no difference in
> the definition. Hardware supports all the events reported by the cpuid.
I'll try to elaborate using an example. Let's say AMD decides to make
hardware with hypothetical support mask of:
resctrl_max_evt_bitmask = 0x4F (no support for Slow Mem).
What if user attempts to set config that enables monitoring of Slow Mem:
val = 0x30
In the above example, val is not larger than resctrl_max_evt_bitmask
but it is an invalid config, no?
Reinette
Hi Reinette,
On 12/6/23 12:32, Reinette Chatre wrote:
> Hi Babu,
>
> On 12/6/2023 9:17 AM, Moger, Babu wrote:
>> On 12/5/23 17:21, Reinette Chatre wrote:
>>> On 11/30/2023 4:57 PM, Babu Moger wrote:
>
> ...
>
>>> Comparing with supported bits would be an additional check, but what does
>>> that imply? Would it be possible for hardware to have a bit set that is
>>> not supported? Would that mean it is actually supported or a hardware bug?
>>
>> No. Hardware supports all the bits reported here. Like i said before
>> wanted to remove the hard-coded value.
>
> The size of the field in the register is different information from what
> the value of that field may be.
Yes. it could be.
>
>
>>
>>>
>>>> }
>>>>
>>>> static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
>>>> @@ -1621,7 +1621,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
>>>> int ret = 0;
>>>>
>>>> /* mon_config cannot be more than the supported set of events */
>>>> - if (val > MAX_EVT_CONFIG_BITS) {
>>>> + if (val > resctrl_max_evt_bitmask) {
>>>> rdt_last_cmd_puts("Invalid event configuration\n");
>>>> return -EINVAL;
>>>> }
>>>
>>> This does not look right. resctrl_max_evt_bitmask contains the supported
>>> types. A user may set a value that is less than resctrl_max_evt_bitmask but
>>> yet have an unsupported bit set, no?
>>
>> I think I have to make this clear in the patch. There is no difference in
>> the definition. Hardware supports all the events reported by the cpuid.
>
> I'll try to elaborate using an example. Let's say AMD decides to make
> hardware with hypothetical support mask of:
> resctrl_max_evt_bitmask = 0x4F (no support for Slow Mem).
>
> What if user attempts to set config that enables monitoring of Slow Mem:
> val = 0x30
>
> In the above example, val is not larger than resctrl_max_evt_bitmask
> but it is an invalid config, no?
Yes. It is invalid config in this case.
How about changing the check to something like this?
if ((val & resctrl_max_evt_bitmask) != val) {
rdt_last_cmd_puts("Invalid event configuration\n");
return -EINVAL;
}
--
Thanks
Babu Moger
Hi Babu,
On 12/6/2023 11:17 AM, Moger, Babu wrote:
> On 12/6/23 12:32, Reinette Chatre wrote:
>> On 12/6/2023 9:17 AM, Moger, Babu wrote:
>>> On 12/5/23 17:21, Reinette Chatre wrote:
>>>> On 11/30/2023 4:57 PM, Babu Moger wrote:
...
>>>>> static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
>>>>> @@ -1621,7 +1621,7 @@ static int mbm_config_write_domain(struct rdt_resource *r,
>>>>> int ret = 0;
>>>>>
>>>>> /* mon_config cannot be more than the supported set of events */
>>>>> - if (val > MAX_EVT_CONFIG_BITS) {
>>>>> + if (val > resctrl_max_evt_bitmask) {
>>>>> rdt_last_cmd_puts("Invalid event configuration\n");
>>>>> return -EINVAL;
>>>>> }
>>>>
>>>> This does not look right. resctrl_max_evt_bitmask contains the supported
>>>> types. A user may set a value that is less than resctrl_max_evt_bitmask but
>>>> yet have an unsupported bit set, no?
>>>
>>> I think I have to make this clear in the patch. There is no difference in
>>> the definition. Hardware supports all the events reported by the cpuid.
>>
>> I'll try to elaborate using an example. Let's say AMD decides to make
>> hardware with hypothetical support mask of:
>> resctrl_max_evt_bitmask = 0x4F (no support for Slow Mem).
>>
>> What if user attempts to set config that enables monitoring of Slow Mem:
>> val = 0x30
>>
>> In the above example, val is not larger than resctrl_max_evt_bitmask
>> but it is an invalid config, no?
>
> Yes. It is invalid config in this case.
>
> How about changing the check to something like this?
>
> if ((val & resctrl_max_evt_bitmask) != val) {
> rdt_last_cmd_puts("Invalid event configuration\n");
> return -EINVAL;
> }
This would address the scenario. I also think that it will be helpful to
print the valid bitmask as part of the error message. The original implementation
specified that all bits are valid and in doing so no interface accompanied the
feature to share with users what the valid bits are. The only way user space can learn
this is is to read the *_config files after the first resctrl mount after a system boot
to see with which config values the system was initialized with (assuming system was
initialized with all supported bits enabled).
Reinette
Hi Reinette,
> -----Original Message-----
> From: Reinette Chatre <[email protected]>
> Sent: Thursday, December 7, 2023 1:02 PM
> To: Moger, Babu <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Phillips, Kim <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; Dadhania, Nikunj
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> Giani, Dhaval <[email protected]>
> Subject: Re: [PATCH 02/15] x86/resctrl: Remove hard-coded memory
> bandwidth event configuration
>
> Hi Babu,
>
> On 12/6/2023 11:17 AM, Moger, Babu wrote:
> > On 12/6/23 12:32, Reinette Chatre wrote:
> >> On 12/6/2023 9:17 AM, Moger, Babu wrote:
> >>> On 12/5/23 17:21, Reinette Chatre wrote:
> >>>> On 11/30/2023 4:57 PM, Babu Moger wrote:
>
> ...
>
> >>>>> static void mondata_config_read(struct rdt_domain *d, struct
> >>>>> mon_config_info *mon_info) @@ -1621,7 +1621,7 @@ static int
> mbm_config_write_domain(struct rdt_resource *r,
> >>>>> int ret = 0;
> >>>>>
> >>>>> /* mon_config cannot be more than the supported set of events */
> >>>>> - if (val > MAX_EVT_CONFIG_BITS) {
> >>>>> + if (val > resctrl_max_evt_bitmask) {
> >>>>> rdt_last_cmd_puts("Invalid event configuration\n");
> >>>>> return -EINVAL;
> >>>>> }
> >>>>
> >>>> This does not look right. resctrl_max_evt_bitmask contains the
> >>>> supported types. A user may set a value that is less than
> >>>> resctrl_max_evt_bitmask but yet have an unsupported bit set, no?
> >>>
> >>> I think I have to make this clear in the patch. There is no
> >>> difference in the definition. Hardware supports all the events reported by
> the cpuid.
> >>
> >> I'll try to elaborate using an example. Let's say AMD decides to make
> >> hardware with hypothetical support mask of:
> >> resctrl_max_evt_bitmask = 0x4F (no support for Slow Mem).
> >>
> >> What if user attempts to set config that enables monitoring of Slow Mem:
> >> val = 0x30
> >>
> >> In the above example, val is not larger than resctrl_max_evt_bitmask
> >> but it is an invalid config, no?
> >
> > Yes. It is invalid config in this case.
> >
> > How about changing the check to something like this?
> >
> > if ((val & resctrl_max_evt_bitmask) != val) {
> > rdt_last_cmd_puts("Invalid event configuration\n");
> > return -EINVAL;
> > }
>
> This would address the scenario. I also think that it will be helpful to print the
> valid bitmask as part of the error message. The original implementation
> specified that all bits are valid and in doing so no interface accompanied the
> feature to share with users what the valid bits are. The only way user space
> can learn this is is to read the *_config files after the first resctrl mount after a
> system boot to see with which config values the system was initialized with
> (assuming system was initialized with all supported bits enabled).
Sure. Will add the error message including the valid bitmask.
Thanks
Babu