diff mbox series

[v4,2/2] x86/resctrl: Read supported bandwidth sources using CPUID command

Message ID ba1305bf2a29ce2667d01cf997718126a7ea607b.1705009003.git.babu.moger@amd.com
State New
Headers
Series None |

Commit Message

Moger, Babu Jan. 11, 2024, 9:36 p.m. UTC 
  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.
Read the supported bandwidth sources using the CPUID command.

While at it, move the mask checking to mon_config_write() before
iterating over all the domains. Also, print the valid bitmask when
the user tries to configure invalid event configuration value.

The CPUID details are documented 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 <babu.moger@amd.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
---
v4: Minor text changes and re-order of commit tags.
    Moved the mask check to mon_config_write() before iterating over all the
    domains.

v3: Changed the event_mask name to mbm_cfg_mask. Added comments about the field.
    Reverted the masking of event configuration to original code.
    Few minor comment changes.

v2: Earlier sent as a part of ABMC feature.
    https://lore.kernel.org/lkml/20231201005720.235639-1-babu.moger@amd.com/
    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 |  3 +++
 arch/x86/kernel/cpu/resctrl/monitor.c  |  6 ++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 14 ++++++++------
 3 files changed, 17 insertions(+), 6 deletions(-)

Comments

Reinette Chatre Jan. 12, 2024, 7:02 p.m. UTC | #1 
Hi Babu,

On 1/11/2024 1:36 PM, Babu Moger wrote:

> @@ -1686,6 +1681,13 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
>  		return -EINVAL;
>  	}
>  
> +	/* mon_config cannot be more than the supported set of events */

copy&paste error? There is no mon_config in this function. 

(copy&paste difficulties reminds me of [1])

> +	if ((val & hw_res->mbm_cfg_mask) != val) {
> +		rdt_last_cmd_printf("Invalid event configuration: The maximum valid "
> +				    "bitmask is 0x%02x\n", hw_res->mbm_cfg_mask);

checkpatch.pl should have warned about this split of text across two lines.
Logging functions and single strings are allowed to exceed the max line length.
If you just merge the two lines then checkpatch.pl may still warn for resctrl strings
but that is because it does not recognize rdt_last_cmd_printf() as a logging function.

You can also just shorten the string so this patch passes the checkpatch.pl check.
For example,
"Invalid event configuration: maximum valid mask is 0x%02x\n"
or
"Invalid event configuration: maximum is 0x%02x\n"
or ?

Reinette

[1] https://lore.kernel.org/lkml/cc273d98-d73c-49bd-8799-b119966e226c@amd.com/
Moger, Babu Jan. 12, 2024, 8:38 p.m. UTC | #2 
Hi  Reinette,

On 1/12/2024 1:02 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 1/11/2024 1:36 PM, Babu Moger wrote:
>
>> @@ -1686,6 +1681,13 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* mon_config cannot be more than the supported set of events */
> copy&paste error? There is no mon_config in this function.
Yea. it should be mbm_cfg_mask.  Will fix it.
>
> (copy&paste difficulties reminds me of [1])
>
>> +	if ((val & hw_res->mbm_cfg_mask) != val) {
>> +		rdt_last_cmd_printf("Invalid event configuration: The maximum valid "
>> +				    "bitmask is 0x%02x\n", hw_res->mbm_cfg_mask);
> checkpatch.pl should have warned about this split of text across two lines.
> Logging functions and single strings are allowed to exceed the max line length.
> If you just merge the two lines then checkpatch.pl may still warn for resctrl strings
> but that is because it does not recognize rdt_last_cmd_printf() as a logging function.
>
> You can also just shorten the string so this patch passes the checkpatch.pl check.
> For example,
> "Invalid event configuration: maximum valid mask is 0x%02x\n"
> or
> "Invalid event configuration: maximum is 0x%02x\n"
> or ?

Yes. Checkpatch reported error when I split the text.

How about this?. Checkpatch is happy.

rdt_last_cmd_printf("Invalid event configuration: max valid mask is 
0x%02x\n",
+                                   hw_res->mbm_cfg_mask);

>
> Reinette
>
> [1] https://lore.kernel.org/lkml/cc273d98-d73c-49bd-8799-b119966e226c@amd.com/

We spent quite a bit of time on this earlier. Yea. It did not go the way 
I wanted it. Now, I needed to get back to higher priority tasks.  Will 
pick it up once, I am done with current priorities.

Thanks

Babu
Reinette Chatre Jan. 12, 2024, 9:24 p.m. UTC | #3 
Hi Babu,

On 1/12/2024 12:38 PM, Moger, Babu wrote:
> Hi  Reinette,
> 
> On 1/12/2024 1:02 PM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 1/11/2024 1:36 PM, Babu Moger wrote:
>>
>>> @@ -1686,6 +1681,13 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
>>>           return -EINVAL;
>>>       }
>>>   +    /* mon_config cannot be more than the supported set of events */
>> copy&paste error? There is no mon_config in this function.
> Yea. it should be mbm_cfg_mask.  Will fix it.

I do not think it is correct to replace mon_config with mbm_cfg_mask. Is this comment
not referring to the user provided value (that is checked against mbm_cfg_mask)? So
perhaps something like:

	 /* Check value from user against supported events. */
	or
	 /* Value from user cannot be more than the supported set of events. */

Please feel free to improve.

>>
>> (copy&paste difficulties reminds me of [1])
>>
>>> +    if ((val & hw_res->mbm_cfg_mask) != val) {
>>> +        rdt_last_cmd_printf("Invalid event configuration: The maximum valid "
>>> +                    "bitmask is 0x%02x\n", hw_res->mbm_cfg_mask);
>> checkpatch.pl should have warned about this split of text across two lines.
>> Logging functions and single strings are allowed to exceed the max line length.
>> If you just merge the two lines then checkpatch.pl may still warn for resctrl strings
>> but that is because it does not recognize rdt_last_cmd_printf() as a logging function.
>>
>> You can also just shorten the string so this patch passes the checkpatch.pl check.
>> For example,
>> "Invalid event configuration: maximum valid mask is 0x%02x\n"
>> or
>> "Invalid event configuration: maximum is 0x%02x\n"
>> or ?
> 
> Yes. Checkpatch reported error when I split the text.
> 
> How about this?. Checkpatch is happy.
> 
> rdt_last_cmd_printf("Invalid event configuration: max valid mask is 0x%02x\n",
> +                                   hw_res->mbm_cfg_mask);
> 

Looks good.

Reinette
Moger, Babu Jan. 12, 2024, 9:54 p.m. UTC | #4 
Hi Reinette,

On 1/12/2024 3:24 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 1/12/2024 12:38 PM, Moger, Babu wrote:
>> Hi  Reinette,
>>
>> On 1/12/2024 1:02 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 1/11/2024 1:36 PM, Babu Moger wrote:
>>>
>>>> @@ -1686,6 +1681,13 @@ static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
>>>>            return -EINVAL;
>>>>        }
>>>>    +    /* mon_config cannot be more than the supported set of events */
>>> copy&paste error? There is no mon_config in this function.
>> Yea. it should be mbm_cfg_mask.  Will fix it.
> I do not think it is correct to replace mon_config with mbm_cfg_mask. Is this comment
> not referring to the user provided value (that is checked against mbm_cfg_mask)? So
> perhaps something like:
>
> 	 /* Check value from user against supported events. */
> 	or
> 	 /* Value from user cannot be more than the supported set of events. */

This looks good. Thanks

Babu
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index d2979748fae4..e3dc35a00a19 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -394,6 +394,8 @@  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.
+ * @mbm_cfg_mask:	Bandwidth sources that can be tracked when Bandwidth
+ *			Monitoring Event Configuration (BMEC) is supported.
  * @cdp_enabled:	CDP state of this resource
  *
  * Members of this structure are either private to the architecture
@@ -408,6 +410,7 @@  struct rdt_hw_resource {
 				 struct rdt_resource *r);
 	unsigned int		mon_scale;
 	unsigned int		mbm_width;
+	unsigned int		mbm_cfg_mask;
 	bool			cdp_enabled;
 };
 
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index f136ac046851..acca577e2b06 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->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..8e9c96d0ee84 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1620,12 +1620,6 @@  static int mbm_config_write_domain(struct rdt_resource *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");
-		return -EINVAL;
-	}
-
 	/*
 	 * Read the current config value first. If both are the same then
 	 * no need to write it again.
@@ -1663,6 +1657,7 @@  static int mbm_config_write_domain(struct rdt_resource *r,
 
 static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
 {
+	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
 	char *dom_str = NULL, *id_str;
 	unsigned long dom_id, val;
 	struct rdt_domain *d;
@@ -1686,6 +1681,13 @@  static int mon_config_write(struct rdt_resource *r, char *tok, u32 evtid)
 		return -EINVAL;
 	}
 
+	/* mon_config cannot be more than the supported set of events */
+	if ((val & hw_res->mbm_cfg_mask) != val) {
+		rdt_last_cmd_printf("Invalid event configuration: The maximum valid "
+				    "bitmask is 0x%02x\n", hw_res->mbm_cfg_mask);
+		return -EINVAL;
+	}
+
 	list_for_each_entry(d, &r->domains, list) {
 		if (d->id == dom_id) {
 			ret = mbm_config_write_domain(r, d, evtid, val);