Hi Reinette,
Sorry for late response. I was out of office for couple of weeks.
On 12/14/23 19:24, Reinette Chatre wrote:
> Hi Babu,
>
> On 12/12/2023 10:02 AM, Babu Moger wrote:
>> If the BMEC (Bandwidth Monitoring Event Configuration) feature is
>> supported, the bandwidth events can be configured. The maximum supported
>> bandwidth bitmask can be determined by following CPUID command.
>>
>> 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.
>>
>> The bandwidth sources can change with the processor generations.
>> Currently, this information is hard-coded. Remove the hard-coded value
>> and detect using CPUID command. Also print the valid bitmask when the
>> user tries to configure invalid value.
>>
>> 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")
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>>
>> ---
>> v2: Earlier Sent as a part of ABMC feature.
>> https://lore.kernel.org/lkml/20231201005720.235639-1-babu.moger@xxxxxxx/
>> But this is not related to ABMC. Sending it separate now.
>> Removed the global resctrl_max_evt_bitmask. Added event_mask as part of
>> the resource.
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 5 ++---
>> arch/x86/kernel/cpu/resctrl/monitor.c | 6 ++++++
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 18 ++++++++++--------
>> 3 files changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index d2979748fae4..3e2f505614d8 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;
>> @@ -394,6 +391,7 @@ struct rdt_parse_data {
>> * @msr_update: Function pointer to update QOS MSRs
>> * @mon_scale: cqm counter * mon_scale = occupancy in bytes
>> * @mbm_width: Monitor width, to detect and correct for overflow.
>> + * @event_mask: Max supported event bitmask.
>
> This is a very generic name and description for this feature. Note that in
> resctrl monitoring an "event" is already clear (see members of enum resctrl_event_id)
> so a generic type of "event_mask" can easily cause confusion with existing
> concept of events. How about "mbm_cfg_mask"? Please also make the description
That should be fine.
> more detailed - it could include that this is unique to BMEC.
Sure.
>
>> * @cdp_enabled: CDP state of this resource
>> *
>> * Members of this structure are either private to the architecture
>> @@ -408,6 +406,7 @@ struct rdt_hw_resource {
>> struct rdt_resource *r);
>> unsigned int mon_scale;
>> unsigned int mbm_width;
>> + unsigned int event_mask;
>> bool cdp_enabled;
>> };
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index f136ac046851..30bf919edfda 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -813,6 +813,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);
>> + hw_res->event_mask = ecx;
>> +
>
> This has the same issue as I mentioned in V1. Note that this treats
> reserved bits as valid values. I think this is a risky thing to do. For example
> when this code is run on future hardware the currently reserved bits may have
> values with different meaning than what this code uses it for.
Sure. Will use the mask MAX_EVT_CONFIG_BITS.
hw_res->mbm_cfg_mask = ecx & MAX_EVT_CONFIG_BITS;
>
>> 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..8a1e9fdab974 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1537,17 +1537,14 @@ static void mon_event_config_read(void *info)
>> {
>> struct mon_config_info *mon_info = info;
>> unsigned int index;
>> - u64 msrval;
>> + u32 h;
>>
>> 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);
>> return;
>> }
>> - rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
>> -
>> - /* Report only the valid event configuration bits */
>> - mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
>> + rdmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, h);
>
> I do not think this code needed to be changed. We do not want to treat
> reserved bits as valid values.
The logic is still the same. We don't have access to rdt_hw_resource in
this function. So, I just moved the masking to mbm_config_show while printing.
Thanks
Babu
>
>> }
>>
>> static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info)
>> @@ -1557,6 +1554,7 @@ static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mo
>>
>> static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
>> {
>> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>> struct mon_config_info mon_info = {0};
>> struct rdt_domain *dom;
>> bool sep = false;
>> @@ -1571,7 +1569,9 @@ static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid
>> mon_info.evtid = evtid;
>> mondata_config_read(dom, &mon_info);
>>
>> - seq_printf(s, "%d=0x%02x", dom->id, mon_info.mon_config);
>> + /* Report only the valid event configuration bits */
>> + seq_printf(s, "%d=0x%02x", dom->id,
>> + mon_info.mon_config & hw_res->event_mask);
>> sep = true;
>> }
>> seq_puts(s, "\n");
>> @@ -1617,12 +1617,14 @@ static void mon_event_config_write(void *info)
>> static int mbm_config_write_domain(struct rdt_resource *r,
>> struct rdt_domain *d, u32 evtid, u32 val)
>> {
>> + struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>> struct mon_config_info mon_info = {0};
>> int ret = 0;
>>
>> /* mon_config cannot be more than the supported set of events */
>> - if (val > MAX_EVT_CONFIG_BITS) {
>> - rdt_last_cmd_puts("Invalid event configuration\n");
>> + if ((val & hw_res->event_mask) != val) {
>> + rdt_last_cmd_printf("Invalid input: The maximum valid bitmask is 0x%02x\n",
>> + hw_res->event_mask);
>> return -EINVAL;
>> }
>>
>>
>>
>
> Reinette
--
Thanks
Babu Moger
