From: Victor Ding <[email protected]>
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 <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Daniel Lezcano <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Pawan Gupta <[email protected]>
Cc: "Peter Zijlstra (Intel)" <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Vineela Tummalapalli <[email protected]>
Cc: LKML <[email protected]>
Cc: [email protected]
Cc: [email protected]
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/[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)
--
2.27.0
From: Victor Ding <[email protected]>
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]>
Cc: Victor Ding <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Daniel Lezcano <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Pawan Gupta <[email protected]>
Cc: "Peter Zijlstra (Intel)" <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Vineela Tummalapalli <[email protected]>
Cc: LKML <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
From: Victor Ding <[email protected]>
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 <[email protected]>
Acked-by: Kim Phillips <[email protected]>
Cc: Victor Ding <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Daniel Lezcano <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Pawan Gupta <[email protected]>
Cc: "Peter Zijlstra (Intel)" <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Vineela Tummalapalli <[email protected]>
Cc: LKML <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
From: Victor Ding <[email protected]>
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]>
Cc: Victor Ding <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Daniel Lezcano <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Pawan Gupta <[email protected]>
Cc: "Peter Zijlstra (Intel)" <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Vineela Tummalapalli <[email protected]>
Cc: LKML <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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]>
Cc: Victor Ding <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Daniel Lezcano <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Josh Poimboeuf <[email protected]>
Cc: Pawan Gupta <[email protected]>
Cc: "Peter Zijlstra (Intel)" <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Vineela Tummalapalli <[email protected]>
Cc: LKML <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
On Wed, 2020-10-07 at 11:14 -0500, Kim Phillips wrote:
> From: Victor Ding <[email protected]>
>
> 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]>
> Cc: Victor Ding <[email protected]>
> Cc: Alexander Shishkin <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Daniel Lezcano <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Josh Poimboeuf <[email protected]>
> Cc: Pawan Gupta <[email protected]>
> Cc: "Peter Zijlstra (Intel)" <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Vineela Tummalapalli <[email protected]>
> Cc: LKML <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> 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
On Fri, Oct 9, 2020 at 2:47 PM Zhang Rui <[email protected]> wrote:
>
> On Wed, 2020-10-07 at 11:14 -0500, Kim Phillips wrote:
> > From: Victor Ding <[email protected]>
> >
> > 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]>
> > Cc: Victor Ding <[email protected]>
> > Cc: Alexander Shishkin <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Daniel Lezcano <[email protected]>
> > Cc: "H. Peter Anvin" <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Josh Poimboeuf <[email protected]>
> > Cc: Pawan Gupta <[email protected]>
> > Cc: "Peter Zijlstra (Intel)" <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Sean Christopherson <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Tony Luck <[email protected]>
> > Cc: Vineela Tummalapalli <[email protected]>
> > Cc: LKML <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > ---
> > 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