[PATCH v2 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h

[PATCH v2 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h

[Kim Phillips]

From: Victor Ding <victording@google.com>

This patch series adds support for AMD Fam17h RAPL counters. As per
AMD PPR, Fam17h 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:
  /sys/class/powercap/intel-rapl/intel-rapl:0/energy_uj
  /sys/class/powercap/intel-rapl/intel-rapl:0/intel-rapl:0:0/energy_uj

Cc: Victor Ding <victording@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Vineela Tummalapalli <vineela.tummalapalli@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: linux-pm@vger.kernel.org
Cc: x86@kernel.org

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 |  3 ++
 drivers/powercap/intel_rapl_msr.c    | 62 ++++++++++++++++++++--------
 3 files changed, 50 insertions(+), 18 deletions(-)

v2: Kim's changes from Victor's original submission:

https://lore.kernel.org/lkml/20200729105206.2991064-1-victording@google.com/

- 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)
-- 
2.27.0

[PATCH v2 1/4] x86/msr-index: sort AMD RAPL MSRs by address

[Kim Phillips]

From: Victor Ding <victording@google.com>

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

No functional changes.

Signed-off-by: Victor Ding <victording@google.com>
Acked-by: Kim Phillips <kim.phillips@amd.com>
Cc: Victor Ding <victording@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Vineela Tummalapalli <vineela.tummalapalli@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: linux-pm@vger.kernel.org
Cc: x86@kernel.org
---
Kim's changes from Victor's original submission:

https://lore.kernel.org/lkml/20200729205144.1.I8556c0b9b6f75bf3121989f5641c33e694fff8d9@changeid/

 - Added my Acked-by.
 - Added Daniel Lezcano to Cc.
 arch/x86/include/asm/msr-index.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 2859ee4f39a8..f1b24f1b774d 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -323,8 +323,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
-- 
2.27.0

[PATCH v2 2/4] powercap/intel_rapl_msr: Convert rapl_msr_priv into pointer

[Kim Phillips]

From: Victor Ding <victording@google.com>

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

No functional changes.

Signed-off-by: Victor Ding <victording@google.com>
Acked-by: Kim Phillips <kim.phillips@amd.com>
Cc: Victor Ding <victording@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Vineela Tummalapalli <vineela.tummalapalli@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: linux-pm@vger.kernel.org
Cc: x86@kernel.org
---
Kim's changes from Victor's original submission:

https://lore.kernel.org/lkml/20200729205144.2.I4cb96a95365506b77761c1416258672a7556b595@changeid/

 - Added my Acked-by.
 - Added Daniel Lezcano to Cc.

 drivers/powercap/intel_rapl_msr.c | 37 +++++++++++++++++--------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c
index d2a2627507a9..c68ef5e4e1c4 100644
--- a/drivers/powercap/intel_rapl_msr.c
+++ b/drivers/powercap/intel_rapl_msr.c
@@ -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 },
@@ -57,9 +59,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);
 	}
@@ -72,7 +74,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;
 
@@ -135,44 +137,45 @@ 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;
 
 	/* Don't bail out if PSys is not supported */
-	rapl_add_platform_domain(&rapl_msr_priv);
+	rapl_add_platform_domain(rapl_msr_priv);
 
 	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);
-	rapl_remove_platform_domain(&rapl_msr_priv);
-	powercap_unregister_control_type(rapl_msr_priv.control_type);
+	cpuhp_remove_state(rapl_msr_priv->pcap_rapl_online);
+	rapl_remove_platform_domain(rapl_msr_priv);
+	powercap_unregister_control_type(rapl_msr_priv->control_type);
 	return 0;
 }
 
-- 
2.27.0

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

[Kim Phillips]

From: Victor Ding <victording@google.com>

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 <victording@google.com>
Acked-by: Kim Phillips <kim.phillips@amd.com>
Cc: Victor Ding <victording@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Vineela Tummalapalli <vineela.tummalapalli@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: linux-pm@vger.kernel.org
Cc: x86@kernel.org
---
Kim's changes from Victor's original submission:

https://lore.kernel.org/lkml/20200729205144.3.I01b89fb23d7498521c84cfdf417450cbbfca46bb@changeid/

 - Added my Acked-by.
 - Added Daniel Lezcano to Cc.

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

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index f1b24f1b774d..c0646f69d2a5 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -324,6 +324,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 */
diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
index 983d75bd5bd1..6905ccffcec3 100644
--- a/drivers/powercap/intel_rapl_common.c
+++ b/drivers/powercap/intel_rapl_common.c
@@ -1054,6 +1054,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_core),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c
index c68ef5e4e1c4..dcaef917f79d 100644
--- a/drivers/powercap/intel_rapl_msr.c
+++ b/drivers/powercap/intel_rapl_msr.c
@@ -48,6 +48,21 @@ static struct rapl_if_priv rapl_msr_priv_intel = {
 	.limits[RAPL_DOMAIN_PACKAGE] = 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 },
+	.regs[RAPL_DOMAIN_PP1] = {
+		0, 0, 0, 0, 0 },
+	.regs[RAPL_DOMAIN_DRAM] = {
+		0, 0, 0, 0, 0 },
+	.regs[RAPL_DOMAIN_PLATFORM] = {
+		0, 0, 0, 0, 0},
+	.limits[RAPL_DOMAIN_PACKAGE] = 1,
+};
+
 /* 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
@@ -137,7 +152,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;
 
-- 
2.27.0

[PATCH v2 4/4] powercap: Add AMD Fam19h RAPL support

[Kim Phillips]

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 <kim.phillips@amd.com>
Cc: Victor Ding <victording@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Vineela Tummalapalli <vineela.tummalapalli@intel.com>
Cc: LKML <linux-kernel@vger.kernel.org>
Cc: linux-pm@vger.kernel.org
Cc: x86@kernel.org
---
Change from the original submission:

https://lore.kernel.org/patchwork/patch/1289843/

 - Added Daniel Lezcano to Cc.

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

diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c
index 6905ccffcec3..56894f4cd394 100644
--- a/drivers/powercap/intel_rapl_common.c
+++ b/drivers/powercap/intel_rapl_common.c
@@ -1056,6 +1056,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_core),
+	X86_MATCH_VENDOR_FAM(AMD, 0x19, &rapl_defaults_core),
 	{}
 };
 MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
-- 
2.27.0

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

[Zhang Rui]

On Wed, 2020-10-07 at 11:14 -0500, Kim Phillips wrote:
> From: Victor Ding <victording@google.com>
> 
> 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 <victording@google.com>
> Acked-by: Kim Phillips <kim.phillips@amd.com>
> Cc: Victor Ding <victording@google.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Vineela Tummalapalli <vineela.tummalapalli@intel.com>
> Cc: LKML <linux-kernel@vger.kernel.org>
> Cc: linux-pm@vger.kernel.org
> Cc: x86@kernel.org
> ---
> Kim's changes from Victor's original submission:
> 
> 
https://lore.kernel.org/lkml/20200729205144.3.I01b89fb23d7498521c84cfdf417450cbbfca46bb@changeid/
> 
>  - Added my Acked-by.
>  - Added Daniel Lezcano to Cc.
> 
>  arch/x86/include/asm/msr-index.h     |  1 +
>  drivers/powercap/intel_rapl_common.c |  2 ++
>  drivers/powercap/intel_rapl_msr.c    | 27
> ++++++++++++++++++++++++++-
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/msr-index.h
> b/arch/x86/include/asm/msr-index.h
> index f1b24f1b774d..c0646f69d2a5 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -324,6 +324,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 */
> diff --git a/drivers/powercap/intel_rapl_common.c
> b/drivers/powercap/intel_rapl_common.c
> index 983d75bd5bd1..6905ccffcec3 100644
> --- a/drivers/powercap/intel_rapl_common.c
> +++ b/drivers/powercap/intel_rapl_common.c
> @@ -1054,6 +1054,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_core),

I double if we can use rapl_defaults_core here.

static const struct rapl_defaults rapl_defaults_core = {
        .floor_freq_reg_addr = 0,
        .check_unit = rapl_check_unit_core,
        .set_floor_freq = set_floor_freq_default,
        .compute_time_window = rapl_compute_time_window_core,
};

        .floor_freq_reg_addr = 0,
is redundant here, even for rapl_defaults_core, we can remove it.

        .check_unit = rapl_check_unit_core,
the Intel UNIT MSR supports three units including Energy/Power/Time.
From the change below, only the energy counter is supported, so you may
need to confirm if all the three units are supported or not.

        .set_floor_freq = set_floor_freq_default,this function sets PL1_CLAMP bit on RAPL_DOMAIN_REG_LIMIT, but RAPL_DOMAIN_REG_LIMIT is not supported on the AMD cpus.

        .compute_time_window = rapl_compute_time_window_core,
this is used for setting the power limits, which is not supported on
the AMD cpus.

IMO, it's better to use a new rapl_defaults that contains valid
callbacks for AMD cpus.

>  	{}
>  };
>  MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
> diff --git a/drivers/powercap/intel_rapl_msr.c
> b/drivers/powercap/intel_rapl_msr.c
> index c68ef5e4e1c4..dcaef917f79d 100644
> --- a/drivers/powercap/intel_rapl_msr.c
> +++ b/drivers/powercap/intel_rapl_msr.c
> @@ -48,6 +48,21 @@ static struct rapl_if_priv rapl_msr_priv_intel = {
>  	.limits[RAPL_DOMAIN_PACKAGE] = 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 },
> +	.regs[RAPL_DOMAIN_PP1] = {
> +		0, 0, 0, 0, 0 },
> +	.regs[RAPL_DOMAIN_DRAM] = {
> +		0, 0, 0, 0, 0 },
> +	.regs[RAPL_DOMAIN_PLATFORM] = {
> +		0, 0, 0, 0, 0},

I don't think you need to set the PP1/DRAM/PLATFORM registers to 0 explicitly if they are not supported.

> +	.limits[RAPL_DOMAIN_PACKAGE] = 1,


Is Pkg Domain PL1 really supported?
At least according to this patch, I don't think so. So if power limit
is supported, it is better to add the support in this patch altogether.

If no, we're actually exposing energy counters only. If this is the
case, I'm not sure if it is proper to do this in powercap class because
we can not do powercap actually. Or at least, if we want to support
power zones with no power limits, we should enhance the code to
support this rather than fake a power limit.

thanks,
rui
> +};
> +
>  /* 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
> @@ -137,7 +152,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;
>  



IF

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

[Victor Ding]

On Fri, Oct 9, 2020 at 2:47 PM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On Wed, 2020-10-07 at 11:14 -0500, Kim Phillips wrote:
> > From: Victor Ding <victording@google.com>
> >
> > 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 <victording@google.com>
> > Acked-by: Kim Phillips <kim.phillips@amd.com>
> > Cc: Victor Ding <victording@google.com>
> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> > Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
> > Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Tony Luck <tony.luck@intel.com>
> > Cc: Vineela Tummalapalli <vineela.tummalapalli@intel.com>
> > Cc: LKML <linux-kernel@vger.kernel.org>
> > Cc: linux-pm@vger.kernel.org
> > Cc: x86@kernel.org
> > ---
> > Kim's changes from Victor's original submission:
> >
> >
> https://lore.kernel.org/lkml/20200729205144.3.I01b89fb23d7498521c84cfdf417450cbbfca46bb@changeid/
> >
> >  - Added my Acked-by.
> >  - Added Daniel Lezcano to Cc.
> >
> >  arch/x86/include/asm/msr-index.h     |  1 +
> >  drivers/powercap/intel_rapl_common.c |  2 ++
> >  drivers/powercap/intel_rapl_msr.c    | 27
> > ++++++++++++++++++++++++++-
> >  3 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/include/asm/msr-index.h
> > b/arch/x86/include/asm/msr-index.h
> > index f1b24f1b774d..c0646f69d2a5 100644
> > --- a/arch/x86/include/asm/msr-index.h
> > +++ b/arch/x86/include/asm/msr-index.h
> > @@ -324,6 +324,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 */
> > diff --git a/drivers/powercap/intel_rapl_common.c
> > b/drivers/powercap/intel_rapl_common.c
> > index 983d75bd5bd1..6905ccffcec3 100644
> > --- a/drivers/powercap/intel_rapl_common.c
> > +++ b/drivers/powercap/intel_rapl_common.c
> > @@ -1054,6 +1054,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_core),
>
> I double if we can use rapl_defaults_core here.
>
> static const struct rapl_defaults rapl_defaults_core = {
>         .floor_freq_reg_addr = 0,
>         .check_unit = rapl_check_unit_core,
>         .set_floor_freq = set_floor_freq_default,
>         .compute_time_window = rapl_compute_time_window_core,
> };
>
>         .floor_freq_reg_addr = 0,
> is redundant here, even for rapl_defaults_core, we can remove it.
>
>         .check_unit = rapl_check_unit_core,
> the Intel UNIT MSR supports three units including Energy/Power/Time.
> From the change below, only the energy counter is supported, so you may
> need to confirm if all the three units are supported or not.
>
>         .set_floor_freq = set_floor_freq_default,this function sets PL1_CLAMP bit on RAPL_DOMAIN_REG_LIMIT, but RAPL_DOMAIN_REG_LIMIT is not supported on the AMD cpus.
>
>         .compute_time_window = rapl_compute_time_window_core,
> this is used for setting the power limits, which is not supported on
> the AMD cpus.
>
> IMO, it's better to use a new rapl_defaults that contains valid
> callbacks for AMD cpus.
Good point. The only reason why I proposed to re-use rapl_defaults_core was
that "check_unit" is the only function needed here, and it is the same
as Intel's.
The rest of callbacks are not used at all since setting the power limits is not
supported on AMD CPUs. Let's create a new callback for AMD. It should be like:
static const struct rapl_defaults rapl_defaults_amd = {
         .check_unit = rapl_check_unit_core,
};
The AMD UNIT MSR behaves the same as Intel's: [3:0] is Power Units, [12:8] is
Energy Status Units, and [19:16] is Time Units.
>
> >       {}
> >  };
> >  MODULE_DEVICE_TABLE(x86cpu, rapl_ids);
> > diff --git a/drivers/powercap/intel_rapl_msr.c
> > b/drivers/powercap/intel_rapl_msr.c
> > index c68ef5e4e1c4..dcaef917f79d 100644
> > --- a/drivers/powercap/intel_rapl_msr.c
> > +++ b/drivers/powercap/intel_rapl_msr.c
> > @@ -48,6 +48,21 @@ static struct rapl_if_priv rapl_msr_priv_intel = {
> >       .limits[RAPL_DOMAIN_PACKAGE] = 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 },
> > +     .regs[RAPL_DOMAIN_PP1] = {
> > +             0, 0, 0, 0, 0 },
> > +     .regs[RAPL_DOMAIN_DRAM] = {
> > +             0, 0, 0, 0, 0 },
> > +     .regs[RAPL_DOMAIN_PLATFORM] = {
> > +             0, 0, 0, 0, 0},
>
> I don't think you need to set the PP1/DRAM/PLATFORM registers to 0 explicitly if they are not supported.
Good suggestion. Let's remove the zeros.
>
> > +     .limits[RAPL_DOMAIN_PACKAGE] = 1,
>
>
> Is Pkg Domain PL1 really supported?
> At least according to this patch, I don't think so. So if power limit
> is supported, it is better to add the support in this patch altogether.
>
> If no, we're actually exposing energy counters only. If this is the
> case, I'm not sure if it is proper to do this in powercap class because
> we can not do powercap actually. Or at least, if we want to support
> power zones with no power limits, we should enhance the code to
> support this rather than fake a power limit.
>
Correct, this is solely to expose energy counters. Many existing tools
are built on
top of powercap's sysfs to query energy counters, even though they don't set the
power limit. Exposing the energy counters through the same powercap interface
allows these tools built for Intel run seamlessly on AMD. Hence, I
believe powercap
is the best place to expose AMD's energy counters.

I like your idea of enhancing the code to avoid using a fake power
limit. I'll make
the update.
> thanks,
> rui
> > +};
> > +
> >  /* 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
> > @@ -137,7 +152,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;
> >
>
>
>
> IF
>
>
>
Best regards,
Victor Ding