powercap: Enable RAPL for AMD Fam17h and Fam19h

PATCH In review Victor Ding 5 participants 14 messages Oct 27, 2020 – Nov 10, 2020

Subsystems (2)

x86 Power management core

lore

Share & tools

lore

Victor Ding

author

2020-10-27 07:24 UTC #0 lore raw

v3 cover powercap: Enable RAPL for AMD Fam17h and Fam19h

This patch series adds support for AMD Fam17h RAPL counters. As per AMD PPR, Fam17h and Fam19h support RAPL counters to monitor power usage. The RAPL counter operates as with Intel RAPL. Therefore, it is beneficial to re-use existing framework for Intel, especially to allow existing tools to seamlessly run on AMD.

From the user's point view, this series enables the following two sysfs entry on AMD Fam17h or Fam19h:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-rapl:0:0/energy_uj

Changes in v3:
By Victor Ding <[email protected]>
- Rebased to the latest code.
- Created a new rapl_defaults for AMD CPUs.
- Removed redundant setting to zeros.
- Stopped using the fake power limit domain 1.

Kim Phillips (1):
powercap: Add AMD Fam19h RAPL support

Victor Ding (3):
x86/msr-index: sort AMD RAPL MSRs by address powercap/intel_rapl_msr: Convert rapl_msr_priv into pointer
powercap: Add AMD Fam17h RAPL support

 arch/x86/include/asm/msr-index.h     |  3 +-
 drivers/powercap/intel_rapl_common.c |  7 ++++
 drivers/powercap/intel_rapl_msr.c    | 51 ++++++++++++++++++++--------
 3 files changed, 45 insertions(+), 16 deletions(-)

Signature · 1 line

2.29.0.rc2.309.g374f81d7ae-goog

Victor Ding

author

2020-10-27 07:24 UTC #1 lore raw

PATCH v3 1/4 x86/msr-index: sort AMD RAPL MSRs by address

MSRs in the rest of this file are sorted by their addresses; fixing the two outliers.

No functional changes.

Signed-off-by Victor Ding <[email protected]>

Acked-by Kim Phillips <[email protected]>

---

(no changes since v2)

Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code changes).

 arch/x86/include/asm/msr-index.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/include/asm/msr-index.h +1 -1

index 972a34d93505..21917e134ad4 100644
@@ -326,8 +326,8 @@
 #define MSR_PP1_ENERGY_STATUS		0x00000641
 #define MSR_PP1_POLICY			0x00000642
 
-#define MSR_AMD_PKG_ENERGY_STATUS	0xc001029b
 #define MSR_AMD_RAPL_POWER_UNIT		0xc0010299
+#define MSR_AMD_PKG_ENERGY_STATUS	0xc001029b
 
 /* Config TDP MSRs */
 #define MSR_CONFIG_TDP_NOMINAL		0x00000648

Signature · 1 line

2.29.0.rc2.309.g374f81d7ae-goog

Victor Ding

author

2020-10-27 07:24 UTC #2 lore raw

PATCH v3 2/4 powercap/intel_rapl_msr: Convert rapl_msr_priv into pointer

In reply to Victor Ding

This patch changes the static struct rapl_msr_priv to a pointer to allow using a different set of RAPL MSR interface, preparing for supporting AMD's RAPL MSR interface.

No functional changes.

Signed-off-by Victor Ding <[email protected]>

Acked-by Kim Phillips <[email protected]>

---

(no changes since v2)

Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code changes).

 drivers/powercap/intel_rapl_msr.c | 33 +++++++++++++++++--------------
 1 file changed, 18 insertions(+), 15 deletions(-)

drivers/powercap/intel_rapl_msr.c +18 ~~-15~~

index 1646808d354c..a819b3b89b2f 100644
@@ -31,7 +31,9 @@
 #define MSR_VR_CURRENT_CONFIG		0x00000601
 
 /* private data for RAPL MSR Interface */
-static struct rapl_if_priv rapl_msr_priv = {
+static struct rapl_if_priv *rapl_msr_priv;
+
+static struct rapl_if_priv rapl_msr_priv_intel = {
 	.reg_unit = MSR_RAPL_POWER_UNIT,
 	.regs[RAPL_DOMAIN_PACKAGE] = {
 		MSR_PKG_POWER_LIMIT, MSR_PKG_ENERGY_STATUS, MSR_PKG_PERF_STATUS, 0, MSR_PKG_POWER_INFO },

@@ -58,9 +60,9 @@ static int rapl_cpu_online(unsigned int cpu)
 {
 	struct rapl_package *rp;
 
-	rp = rapl_find_package_domain(cpu, &rapl_msr_priv);
+	rp = rapl_find_package_domain(cpu, rapl_msr_priv);
 	if (!rp) {
-		rp = rapl_add_package(cpu, &rapl_msr_priv);
+		rp = rapl_add_package(cpu, rapl_msr_priv);
 		if (IS_ERR(rp))
 			return PTR_ERR(rp);
 	}

@@ -73,7 +75,7 @@ static int rapl_cpu_down_prep(unsigned int cpu)
 	struct rapl_package *rp;
 	int lead_cpu;
 
-	rp = rapl_find_package_domain(cpu, &rapl_msr_priv);
+	rp = rapl_find_package_domain(cpu, rapl_msr_priv);
 	if (!rp)
 		return 0;

@@ -136,40 +138,41 @@ static int rapl_msr_probe(struct platform_device *pdev)
 	const struct x86_cpu_id *id = x86_match_cpu(pl4_support_ids);
 	int ret;
 
-	rapl_msr_priv.read_raw = rapl_msr_read_raw;
-	rapl_msr_priv.write_raw = rapl_msr_write_raw;
+	rapl_msr_priv = &rapl_msr_priv_intel;
+	rapl_msr_priv->read_raw = rapl_msr_read_raw;
+	rapl_msr_priv->write_raw = rapl_msr_write_raw;
 
 	if (id) {
-		rapl_msr_priv.limits[RAPL_DOMAIN_PACKAGE] = 3;
-		rapl_msr_priv.regs[RAPL_DOMAIN_PACKAGE][RAPL_DOMAIN_REG_PL4] =
+		rapl_msr_priv->limits[RAPL_DOMAIN_PACKAGE] = 3;
+		rapl_msr_priv->regs[RAPL_DOMAIN_PACKAGE][RAPL_DOMAIN_REG_PL4] =
 			MSR_VR_CURRENT_CONFIG;
 		pr_info("PL4 support detected.\n");
 	}
 
-	rapl_msr_priv.control_type = powercap_register_control_type(NULL, "intel-rapl", NULL);
-	if (IS_ERR(rapl_msr_priv.control_type)) {
+	rapl_msr_priv->control_type = powercap_register_control_type(NULL, "intel-rapl", NULL);
+	if (IS_ERR(rapl_msr_priv->control_type)) {
 		pr_debug("failed to register powercap control_type.\n");
-		return PTR_ERR(rapl_msr_priv.control_type);
+		return PTR_ERR(rapl_msr_priv->control_type);
 	}
 
 	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "powercap/rapl:online",
 				rapl_cpu_online, rapl_cpu_down_prep);
 	if (ret < 0)
 		goto out;
-	rapl_msr_priv.pcap_rapl_online = ret;
+	rapl_msr_priv->pcap_rapl_online = ret;
 
 	return 0;
 
 out:
 	if (ret)
-		powercap_unregister_control_type(rapl_msr_priv.control_type);
+		powercap_unregister_control_type(rapl_msr_priv->control_type);
 	return ret;
 }
 
 static int rapl_msr_remove(struct platform_device *pdev)
 {
-	cpuhp_remove_state(rapl_msr_priv.pcap_rapl_online);
-	powercap_unregister_control_type(rapl_msr_priv.control_type);
+	cpuhp_remove_state(rapl_msr_priv->pcap_rapl_online);
+	powercap_unregister_control_type(rapl_msr_priv->control_type);
 	return 0;
 }

Signature · 1 line

2.29.0.rc2.309.g374f81d7ae-goog

Victor Ding

author

2020-10-27 07:25 UTC #3 lore raw

PATCH v3 3/4 powercap: Add AMD Fam17h RAPL support

In reply to Victor Ding

This patch enables AMD Fam17h RAPL support for the power capping framework. The support is as per AMD Fam17h Model31h (Zen2) and model 00-ffh (Zen1) PPR.

Tested by comparing the results of following two sysfs entries and the values directly read from corresponding MSRs via /dev/cpu/[x]/msr:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-rapl:0:0/energy_uj

Signed-off-by Victor Ding <[email protected]>

Acked-by Kim Phillips <[email protected]>

---

Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code change).

 arch/x86/include/asm/msr-index.h     |  1 +
 drivers/powercap/intel_rapl_common.c |  6 ++++++
 drivers/powercap/intel_rapl_msr.c    | 20 +++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)

arch/x86/include/asm/msr-index.h +1 -0

index 21917e134ad4..c36a083c8ec0 100644
@@ -327,6 +327,7 @@
 #define MSR_PP1_POLICY			0x00000642
 
 #define MSR_AMD_RAPL_POWER_UNIT		0xc0010299
+#define MSR_AMD_CORE_ENERGY_STATUS		0xc001029a
 #define MSR_AMD_PKG_ENERGY_STATUS	0xc001029b
 
 /* Config TDP MSRs */

drivers/powercap/intel_rapl_common.c +6 -0

index 0b2830efc574..bedd780bed12 100644
@@ -1011,6 +1011,10 @@ static const struct rapl_defaults rapl_defaults_cht = {
 	.compute_time_window = rapl_compute_time_window_atom,
 };
 
+static const struct rapl_defaults rapl_defaults_amd = {
+	.check_unit = rapl_check_unit_core,
+};
+
 static const struct x86_cpu_id rapl_ids[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE,		&rapl_defaults_core),
 	X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X,	&rapl_defaults_core),

@@ -1061,6 +1065,8 @@ static const struct x86_cpu_id rapl_ids[] __initconst = {
 
 	X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL,	&rapl_defaults_hsw_server),
 	X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM,	&rapl_defaults_hsw_server),
+
+	X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);

drivers/powercap/intel_rapl_msr.c +19 -1

index a819b3b89b2f..78213d4b5b16 100644
@@ -49,6 +49,14 @@ static struct rapl_if_priv rapl_msr_priv_intel = {
 	.limits[RAPL_DOMAIN_PLATFORM] = 2,
 };
 
+static struct rapl_if_priv rapl_msr_priv_amd = {
+	.reg_unit = MSR_AMD_RAPL_POWER_UNIT,
+	.regs[RAPL_DOMAIN_PACKAGE] = {
+		0, MSR_AMD_PKG_ENERGY_STATUS, 0, 0, 0 },
+	.regs[RAPL_DOMAIN_PP0] = {
+		0, MSR_AMD_CORE_ENERGY_STATUS, 0, 0, 0 },
+};
+
 /* Handles CPU hotplug on multi-socket systems.
  * If a CPU goes online as the first CPU of the physical package
  * we add the RAPL package to the system. Similarly, when the last

@@ -138,7 +146,17 @@ static int rapl_msr_probe(struct platform_device *pdev)
 	const struct x86_cpu_id *id = x86_match_cpu(pl4_support_ids);
 	int ret;
 
-	rapl_msr_priv = &rapl_msr_priv_intel;
+	switch (boot_cpu_data.x86_vendor) {
+	case X86_VENDOR_INTEL:
+		rapl_msr_priv = &rapl_msr_priv_intel;
+		break;
+	case X86_VENDOR_AMD:
+		rapl_msr_priv = &rapl_msr_priv_amd;
+		break;
+	default:
+		pr_err("intel-rapl does not support CPU vendor %d\n", boot_cpu_data.x86_vendor);
+		return -ENODEV;
+	}
 	rapl_msr_priv->read_raw = rapl_msr_read_raw;
 	rapl_msr_priv->write_raw = rapl_msr_write_raw;

Signature · 1 line

2.29.0.rc2.309.g374f81d7ae-goog

Zhang Rui

2020-11-02 01:39 UTC #4 lore raw

Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support

Quoted 54 lines · Victor Ding

This patch enables AMD Fam17h RAPL support for the power capping framework. The support is as per AMD Fam17h Model31h (Zen2) and model 00-ffh (Zen1) PPR.
Tested by comparing the results of following two sysfs entries and the
values directly read from corresponding MSRs via /dev/cpu/[x]/msr:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
rapl:0:0/energy_uj
Signed-off-by Victor Ding <[email protected]>
Acked-by Kim Phillips <[email protected]>
---
Changes in v3:
By Victor Ding <[email protected]>
- Rebased to the latest code.
- Created a new rapl_defaults for AMD CPUs.
- Removed redundant setting to zeros.
- Stopped using the fake power limit domain 1.
Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code change).
 arch/x86/include/asm/msr-index.h     |  1 +
 drivers/powercap/intel_rapl_common.c |  6 ++++++
 drivers/powercap/intel_rapl_msr.c    | 20 +++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
arch/x86/include/asm/msr-index.h +1 -0
index 21917e134ad4..c36a083c8ec0 100644
@@ -327,6 +327,7 @@
 #define MSR_PP1_POLICY			0x00000642
 
 #define MSR_AMD_RAPL_POWER_UNIT		0xc0010299
+#define MSR_AMD_CORE_ENERGY_STATUS		0xc001029a
 #define MSR_AMD_PKG_ENERGY_STATUS	0xc001029b
 
 /* Config TDP MSRs */
drivers/powercap/intel_rapl_common.c +0 -0
index 0b2830efc574..bedd780bed12 100644

drivers/powercap/intel_rapl_common.c static const struct rapl_defaults · line 1018

Quoted 8 lines

drivers/powercap/intel_rapl_common.c +4 -0

@@ -1011,6 +1011,10 @@ static const struct rapl_defaults
 rapl_defaults_cht = {
 	.compute_time_window = rapl_compute_time_window_atom,
 };
 
+static const struct rapl_defaults rapl_defaults_amd = {
+	.check_unit = rapl_check_unit_core,
+};
+

why do we need power_unit and time_unit if we only want to expose the energy counter?

Plus, in rapl_init_domains(), PL1 is enabled for every RAPL Domain blindly, I'm not sure how this is handled on the AMD CPUs. Is PL1 invalidated by rapl_detect_powerlimit()? or is it still registered as a valid constraint into powercap sysfs I/F?

Currently, the code makes the assumption that there is only on power limit if priv->limits[domain_id] not set, we probably need to change this if we want to support RAPL domains with no power limit.

thanks,
rui

Quoted 57 lines

drivers/powercap/intel_rapl_common.c +0 -0

 static const struct x86_cpu_id rapl_ids[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE,		&rapl_default

s_core),

drivers/powercap/intel_rapl_common.c +2 -0

 	X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X,	&rapl_defaults_core),

@@ -1061,6 +1065,8 @@ static const struct x86_cpu_id rapl_ids[]
 __initconst = {
 
 	X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL,	&rapl_defaults_hsw_se
 rver),
 	X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM,	&rapl_defaults_hsw_se
 rver),
+
+	X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd),

drivers/powercap/intel_rapl_common.c +0 -0

 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);

drivers/powercap/intel_rapl_msr.c +19 -1

index a819b3b89b2f..78213d4b5b16 100644
@@ -49,6 +49,14 @@ static struct rapl_if_priv rapl_msr_priv_intel = {
 	.limits[RAPL_DOMAIN_PLATFORM] = 2,
 };
 
+static struct rapl_if_priv rapl_msr_priv_amd = {
+	.reg_unit = MSR_AMD_RAPL_POWER_UNIT,
+	.regs[RAPL_DOMAIN_PACKAGE] = {
+		0, MSR_AMD_PKG_ENERGY_STATUS, 0, 0, 0 },
+	.regs[RAPL_DOMAIN_PP0] = {
+		0, MSR_AMD_CORE_ENERGY_STATUS, 0, 0, 0 },
+};
+
 /* Handles CPU hotplug on multi-socket systems.
  * If a CPU goes online as the first CPU of the physical package
  * we add the RAPL package to the system. Similarly, when the last

@@ -138,7 +146,17 @@ static int rapl_msr_probe(struct platform_device
 *pdev)
 	const struct x86_cpu_id *id = x86_match_cpu(pl4_support_ids);
 	int ret;
 
-	rapl_msr_priv = &rapl_msr_priv_intel;
+	switch (boot_cpu_data.x86_vendor) {
+	case X86_VENDOR_INTEL:
+		rapl_msr_priv = &rapl_msr_priv_intel;
+		break;
+	case X86_VENDOR_AMD:
+		rapl_msr_priv = &rapl_msr_priv_amd;
+		break;
+	default:
+		pr_err("intel-rapl does not support CPU vendor %d\n",
 boot_cpu_data.x86_vendor);
+		return -ENODEV;
+	}
 	rapl_msr_priv->read_raw = rapl_msr_read_raw;

rapl_msr_priv->write_raw = rapl_msr_write_raw;

Victor Ding

author

2020-11-03 06:11 UTC #5 lore raw

Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support

Quoted 60 lines · Zhang Rui

Quoted 58 lines · Victor Ding
This patch enables AMD Fam17h RAPL support for the power capping framework. The support is as per AMD Fam17h Model31h (Zen2) and model 00-ffh (Zen1) PPR.
Tested by comparing the results of following two sysfs entries and the
values directly read from corresponding MSRs via /dev/cpu/[x]/msr:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
rapl:0:0/energy_uj
Signed-off-by Victor Ding <[email protected]>
Acked-by Kim Phillips <[email protected]>
---
Changes in v3:
By Victor Ding <[email protected]>
- Rebased to the latest code.
- Created a new rapl_defaults for AMD CPUs.
- Removed redundant setting to zeros.
- Stopped using the fake power limit domain 1.
Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code change).
 arch/x86/include/asm/msr-index.h     |  1 +
 drivers/powercap/intel_rapl_common.c |  6 ++++++
 drivers/powercap/intel_rapl_msr.c    | 20 +++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
arch/x86/include/asm/msr-index.h +1 -0
index 21917e134ad4..c36a083c8ec0 100644
@@ -327,6 +327,7 @@
 #define MSR_PP1_POLICY                       0x00000642
 
 #define MSR_AMD_RAPL_POWER_UNIT              0xc0010299
+#define MSR_AMD_CORE_ENERGY_STATUS           0xc001029a
 #define MSR_AMD_PKG_ENERGY_STATUS    0xc001029b
 
 /* Config TDP MSRs */
drivers/powercap/intel_rapl_common.c +0 -0
index 0b2830efc574..bedd780bed12 100644
@@ -1011,6 +1011,10 @@ static const struct rapl_defaults
 rapl_defaults_cht = {
      .compute_time_window = rapl_compute_time_window_atom,
 };
 

Quoted 7 lines

drivers/powercap/intel_rapl_common.c static const struct rapl_defaults rapl_defaults_cht = { · line 1017
Quoted 4 lines
drivers/powercap/intel_rapl_common.c +4 -0
+static const struct rapl_defaults rapl_defaults_amd = {
+     .check_unit = rapl_check_unit_core,
+};
+
why do we need power_unit and time_unit if we only want to expose the energy counter?

AMD's Power Unit MSR provides identical information as Intel's, including time units, power units, and energy status units. By reusing the check unit method, we could avoid code duplication as well as easing future enhance- ment when AMD starts to support power limits.

Quoted 5 lines

Plus, in rapl_init_domains(), PL1 is enabled for every RAPL Domain blindly, I'm not sure how this is handled on the AMD CPUs. Is PL1 invalidated by rapl_detect_powerlimit()? or is it still registered as a valid constraint into powercap sysfs I/F?

AMD's CORE_ENERGY_STAT MSR is like Intel's PP0_ENERGY_STATUS; therefore, PL1 also always exists on AMD. rapl_detect_powerlimit() correctly markes the domain as monitoring-only after finding power limit MSRs do not exist.

Quoted 4 lines

Currently, the code makes the assumption that there is only on power limit if priv->limits[domain_id] not set, we probably need to change this if we want to support RAPL domains with no power limit.

The existing code already supports RAPL domains with no power limit: PL1 is enabled when there is zero or one power limit, rapl_detect_powerlimit() will then
mark if PL1 is monitoring-only if power limit MSRs do not exist. Both AMD's RAPL domains are monitoring-only and are correctly marked and handled.

Quoted 44 lines

thanks,
rui

Quoted 41 lines

drivers/powercap/intel_rapl_common.c +0 -0

 static const struct x86_cpu_id rapl_ids[] __initconst = {
      X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE,         &rapl_default

s_core),
X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X, &rapl_defaults_core),

drivers/powercap/intel_rapl_common.c +2 -0

@@ -1061,6 +1065,8 @@ static const struct x86_cpu_id rapl_ids[]
 __initconst = {
 
      X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL,        &rapl_defaults_hsw_se
 rver),
      X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM,        &rapl_defaults_hsw_se
 rver),
+
+     X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd),

{}

};
MODULE_DEVICE_TABLE(x86cpu, rapl_ids);

drivers/powercap/intel_rapl_msr.c +8 -0

index a819b3b89b2f..78213d4b5b16 100644
@@ -49,6 +49,14 @@ static struct rapl_if_priv rapl_msr_priv_intel = {
      .limits[RAPL_DOMAIN_PLATFORM] = 2,
 };
 
+static struct rapl_if_priv rapl_msr_priv_amd = {
+     .reg_unit = MSR_AMD_RAPL_POWER_UNIT,
+     .regs[RAPL_DOMAIN_PACKAGE] = {
+             0, MSR_AMD_PKG_ENERGY_STATUS, 0, 0, 0 },
+     .regs[RAPL_DOMAIN_PP0] = {
+             0, MSR_AMD_CORE_ENERGY_STATUS, 0, 0, 0 },
+};
+
 /* Handles CPU hotplug on multi-socket systems.
  * If a CPU goes online as the first CPU of the physical package
  * we add the RAPL package to the system. Similarly, when the last

@@ -138,7 +146,17 @@ static int rapl_msr_probe(struct platform_device
 *pdev)
      const struct x86_cpu_id *id = x86_match_cpu(pl4_support_ids);
      int ret;

Quoted 17 lines

Quoted 14 lines

drivers/powercap/intel_rapl_msr.c +11 -1

-     rapl_msr_priv = &rapl_msr_priv_intel;
+     switch (boot_cpu_data.x86_vendor) {
+     case X86_VENDOR_INTEL:
+             rapl_msr_priv = &rapl_msr_priv_intel;
+             break;
+     case X86_VENDOR_AMD:
+             rapl_msr_priv = &rapl_msr_priv_amd;
+             break;
+     default:
+             pr_err("intel-rapl does not support CPU vendor %d\n",
 boot_cpu_data.x86_vendor);
+             return -ENODEV;
+     }
      rapl_msr_priv->read_raw = rapl_msr_read_raw;

Quoted 2 lines

      rapl_msr_priv->write_raw = rapl_msr_write_raw;

Best regards,
Victor Ding

Srinivas Pandruvada

2020-11-03 17:09 UTC #6 lore raw

Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support

Quoted 7 lines · Victor Ding

Quoted 5 lines · Zhang Rui
Quoted 4 lines · Victor Ding
This patch enables AMD Fam17h RAPL support for the power capping framework. The support is as per AMD Fam17h Model31h (Zen2) and model 00-ffh (Zen1) PPR.

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Tested by comparing the results of following two sysfs entries and
the
values directly read from corresponding MSRs via /dev/cpu/[x]/msr:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
rapl:0:0/energy_uj

Is this for just energy reporting? No capping of power?

Thanks,
Srinivas

Quoted 155 lines

Quoted 48 lines
Quoted 48 lines
Signed-off-by Victor Ding <[email protected]>
Acked-by Kim Phillips <[email protected]>
---
Changes in v3:
By Victor Ding <[email protected]>
- Rebased to the latest code.
- Created a new rapl_defaults for AMD CPUs.
- Removed redundant setting to zeros.
- Stopped using the fake power limit domain 1.
Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code change).
 arch/x86/include/asm/msr-index.h     |  1 +
 drivers/powercap/intel_rapl_common.c |  6 ++++++
 drivers/powercap/intel_rapl_msr.c    | 20 +++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
arch/x86/include/asm/msr-index.h +1 -0
index 21917e134ad4..c36a083c8ec0 100644
@@ -327,6 +327,7 @@
 #define MSR_PP1_POLICY                       0x00000642
 
 #define MSR_AMD_RAPL_POWER_UNIT              0xc0010299
+#define MSR_AMD_CORE_ENERGY_STATUS           0xc001029a
 #define MSR_AMD_PKG_ENERGY_STATUS    0xc001029b
 
 /* Config TDP MSRs */
drivers/powercap/intel_rapl_common.c +0 -0
index 0b2830efc574..bedd780bed12 100644
@@ -1011,6 +1011,10 @@ static const struct rapl_defaults
 rapl_defaults_cht = {
      .compute_time_window = rapl_compute_time_window_atom,
 };
 
Quoted 8 lines
drivers/powercap/intel_rapl_common.c static const struct rapl_defaults rapl_defaults_cht = { · line 1017
Quoted 4 lines
drivers/powercap/intel_rapl_common.c +4 -0
+static const struct rapl_defaults rapl_defaults_amd = {
+     .check_unit = rapl_check_unit_core,
+};
+
why do we need power_unit and time_unit if we only want to expose the
energy counter?
AMD's Power Unit MSR provides identical information as Intel's, including
time units, power units, and energy status units. By reusing the check unit
method, we could avoid code duplication as well as easing future enhance-
ment when AMD starts to support power limits.
Quoted 4 lines
Plus, in rapl_init_domains(), PL1 is enabled for every RAPL Domain blindly, I'm not sure how this is handled on the AMD CPUs. Is PL1 invalidated by rapl_detect_powerlimit()? or is it still registered as a valid constraint into powercap sysfs I/F?
AMD's CORE_ENERGY_STAT MSR is like Intel's PP0_ENERGY_STATUS; therefore, PL1 also always exists on AMD. rapl_detect_powerlimit() correctly
markes the domain as monitoring-only after finding power limit MSRs do not
exist.
Quoted 5 lines
Currently, the code makes the assumption that there is only on power
limit if priv->limits[domain_id] not set, we probably need to change
this if we want to support RAPL domains with no power limit.
The existing code already supports RAPL domains with no power limit:
PL1 is
enabled when there is zero or one power limit, rapl_detect_powerlimit() will then
mark if PL1 is monitoring-only if power limit MSRs do not exist. Both AMD's RAPL
domains are monitoring-only and are correctly marked and handled.
Quoted 60 lines
thanks,
rui
Quoted 58 lines
drivers/powercap/intel_rapl_common.c +0 -0
 static const struct x86_cpu_id rapl_ids[] __initconst = {
      X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE,         &rapl_defau
lt
s_core),
X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X, &rapl_defau
lts_core),
drivers/powercap/intel_rapl_common.c +0 -0
@@ -1061,6 +1065,8 @@ static const struct x86_cpu_id rapl_ids[]
 __initconst = {
 
      X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL,        &rapl_defau
lts_hsw_se
rver),
X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM, &rapl_defau
lts_hsw_se
rver),
drivers/powercap/intel_rapl_common.c +2 -0
+
+     X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd),
      {}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
drivers/powercap/intel_rapl_msr.c +8 -0
index a819b3b89b2f..78213d4b5b16 100644
@@ -49,6 +49,14 @@ static struct rapl_if_priv rapl_msr_priv_intel
 = {
      .limits[RAPL_DOMAIN_PLATFORM] = 2,
 };
 
+static struct rapl_if_priv rapl_msr_priv_amd = {
+     .reg_unit = MSR_AMD_RAPL_POWER_UNIT,
+     .regs[RAPL_DOMAIN_PACKAGE] = {
+             0, MSR_AMD_PKG_ENERGY_STATUS, 0, 0, 0 },
+     .regs[RAPL_DOMAIN_PP0] = {
+             0, MSR_AMD_CORE_ENERGY_STATUS, 0, 0, 0 },
+};
+
 /* Handles CPU hotplug on multi-socket systems.
  * If a CPU goes online as the first CPU of the physical package
* we add the RAPL package to the system. Similarly, when the
last
drivers/powercap/intel_rapl_msr.c +0 -0
@@ -138,7 +146,17 @@ static int rapl_msr_probe(struct
platform_device
*pdev)
const struct x86_cpu_id *id =
x86_match_cpu(pl4_support_ids);
      int ret;
drivers/powercap/intel_rapl_msr.c +8 -1
-     rapl_msr_priv = &rapl_msr_priv_intel;
+     switch (boot_cpu_data.x86_vendor) {
+     case X86_VENDOR_INTEL:
+             rapl_msr_priv = &rapl_msr_priv_intel;
+             break;
+     case X86_VENDOR_AMD:
+             rapl_msr_priv = &rapl_msr_priv_amd;
+             break;
+     default:
Quoted 8 lines
drivers/powercap/intel_rapl_msr.c static int rapl_msr_probe(struct platform_device *pdev) · line 161
Quoted 8 lines
+ pr_err("intel-rapl does not support CPU vendor
%d\n",
boot_cpu_data.x86_vendor);
drivers/powercap/intel_rapl_msr.c +2 -0
+             return -ENODEV;
+     }
      rapl_msr_priv->read_raw = rapl_msr_read_raw;
      rapl_msr_priv->write_raw = rapl_msr_write_raw;
Best regards,
Victor Ding

Victor Ding

author

2020-11-04 01:44 UTC #7 lore raw

Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support

Quoted 18 lines · Srinivas Pandruvada

Quoted 7 lines · Victor Ding
Quoted 5 lines · Zhang Rui
Quoted 4 lines · Victor Ding
This patch enables AMD Fam17h RAPL support for the power capping framework. The support is as per AMD Fam17h Model31h (Zen2) and model 00-ffh (Zen1) PPR.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Tested by comparing the results of following two sysfs entries and
the
values directly read from corresponding MSRs via /dev/cpu/[x]/msr:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
rapl:0:0/energy_uj

Quoted 1 line

Is this for just energy reporting? No capping of power?

Correct, the hardware does not support capping of power.

Quoted 161 lines

Thanks,
Srinivas
Quoted 155 lines
Quoted 48 lines
Quoted 48 lines
Signed-off-by Victor Ding <[email protected]>
Acked-by Kim Phillips <[email protected]>
---
Changes in v3:
By Victor Ding <[email protected]>
- Rebased to the latest code.
- Created a new rapl_defaults for AMD CPUs.
- Removed redundant setting to zeros.
- Stopped using the fake power limit domain 1.
Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code change).
 arch/x86/include/asm/msr-index.h     |  1 +
 drivers/powercap/intel_rapl_common.c |  6 ++++++
 drivers/powercap/intel_rapl_msr.c    | 20 +++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
arch/x86/include/asm/msr-index.h +1 -0
index 21917e134ad4..c36a083c8ec0 100644
@@ -327,6 +327,7 @@
 #define MSR_PP1_POLICY                       0x00000642
 
 #define MSR_AMD_RAPL_POWER_UNIT              0xc0010299
+#define MSR_AMD_CORE_ENERGY_STATUS           0xc001029a
 #define MSR_AMD_PKG_ENERGY_STATUS    0xc001029b
 
 /* Config TDP MSRs */
drivers/powercap/intel_rapl_common.c +0 -0
index 0b2830efc574..bedd780bed12 100644
@@ -1011,6 +1011,10 @@ static const struct rapl_defaults
 rapl_defaults_cht = {
      .compute_time_window = rapl_compute_time_window_atom,
 };
 
Quoted 8 lines
drivers/powercap/intel_rapl_common.c static const struct rapl_defaults rapl_defaults_cht = { · line 1017
Quoted 4 lines
drivers/powercap/intel_rapl_common.c +4 -0
+static const struct rapl_defaults rapl_defaults_amd = {
+     .check_unit = rapl_check_unit_core,
+};
+
why do we need power_unit and time_unit if we only want to expose the
energy counter?
AMD's Power Unit MSR provides identical information as Intel's, including
time units, power units, and energy status units. By reusing the check unit
method, we could avoid code duplication as well as easing future enhance-
ment when AMD starts to support power limits.
Quoted 4 lines
Plus, in rapl_init_domains(), PL1 is enabled for every RAPL Domain blindly, I'm not sure how this is handled on the AMD CPUs. Is PL1 invalidated by rapl_detect_powerlimit()? or is it still registered as a valid constraint into powercap sysfs I/F?
AMD's CORE_ENERGY_STAT MSR is like Intel's PP0_ENERGY_STATUS; therefore, PL1 also always exists on AMD. rapl_detect_powerlimit() correctly
markes the domain as monitoring-only after finding power limit MSRs do not
exist.
Quoted 5 lines
Currently, the code makes the assumption that there is only on power
limit if priv->limits[domain_id] not set, we probably need to change
this if we want to support RAPL domains with no power limit.
The existing code already supports RAPL domains with no power limit:
PL1 is
enabled when there is zero or one power limit, rapl_detect_powerlimit() will then
mark if PL1 is monitoring-only if power limit MSRs do not exist. Both AMD's RAPL
domains are monitoring-only and are correctly marked and handled.
Quoted 60 lines
thanks,
rui
Quoted 58 lines
drivers/powercap/intel_rapl_common.c +0 -0
 static const struct x86_cpu_id rapl_ids[] __initconst = {
      X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE,         &rapl_defau
lt
s_core),
X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X, &rapl_defau
lts_core),
drivers/powercap/intel_rapl_common.c +0 -0
@@ -1061,6 +1065,8 @@ static const struct x86_cpu_id rapl_ids[]
 __initconst = {
 
      X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL,        &rapl_defau
lts_hsw_se
rver),
X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM, &rapl_defau
lts_hsw_se
rver),
drivers/powercap/intel_rapl_common.c +2 -0
+
+     X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd),
      {}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
drivers/powercap/intel_rapl_msr.c +8 -0
index a819b3b89b2f..78213d4b5b16 100644
@@ -49,6 +49,14 @@ static struct rapl_if_priv rapl_msr_priv_intel
 = {
      .limits[RAPL_DOMAIN_PLATFORM] = 2,
 };
 
+static struct rapl_if_priv rapl_msr_priv_amd = {
+     .reg_unit = MSR_AMD_RAPL_POWER_UNIT,
+     .regs[RAPL_DOMAIN_PACKAGE] = {
+             0, MSR_AMD_PKG_ENERGY_STATUS, 0, 0, 0 },
+     .regs[RAPL_DOMAIN_PP0] = {
+             0, MSR_AMD_CORE_ENERGY_STATUS, 0, 0, 0 },
+};
+
 /* Handles CPU hotplug on multi-socket systems.
  * If a CPU goes online as the first CPU of the physical package
* we add the RAPL package to the system. Similarly, when the
last
drivers/powercap/intel_rapl_msr.c +0 -0
@@ -138,7 +146,17 @@ static int rapl_msr_probe(struct
platform_device
*pdev)
const struct x86_cpu_id *id =
x86_match_cpu(pl4_support_ids);
      int ret;
drivers/powercap/intel_rapl_msr.c +8 -1
-     rapl_msr_priv = &rapl_msr_priv_intel;
+     switch (boot_cpu_data.x86_vendor) {
+     case X86_VENDOR_INTEL:
+             rapl_msr_priv = &rapl_msr_priv_intel;
+             break;
+     case X86_VENDOR_AMD:
+             rapl_msr_priv = &rapl_msr_priv_amd;
+             break;
+     default:
Quoted 8 lines
drivers/powercap/intel_rapl_msr.c static int rapl_msr_probe(struct platform_device *pdev) · line 161
Quoted 8 lines
+ pr_err("intel-rapl does not support CPU vendor
%d\n",
boot_cpu_data.x86_vendor);
drivers/powercap/intel_rapl_msr.c +2 -0
+             return -ENODEV;
+     }
      rapl_msr_priv->read_raw = rapl_msr_read_raw;
      rapl_msr_priv->write_raw = rapl_msr_write_raw;
Best regards,
Victor Ding

Best regards,
Victor Ding

Srinivas Pandruvada

2020-11-04 02:17 UTC #8 lore raw

Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support

Quoted 22 lines · Victor Ding

Quoted 20 lines · Srinivas Pandruvada
Quoted 18 lines · Victor Ding
Quoted 16 lines · Zhang Rui
Quoted 15 lines · Victor Ding
This patch enables AMD Fam17h RAPL support for the power capping
framework. The support is as per AMD Fam17h Model31h (Zen2) and
model 00-ffh (Zen1) PPR.
Tested by comparing the results of following two sysfs entries
and
the
values directly read from corresponding MSRs via /dev/cpu/[x]/msr:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
rapl:0:0/energy_uj

Quoted 2 lines

Quoted 1 line
Is this for just energy reporting? No capping of power?
Correct, the hardware does not support capping of power.

I wonder if there is no capping, is this the right interface? Do you have specific user space, which cares about this?

I think these counters are already exposed via hwmon sysf.

Thanks,
Srinivas

Quoted 175 lines

Quoted 173 lines
Thanks,
Srinivas
Quoted 169 lines
Quoted 48 lines
Quoted 48 lines
Signed-off-by Victor Ding <[email protected]>
Acked-by Kim Phillips <[email protected]>
---
Changes in v3:
By Victor Ding <[email protected]>
- Rebased to the latest code.
- Created a new rapl_defaults for AMD CPUs.
- Removed redundant setting to zeros.
- Stopped using the fake power limit domain 1.
Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code change).
 arch/x86/include/asm/msr-index.h     |  1 +
 drivers/powercap/intel_rapl_common.c |  6 ++++++
 drivers/powercap/intel_rapl_msr.c    | 20
+++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
arch/x86/include/asm/msr-index.h +1 -0
index 21917e134ad4..c36a083c8ec0 100644
@@ -327,6 +327,7 @@
 #define MSR_PP1_POLICY                       0x00000642
 
 #define MSR_AMD_RAPL_POWER_UNIT              0xc0010299
+#define MSR_AMD_CORE_ENERGY_STATUS           0xc001029a
 #define MSR_AMD_PKG_ENERGY_STATUS    0xc001029b
 
 /* Config TDP MSRs */
drivers/powercap/intel_rapl_common.c +0 -0
index 0b2830efc574..bedd780bed12 100644
@@ -1011,6 +1011,10 @@ static const struct rapl_defaults
 rapl_defaults_cht = {
      .compute_time_window = rapl_compute_time_window_atom,
 };
 
Quoted 9 lines
drivers/powercap/intel_rapl_common.c static const struct rapl_defaults rapl_defaults_cht = { · line 1017
Quoted 4 lines
drivers/powercap/intel_rapl_common.c +4 -0
+static const struct rapl_defaults rapl_defaults_amd = {
+     .check_unit = rapl_check_unit_core,
+};
+
why do we need power_unit and time_unit if we only want to expose
the
energy counter?
AMD's Power Unit MSR provides identical information as Intel's, including
time units, power units, and energy status units. By reusing the check unit
method, we could avoid code duplication as well as easing future enhance-
ment when AMD starts to support power limits.
Quoted 5 lines
Plus, in rapl_init_domains(), PL1 is enabled for every RAPL Domain
blindly, I'm not sure how this is handled on the AMD CPUs. Is PL1 invalidated by rapl_detect_powerlimit()? or is it still registered as a valid constraint into powercap sysfs I/F?
AMD's CORE_ENERGY_STAT MSR is like Intel's PP0_ENERGY_STATUS; therefore, PL1 also always exists on AMD. rapl_detect_powerlimit()
correctly
markes the domain as monitoring-only after finding power limit MSRs
do not
exist.
Quoted 5 lines
Currently, the code makes the assumption that there is only on power
limit if priv->limits[domain_id] not set, we probably need to change
this if we want to support RAPL domains with no power limit.
The existing code already supports RAPL domains with no power limit:
PL1 is
enabled when there is zero or one power limit, rapl_detect_powerlimit() will then
mark if PL1 is monitoring-only if power limit MSRs do not exist. Both
AMD's RAPL
domains are monitoring-only and are correctly marked and handled.
Quoted 68 lines
thanks,
rui
Quoted 66 lines
drivers/powercap/intel_rapl_common.c +0 -0
 static const struct x86_cpu_id rapl_ids[] __initconst = {
      X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE,         &rapl_d
efau
lt
s_core),
X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X, &rapl_d
efau
lts_core),
drivers/powercap/intel_rapl_common.c +0 -0
@@ -1061,6 +1065,8 @@ static const struct x86_cpu_id
rapl_ids[]
__initconst = {
X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL, &rapl_d
efau
lts_hsw_se
rver),
X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM, &rapl_d
efau
lts_hsw_se
rver),
drivers/powercap/intel_rapl_common.c +2 -0
+
+     X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd),
      {}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
drivers/powercap/intel_rapl_msr.c +0 -0
index a819b3b89b2f..78213d4b5b16 100644
@@ -49,6 +49,14 @@ static struct rapl_if_priv
rapl_msr_priv_intel
= {
.limits[RAPL_DOMAIN_PLATFORM] = 2,
};
drivers/powercap/intel_rapl_msr.c +8 -0
+static struct rapl_if_priv rapl_msr_priv_amd = {
+     .reg_unit = MSR_AMD_RAPL_POWER_UNIT,
+     .regs[RAPL_DOMAIN_PACKAGE] = {
+             0, MSR_AMD_PKG_ENERGY_STATUS, 0, 0, 0 },
+     .regs[RAPL_DOMAIN_PP0] = {
+             0, MSR_AMD_CORE_ENERGY_STATUS, 0, 0, 0 },
+};
+
 /* Handles CPU hotplug on multi-socket systems.
  * If a CPU goes online as the first CPU of the physical
package
* we add the RAPL package to the system. Similarly, when
the
last
drivers/powercap/intel_rapl_msr.c +0 -0
@@ -138,7 +146,17 @@ static int rapl_msr_probe(struct
platform_device
*pdev)
const struct x86_cpu_id *id =
x86_match_cpu(pl4_support_ids);
      int ret;
drivers/powercap/intel_rapl_msr.c +8 -1
-     rapl_msr_priv = &rapl_msr_priv_intel;
+     switch (boot_cpu_data.x86_vendor) {
+     case X86_VENDOR_INTEL:
+             rapl_msr_priv = &rapl_msr_priv_intel;
+             break;
+     case X86_VENDOR_AMD:
+             rapl_msr_priv = &rapl_msr_priv_amd;
+             break;
+     default:
Quoted 8 lines
drivers/powercap/intel_rapl_msr.c static int rapl_msr_probe(struct platform_device *pdev) · line 161
Quoted 8 lines
+ pr_err("intel-rapl does not support CPU vendor
%d\n",
boot_cpu_data.x86_vendor);
drivers/powercap/intel_rapl_msr.c +2 -0
+             return -ENODEV;
+     }
      rapl_msr_priv->read_raw = rapl_msr_read_raw;
      rapl_msr_priv->write_raw = rapl_msr_write_raw;
Best regards,
Victor Ding
Best regards,
Victor Ding

Victor Ding

author

2020-11-05 03:54 UTC #9 lore raw

Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support

Quoted 24 lines · Srinivas Pandruvada

Quoted 22 lines · Victor Ding
Quoted 20 lines · Srinivas Pandruvada
Quoted 18 lines · Victor Ding
Quoted 16 lines · Zhang Rui
Quoted 15 lines · Victor Ding
This patch enables AMD Fam17h RAPL support for the power capping
framework. The support is as per AMD Fam17h Model31h (Zen2) and
model 00-ffh (Zen1) PPR.
Tested by comparing the results of following two sysfs entries
and
the
values directly read from corresponding MSRs via /dev/cpu/[x]/msr:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
rapl:0:0/energy_uj

Quoted 4 lines

Quoted 2 lines
Quoted 1 line
Is this for just energy reporting? No capping of power?
Correct, the hardware does not support capping of power.
I wonder if there is no capping, is this the right interface? Do you have specific user space, which cares about this?

We have tools that previously developed to measure energy status on Intel via the powercap interface. Powercap is the only interface allowing reading RAPL energy counters without requiring MSR access privileges. We want to use these tools on AMD with minimal modifications. I believe the powercap interface should support these counters, regardless of the use cases, mainly for two reasons:
1. Powercap interface already supports monitoring-only power domains, e.g. power limit is locked by BIOS or the (Intel) CPU does not expose an MSR for certain power domains. The latter is the exact situation on AMD;
2. As AMD has partially introduced the equivalent of Intel's RAPL, we should leverage this opportunity to reduce the divergence in the APIs. i.e. OS as a hardware abstraction layer should allow users to use the same set of APIs to access RAPL features if it issupported on both Intel and AMD. In this specific case, if users can query for Intel's RAPL counters via powercap, they should be able to do so as well for AMD's.

Quoted 2 lines

I think these counters are already exposed via hwmon sysf.

Yes, they were introduced early this year. However, it is not the same as the counters exposed via powercap interface: powercap exposes the actual value of the energy counters while hwmon adds an accumulation layer on top.
In addition, I don't think Intel's RAPL counters are exposed via hwmon;
therefore: 1. existing fine grade power monitoring tools are not based on hwmon; 2. new tools cannot query the same set of counters via the same API so that they have to actively maintain two sets of logic.

Quoted 180 lines

Thanks,
Srinivas
Quoted 175 lines
Quoted 173 lines
Thanks,
Srinivas
Quoted 169 lines
Quoted 48 lines
Quoted 48 lines
Signed-off-by Victor Ding <[email protected]>
Acked-by Kim Phillips <[email protected]>
---
Changes in v3:
By Victor Ding <[email protected]>
- Rebased to the latest code.
- Created a new rapl_defaults for AMD CPUs.
- Removed redundant setting to zeros.
- Stopped using the fake power limit domain 1.
Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code change).
 arch/x86/include/asm/msr-index.h     |  1 +
 drivers/powercap/intel_rapl_common.c |  6 ++++++
 drivers/powercap/intel_rapl_msr.c    | 20
+++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
arch/x86/include/asm/msr-index.h +1 -0
index 21917e134ad4..c36a083c8ec0 100644
@@ -327,6 +327,7 @@
 #define MSR_PP1_POLICY                       0x00000642
 
 #define MSR_AMD_RAPL_POWER_UNIT              0xc0010299
+#define MSR_AMD_CORE_ENERGY_STATUS           0xc001029a
 #define MSR_AMD_PKG_ENERGY_STATUS    0xc001029b
 
 /* Config TDP MSRs */
drivers/powercap/intel_rapl_common.c +0 -0
index 0b2830efc574..bedd780bed12 100644
@@ -1011,6 +1011,10 @@ static const struct rapl_defaults
 rapl_defaults_cht = {
      .compute_time_window = rapl_compute_time_window_atom,
 };
 
Quoted 9 lines
drivers/powercap/intel_rapl_common.c static const struct rapl_defaults rapl_defaults_cht = { · line 1017
Quoted 4 lines
drivers/powercap/intel_rapl_common.c +4 -0
+static const struct rapl_defaults rapl_defaults_amd = {
+     .check_unit = rapl_check_unit_core,
+};
+
why do we need power_unit and time_unit if we only want to expose
the
energy counter?
AMD's Power Unit MSR provides identical information as Intel's, including
time units, power units, and energy status units. By reusing the check unit
method, we could avoid code duplication as well as easing future enhance-
ment when AMD starts to support power limits.
Quoted 5 lines
Plus, in rapl_init_domains(), PL1 is enabled for every RAPL Domain
blindly, I'm not sure how this is handled on the AMD CPUs. Is PL1 invalidated by rapl_detect_powerlimit()? or is it still registered as a valid constraint into powercap sysfs I/F?
AMD's CORE_ENERGY_STAT MSR is like Intel's PP0_ENERGY_STATUS; therefore, PL1 also always exists on AMD. rapl_detect_powerlimit()
correctly
markes the domain as monitoring-only after finding power limit MSRs
do not
exist.
Quoted 5 lines
Currently, the code makes the assumption that there is only on power
limit if priv->limits[domain_id] not set, we probably need to change
this if we want to support RAPL domains with no power limit.
The existing code already supports RAPL domains with no power limit:
PL1 is
enabled when there is zero or one power limit, rapl_detect_powerlimit() will then
mark if PL1 is monitoring-only if power limit MSRs do not exist. Both
AMD's RAPL
domains are monitoring-only and are correctly marked and handled.
Quoted 68 lines
thanks,
rui
Quoted 66 lines
drivers/powercap/intel_rapl_common.c +0 -0
 static const struct x86_cpu_id rapl_ids[] __initconst = {
      X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE,         &rapl_d
efau
lt
s_core),
X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X, &rapl_d
efau
lts_core),
drivers/powercap/intel_rapl_common.c +0 -0
@@ -1061,6 +1065,8 @@ static const struct x86_cpu_id
rapl_ids[]
__initconst = {
X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL, &rapl_d
efau
lts_hsw_se
rver),
X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM, &rapl_d
efau
lts_hsw_se
rver),
drivers/powercap/intel_rapl_common.c +2 -0
+
+     X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd),
      {}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
drivers/powercap/intel_rapl_msr.c +0 -0
index a819b3b89b2f..78213d4b5b16 100644
@@ -49,6 +49,14 @@ static struct rapl_if_priv
rapl_msr_priv_intel
= {
.limits[RAPL_DOMAIN_PLATFORM] = 2,
};
drivers/powercap/intel_rapl_msr.c +8 -0
+static struct rapl_if_priv rapl_msr_priv_amd = {
+     .reg_unit = MSR_AMD_RAPL_POWER_UNIT,
+     .regs[RAPL_DOMAIN_PACKAGE] = {
+             0, MSR_AMD_PKG_ENERGY_STATUS, 0, 0, 0 },
+     .regs[RAPL_DOMAIN_PP0] = {
+             0, MSR_AMD_CORE_ENERGY_STATUS, 0, 0, 0 },
+};
+
 /* Handles CPU hotplug on multi-socket systems.
  * If a CPU goes online as the first CPU of the physical
package
* we add the RAPL package to the system. Similarly, when
the
last
drivers/powercap/intel_rapl_msr.c +0 -0
@@ -138,7 +146,17 @@ static int rapl_msr_probe(struct
platform_device
*pdev)
const struct x86_cpu_id *id =
x86_match_cpu(pl4_support_ids);
      int ret;
drivers/powercap/intel_rapl_msr.c +8 -1
-     rapl_msr_priv = &rapl_msr_priv_intel;
+     switch (boot_cpu_data.x86_vendor) {
+     case X86_VENDOR_INTEL:
+             rapl_msr_priv = &rapl_msr_priv_intel;
+             break;
+     case X86_VENDOR_AMD:
+             rapl_msr_priv = &rapl_msr_priv_amd;
+             break;
+     default:
Quoted 8 lines
drivers/powercap/intel_rapl_msr.c static int rapl_msr_probe(struct platform_device *pdev) · line 161
Quoted 8 lines
+ pr_err("intel-rapl does not support CPU vendor
%d\n",
boot_cpu_data.x86_vendor);
drivers/powercap/intel_rapl_msr.c +2 -0
+             return -ENODEV;
+     }
      rapl_msr_priv->read_raw = rapl_msr_read_raw;
      rapl_msr_priv->write_raw = rapl_msr_write_raw;
Best regards,
Victor Ding
Best regards,
Victor Ding

Best regards,
Victor Ding

Srinivas Pandruvada

2020-11-05 17:14 UTC #10 lore raw

Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support

Quoted 53 lines · Victor Ding

Quoted 25 lines · Srinivas Pandruvada
Quoted 24 lines · Victor Ding
Quoted 22 lines · Srinivas Pandruvada
Quoted 20 lines · Victor Ding
Quoted 17 lines · Zhang Rui
Quoted 16 lines · Victor Ding
This patch enables AMD Fam17h RAPL support for the power capping
framework. The support is as per AMD Fam17h Model31h (Zen2)
and
model 00-ffh (Zen1) PPR.
Tested by comparing the results of following two sysfs entries
and
the
values directly read from corresponding MSRs via /dev/cpu/[x]/msr:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
rapl:0:0/energy_uj
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
Is this for just energy reporting? No capping of power?
Correct, the hardware does not support capping of power.
I wonder if there is no capping, is this the right interface? Do you have specific user space, which cares about this?
We have tools that previously developed to measure energy status on Intel via the powercap interface. Powercap is the only interface allowing reading RAPL energy counters without requiring MSR access privileges. We want to use these tools on AMD with minimal modifications.
I believe the powercap interface should support these counters, regardless of the use cases, mainly for two reasons:
1. Powercap interface already supports monitoring-only power domains, e.g. power limit is locked by BIOS or the (Intel) CPU does not expose an
MSR for certain power domains. The latter is the exact situation on AMD;
2. As AMD has partially introduced the equivalent of Intel's RAPL, we should leverage this opportunity to reduce the divergence in the APIs. i.e.
OS as a hardware abstraction layer should allow users to use the same set of APIs to access RAPL features if it issupported on both Intel and AMD.
In this specific case, if users can query for Intel's RAPL counters via
powercap, they should be able to do so as well for AMD's.
Quoted 1 line
I think these counters are already exposed via hwmon sysf.

Quoted 12 lines

Yes, they were introduced early this year. However, it is not the same as
the counters exposed via powercap interface: powercap exposes the actual value of the energy counters while hwmon adds an accumulation layer on top.
In addition, I don't think Intel's RAPL counters are exposed via hwmon;
therefore: 1. existing fine grade power monitoring tools are not based on
hwmon; 2. new tools cannot query the same set of counters via the same
API so that they have to actively maintain two sets of logic.

Fine with me. I think eventually the power capping interface will be supported.

Thanks,
Srinivas

Quoted 199 lines

Quoted 197 lines
Thanks,
Srinivas
Quoted 194 lines
Quoted 192 lines
Thanks,
Srinivas
Quoted 188 lines
Quoted 49 lines
Quoted 49 lines
Signed-off-by Victor Ding <[email protected]>
Acked-by Kim Phillips <[email protected]>
---
Changes in v3:
By Victor Ding <[email protected]>
- Rebased to the latest code.
- Created a new rapl_defaults for AMD CPUs.
- Removed redundant setting to zeros.
- Stopped using the fake power limit domain 1.
Changes in v2:
By Kim Phillips <[email protected]>:
- Added Kim's Acked-by.
- Added Daniel Lezcano to Cc.
- (No code change).
 arch/x86/include/asm/msr-index.h     |  1 +
 drivers/powercap/intel_rapl_common.c |  6 ++++++
 drivers/powercap/intel_rapl_msr.c    | 20
+++++++++++++++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)
arch/x86/include/asm/msr-index.h +1 -0
index 21917e134ad4..c36a083c8ec0 100644
@@ -327,6 +327,7 @@
 #define MSR_PP1_POLICY                       0x00000642
 
 #define MSR_AMD_RAPL_POWER_UNIT              0xc0010299
+#define MSR_AMD_CORE_ENERGY_STATUS           0xc001029a
 #define MSR_AMD_PKG_ENERGY_STATUS    0xc001029b
 
 /* Config TDP MSRs */
drivers/powercap/intel_rapl_common.c +0 -0
index 0b2830efc574..bedd780bed12 100644
@@ -1011,6 +1011,10 @@ static const struct rapl_defaults
 rapl_defaults_cht = {
      .compute_time_window =
 rapl_compute_time_window_atom,
 };
 
Quoted 9 lines
drivers/powercap/intel_rapl_common.c static const struct rapl_defaults · line 1019
Quoted 4 lines
drivers/powercap/intel_rapl_common.c +4 -0
+static const struct rapl_defaults rapl_defaults_amd = {
+     .check_unit = rapl_check_unit_core,
+};
+
why do we need power_unit and time_unit if we only want to expose
the
energy counter?
AMD's Power Unit MSR provides identical information as Intel's,
including
time units, power units, and energy status units. By reusing the
check unit
method, we could avoid code duplication as well as easing future
enhance-
ment when AMD starts to support power limits.
Quoted 6 lines
Plus, in rapl_init_domains(), PL1 is enabled for every RAPL Domain
blindly, I'm not sure how this is handled on the AMD CPUs. Is PL1 invalidated by rapl_detect_powerlimit()? or is it still
registered as a valid constraint into powercap sysfs I/F?
AMD's CORE_ENERGY_STAT MSR is like Intel's PP0_ENERGY_STATUS; therefore, PL1 also always exists on AMD. rapl_detect_powerlimit()
correctly
markes the domain as monitoring-only after finding power limit
MSRs
do not
exist.
Quoted 8 lines
Currently, the code makes the assumption that there is only on
power
limit if priv->limits[domain_id] not set, we probably need to
change
this if we want to support RAPL domains with no power limit.
The existing code already supports RAPL domains with no power limit:
PL1 is
enabled when there is zero or one power limit, rapl_detect_powerlimit() will then
mark if PL1 is monitoring-only if power limit MSRs do not exist.
Both
AMD's RAPL
domains are monitoring-only and are correctly marked and handled.
Quoted 76 lines
thanks,
rui
Quoted 74 lines
drivers/powercap/intel_rapl_common.c +0 -0
 static const struct x86_cpu_id rapl_ids[] __initconst =
{
X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE, &ra
pl_d
efau
lt
s_core),
X86_MATCH_INTEL_FAM6_MODEL(SANDYBRIDGE_X, &ra
pl_d
efau
lts_core),
drivers/powercap/intel_rapl_common.c +0 -0
@@ -1061,6 +1065,8 @@ static const struct x86_cpu_id
rapl_ids[]
__initconst = {
X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNL, &ra
pl_d
efau
lts_hsw_se
rver),
X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM, &ra
pl_d
efau
lts_hsw_se
rver),
drivers/powercap/intel_rapl_common.c +2 -0
+
+     X86_MATCH_VENDOR_FAM(AMD, 0x17,
&rapl_defaults_amd),
      {}
};
MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
drivers/powercap/intel_rapl_msr.c +0 -0
index a819b3b89b2f..78213d4b5b16 100644
@@ -49,6 +49,14 @@ static struct rapl_if_priv
rapl_msr_priv_intel
= {
.limits[RAPL_DOMAIN_PLATFORM] = 2,
};
drivers/powercap/intel_rapl_msr.c +8 -0
+static struct rapl_if_priv rapl_msr_priv_amd = {
+     .reg_unit = MSR_AMD_RAPL_POWER_UNIT,
+     .regs[RAPL_DOMAIN_PACKAGE] = {
+             0, MSR_AMD_PKG_ENERGY_STATUS, 0, 0, 0 },
+     .regs[RAPL_DOMAIN_PP0] = {
+             0, MSR_AMD_CORE_ENERGY_STATUS, 0, 0, 0 },
+};
+
 /* Handles CPU hotplug on multi-socket systems.
  * If a CPU goes online as the first CPU of the physical
package
* we add the RAPL package to the system. Similarly,
when
the
last
drivers/powercap/intel_rapl_msr.c +0 -0
@@ -138,7 +146,17 @@ static int rapl_msr_probe(struct
platform_device
*pdev)
const struct x86_cpu_id *id =
x86_match_cpu(pl4_support_ids);
      int ret;
drivers/powercap/intel_rapl_msr.c +9 -1
-     rapl_msr_priv = &rapl_msr_priv_intel;
+     switch (boot_cpu_data.x86_vendor) {
+     case X86_VENDOR_INTEL:
+             rapl_msr_priv = &rapl_msr_priv_intel;
+             break;
+     case X86_VENDOR_AMD:
+             rapl_msr_priv = &rapl_msr_priv_amd;
+             break;
+     default:
+             pr_err("intel-rapl does not support CPU
Quoted 8 lines
drivers/powercap/intel_rapl_msr.c
Quoted 8 lines
vendor
%d\n",
boot_cpu_data.x86_vendor);
drivers/powercap/intel_rapl_msr.c +2 -0
+             return -ENODEV;
+     }
      rapl_msr_priv->read_raw = rapl_msr_read_raw;
      rapl_msr_priv->write_raw = rapl_msr_write_raw;
Best regards,
Victor Ding
Best regards,
Victor Ding
Best regards,
Victor Ding

Rafael J. Wysocki

2020-11-05 18:04 UTC #11 lore raw

Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support

Quoted 67 lines · CET Srinivas Pandruvada

Quoted 53 lines · Victor Ding
Quoted 25 lines · Srinivas Pandruvada
Quoted 24 lines · Victor Ding
Quoted 22 lines · Srinivas Pandruvada
Quoted 20 lines · Victor Ding
Quoted 17 lines · Zhang Rui
Quoted 16 lines · Victor Ding
This patch enables AMD Fam17h RAPL support for the power capping
framework. The support is as per AMD Fam17h Model31h (Zen2)
and
model 00-ffh (Zen1) PPR.
Tested by comparing the results of following two sysfs entries
and
the
values directly read from corresponding MSRs via /dev/cpu/[x]/msr:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-
rapl:0:0/energy_uj
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
Is this for just energy reporting? No capping of power?
Correct, the hardware does not support capping of power.
I wonder if there is no capping, is this the right interface? Do you have specific user space, which cares about this?
We have tools that previously developed to measure energy status on Intel via the powercap interface. Powercap is the only interface allowing reading RAPL energy counters without requiring MSR access privileges. We want to use these tools on AMD with minimal modifications.
I believe the powercap interface should support these counters, regardless of the use cases, mainly for two reasons:
1. Powercap interface already supports monitoring-only power domains, e.g. power limit is locked by BIOS or the (Intel) CPU does not expose an
MSR for certain power domains. The latter is the exact situation on AMD;
2. As AMD has partially introduced the equivalent of Intel's RAPL, we should leverage this opportunity to reduce the divergence in the APIs. i.e.
OS as a hardware abstraction layer should allow users to use the same set of APIs to access RAPL features if it issupported on both Intel and AMD.
In this specific case, if users can query for Intel's RAPL counters via
powercap, they should be able to do so as well for AMD's.
Quoted 1 line
I think these counters are already exposed via hwmon sysf.
Quoted 12 lines
Yes, they were introduced early this year. However, it is not the same as
the counters exposed via powercap interface: powercap exposes the actual value of the energy counters while hwmon adds an accumulation layer on top.
In addition, I don't think Intel's RAPL counters are exposed via hwmon;
therefore: 1. existing fine grade power monitoring tools are not based on
hwmon; 2. new tools cannot query the same set of counters via the same
API so that they have to actively maintain two sets of logic.

Quoted 1 line

Fine with me.

OK, I'll queue up the series for 5.11 then if there are no other concerns.

Thanks!

Victor Ding

author

2020-10-27 07:25 UTC #12 lore raw

PATCH v3 4/4 powercap: Add AMD Fam19h RAPL support

In reply to Victor Ding

From: Kim Phillips <[email protected]>

AMD Family 19h's RAPL MSRs are identical to Family 17h's. Extend Family 17h's support to Family 19h.

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Victor Ding <[email protected]>

---

Changes in v2:
By Kim Phillips <[email protected]>
- Added the Fam19h patch to the end of the series
- Added my Acked-by
- Added Daniel Lezcano to Cc
- (linux-pm was already on Cc)
- (No code changes)
By Kim Phillips <[email protected]>:
- Added Daniel Lezcano to Cc.
- (No code changes).

 drivers/powercap/intel_rapl_common.c | 1 +
 1 file changed, 1 insertion(+)

drivers/powercap/intel_rapl_common.c +1 -0

index bedd780bed12..264872f7f46d 100644
@@ -1067,6 +1067,7 @@ static const struct x86_cpu_id rapl_ids[] __initconst = {
 	X86_MATCH_INTEL_FAM6_MODEL(XEON_PHI_KNM,	&rapl_defaults_hsw_server),
 
 	X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd),
+	X86_MATCH_VENDOR_FAM(AMD, 0x19, &rapl_defaults_amd),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);

Signature · 1 line

2.29.0.rc2.309.g374f81d7ae-goog

Rafael J. Wysocki

maintainer

2020-11-10 19:24 UTC #13 lore raw

In reply to Victor Ding

Quoted 35 lines · Victor Ding

This patch series adds support for AMD Fam17h RAPL counters. As per AMD PPR, Fam17h and Fam19h support RAPL counters to monitor power usage. The RAPL counter operates as with Intel RAPL. Therefore, it is beneficial to re-use existing framework for Intel, especially to allow existing tools to seamlessly run on AMD.
From the user's point view, this series enables the following two sysfs entry on AMD Fam17h or Fam19h:
/sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj /sys/class/powercap/intel-rapl/intel-rapl:0/intel-rapl:0:0/energy_uj
Changes in v3:
By Victor Ding <[email protected]>
- Rebased to the latest code.
- Created a new rapl_defaults for AMD CPUs.
- Removed redundant setting to zeros.
- Stopped using the fake power limit domain 1.
Changes in v2:
By Kim Phillips <[email protected]>
- Added the Fam19h patch to the end of the series
- Added my Acked-by
- Added Daniel Lezcano to Cc
- (linux-pm was already on Cc)
- (No code changes)
Kim Phillips (1):
powercap: Add AMD Fam19h RAPL support
Victor Ding (3):
x86/msr-index: sort AMD RAPL MSRs by address powercap/intel_rapl_msr: Convert rapl_msr_priv into pointer
powercap: Add AMD Fam17h RAPL support

Quoted 6 lines

 arch/x86/include/asm/msr-index.h     |  3 +-
 drivers/powercap/intel_rapl_common.c |  7 ++++
 drivers/powercap/intel_rapl_msr.c    | 51 ++++++++++++++++++++--------
 3 files changed, 45 insertions(+), 16 deletions(-)

All patches applied as 5.11 material (which some minor edits in the changelogs), thanks!