* [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h
@ 2020-10-27 7:23 Victor Ding
2020-10-27 7:23 ` [PATCH v3 1/4] x86/msr-index: sort AMD RAPL MSRs by address Victor Ding
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Victor Ding @ 2020-10-27 7:23 UTC (permalink / raw)
To: linux-kernel
Cc: Daniel Lezcano, Kim Phillips, Zhang Rui, linux-pm, Victor Ding,
Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel,
Kan Liang, Pawan Gupta, Peter Zijlstra (Intel),
Rafael J. Wysocki, Sean Christopherson, Srinivas Pandruvada,
Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86
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 <victording@google.com>
- 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 <kim.phillips@amd.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)
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(-)
--
2.29.0.rc2.309.g374f81d7ae-goog
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v3 1/4] x86/msr-index: sort AMD RAPL MSRs by address 2020-10-27 7:23 [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h Victor Ding @ 2020-10-27 7:23 ` Victor Ding 2020-10-27 7:23 ` [PATCH v3 2/4] powercap/intel_rapl_msr: Convert rapl_msr_priv into pointer Victor Ding ` (3 subsequent siblings) 4 siblings, 0 replies; 14+ messages in thread From: Victor Ding @ 2020-10-27 7:23 UTC (permalink / raw) To: linux-kernel Cc: Daniel Lezcano, Kim Phillips, Zhang Rui, linux-pm, Victor Ding, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Sean Christopherson, Srinivas Pandruvada, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 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> --- (no changes since v2) Changes in v2: By Kim Phillips <kim.phillips@amd.com>: - 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(-) diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index 972a34d93505..21917e134ad4 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -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 -- 2.29.0.rc2.309.g374f81d7ae-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 2/4] powercap/intel_rapl_msr: Convert rapl_msr_priv into pointer 2020-10-27 7:23 [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h Victor Ding 2020-10-27 7:23 ` [PATCH v3 1/4] x86/msr-index: sort AMD RAPL MSRs by address Victor Ding @ 2020-10-27 7:23 ` Victor Ding 2020-10-27 7:23 ` [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support Victor Ding ` (2 subsequent siblings) 4 siblings, 0 replies; 14+ messages in thread From: Victor Ding @ 2020-10-27 7:23 UTC (permalink / raw) To: linux-kernel Cc: Daniel Lezcano, Kim Phillips, Zhang Rui, linux-pm, Victor Ding, Rafael J. Wysocki 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 <victording@google.com> Acked-by: Kim Phillips <kim.phillips@amd.com> --- (no changes since v2) Changes in v2: By Kim Phillips <kim.phillips@amd.com>: - 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(-) diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c index 1646808d354c..a819b3b89b2f 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 }, @@ -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; } -- 2.29.0.rc2.309.g374f81d7ae-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support 2020-10-27 7:23 [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h Victor Ding 2020-10-27 7:23 ` [PATCH v3 1/4] x86/msr-index: sort AMD RAPL MSRs by address Victor Ding 2020-10-27 7:23 ` [PATCH v3 2/4] powercap/intel_rapl_msr: Convert rapl_msr_priv into pointer Victor Ding @ 2020-10-27 7:23 ` Victor Ding 2020-11-02 1:38 ` Zhang Rui 2020-10-27 7:23 ` [PATCH v3 4/4] powercap: Add AMD Fam19h " Victor Ding 2020-11-10 19:24 ` [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h Rafael J. Wysocki 4 siblings, 1 reply; 14+ messages in thread From: Victor Ding @ 2020-10-27 7:23 UTC (permalink / raw) To: linux-kernel Cc: Daniel Lezcano, Kim Phillips, Zhang Rui, linux-pm, Victor Ding, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Rafael J. Wysocki, Sean Christopherson, Srinivas Pandruvada, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 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> --- Changes in v3: By Victor Ding <victording@google.com> - 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 <kim.phillips@amd.com>: - 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(-) diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h index 21917e134ad4..c36a083c8ec0 100644 --- a/arch/x86/include/asm/msr-index.h +++ b/arch/x86/include/asm/msr-index.h @@ -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 */ diff --git a/drivers/powercap/intel_rapl_common.c b/drivers/powercap/intel_rapl_common.c index 0b2830efc574..bedd780bed12 100644 --- a/drivers/powercap/intel_rapl_common.c +++ b/drivers/powercap/intel_rapl_common.c @@ -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); diff --git a/drivers/powercap/intel_rapl_msr.c b/drivers/powercap/intel_rapl_msr.c index a819b3b89b2f..78213d4b5b16 100644 --- a/drivers/powercap/intel_rapl_msr.c +++ b/drivers/powercap/intel_rapl_msr.c @@ -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; -- 2.29.0.rc2.309.g374f81d7ae-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support 2020-10-27 7:23 ` [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support Victor Ding @ 2020-11-02 1:38 ` Zhang Rui 2020-11-03 6:10 ` Victor Ding 0 siblings, 1 reply; 14+ messages in thread From: Zhang Rui @ 2020-11-02 1:38 UTC (permalink / raw) To: Victor Ding, linux-kernel Cc: Daniel Lezcano, Kim Phillips, linux-pm, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Rafael J. Wysocki, Sean Christopherson, Srinivas Pandruvada, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 On Tue, 2020-10-27 at 07:23 +0000, Victor Ding wrote: > 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> > > > --- > > Changes in v3: > By Victor Ding <victording@google.com> > - 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 <kim.phillips@amd.com>: > - 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(-) > > diff --git a/arch/x86/include/asm/msr-index.h > b/arch/x86/include/asm/msr-index.h > index 21917e134ad4..c36a083c8ec0 100644 > --- a/arch/x86/include/asm/msr-index.h > +++ b/arch/x86/include/asm/msr-index.h > @@ -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 */ > diff --git a/drivers/powercap/intel_rapl_common.c > b/drivers/powercap/intel_rapl_common.c > index 0b2830efc574..bedd780bed12 100644 > --- a/drivers/powercap/intel_rapl_common.c > +++ b/drivers/powercap/intel_rapl_common.c > @@ -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 > 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), > @@ -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); > diff --git a/drivers/powercap/intel_rapl_msr.c > b/drivers/powercap/intel_rapl_msr.c > index a819b3b89b2f..78213d4b5b16 100644 > --- a/drivers/powercap/intel_rapl_msr.c > +++ b/drivers/powercap/intel_rapl_msr.c > @@ -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; > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support 2020-11-02 1:38 ` Zhang Rui @ 2020-11-03 6:10 ` Victor Ding 2020-11-03 17:09 ` Srinivas Pandruvada 0 siblings, 1 reply; 14+ messages in thread From: Victor Ding @ 2020-11-03 6:10 UTC (permalink / raw) To: Zhang Rui Cc: LKML, Daniel Lezcano, Kim Phillips, linux-pm, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Rafael J. Wysocki, Sean Christopherson, Srinivas Pandruvada, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui <rui.zhang@intel.com> wrote: > > On Tue, 2020-10-27 at 07:23 +0000, Victor Ding wrote: > > 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> > > > > > > --- > > > > Changes in v3: > > By Victor Ding <victording@google.com> > > - 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 <kim.phillips@amd.com>: > > - 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(-) > > > > diff --git a/arch/x86/include/asm/msr-index.h > > b/arch/x86/include/asm/msr-index.h > > index 21917e134ad4..c36a083c8ec0 100644 > > --- a/arch/x86/include/asm/msr-index.h > > +++ b/arch/x86/include/asm/msr-index.h > > @@ -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 */ > > diff --git a/drivers/powercap/intel_rapl_common.c > > b/drivers/powercap/intel_rapl_common.c > > index 0b2830efc574..bedd780bed12 100644 > > --- a/drivers/powercap/intel_rapl_common.c > > +++ b/drivers/powercap/intel_rapl_common.c > > @@ -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? 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. > > 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. > > 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. > > thanks, > rui > > 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), > > @@ -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); > > diff --git a/drivers/powercap/intel_rapl_msr.c > > b/drivers/powercap/intel_rapl_msr.c > > index a819b3b89b2f..78213d4b5b16 100644 > > --- a/drivers/powercap/intel_rapl_msr.c > > +++ b/drivers/powercap/intel_rapl_msr.c > > @@ -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; > > > Best regards, Victor Ding ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support 2020-11-03 6:10 ` Victor Ding @ 2020-11-03 17:09 ` Srinivas Pandruvada 2020-11-04 1:43 ` Victor Ding 0 siblings, 1 reply; 14+ messages in thread From: Srinivas Pandruvada @ 2020-11-03 17:09 UTC (permalink / raw) To: Victor Ding, Zhang Rui Cc: LKML, Daniel Lezcano, Kim Phillips, linux-pm, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Rafael J. Wysocki, Sean Christopherson, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote: > On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui <rui.zhang@intel.com> > wrote: > > On Tue, 2020-10-27 at 07:23 +0000, Victor Ding wrote: > > > 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 Is this for just energy reporting? No capping of power? Thanks, Srinivas > > > > > > Signed-off-by: Victor Ding <victording@google.com> > > > Acked-by: Kim Phillips <kim.phillips@amd.com> > > > > > > > > > --- > > > > > > Changes in v3: > > > By Victor Ding <victording@google.com> > > > - 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 <kim.phillips@amd.com>: > > > - 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(-) > > > > > > diff --git a/arch/x86/include/asm/msr-index.h > > > b/arch/x86/include/asm/msr-index.h > > > index 21917e134ad4..c36a083c8ec0 100644 > > > --- a/arch/x86/include/asm/msr-index.h > > > +++ b/arch/x86/include/asm/msr-index.h > > > @@ -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 */ > > > diff --git a/drivers/powercap/intel_rapl_common.c > > > b/drivers/powercap/intel_rapl_common.c > > > index 0b2830efc574..bedd780bed12 100644 > > > --- a/drivers/powercap/intel_rapl_common.c > > > +++ b/drivers/powercap/intel_rapl_common.c > > > @@ -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? > 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. > > 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. > > 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. > > thanks, > > rui > > > 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), > > > @@ -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), > > > + > > > + X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd), > > > {} > > > }; > > > MODULE_DEVICE_TABLE(x86cpu, rapl_ids); > > > diff --git a/drivers/powercap/intel_rapl_msr.c > > > b/drivers/powercap/intel_rapl_msr.c > > > index a819b3b89b2f..78213d4b5b16 100644 > > > --- a/drivers/powercap/intel_rapl_msr.c > > > +++ b/drivers/powercap/intel_rapl_msr.c > > > @@ -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; > > > > Best regards, > Victor Ding ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support 2020-11-03 17:09 ` Srinivas Pandruvada @ 2020-11-04 1:43 ` Victor Ding 2020-11-04 2:16 ` Srinivas Pandruvada 0 siblings, 1 reply; 14+ messages in thread From: Victor Ding @ 2020-11-04 1:43 UTC (permalink / raw) To: Srinivas Pandruvada Cc: Zhang Rui, LKML, Daniel Lezcano, Kim Phillips, linux-pm, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Rafael J. Wysocki, Sean Christopherson, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 On Wed, Nov 4, 2020 at 4:09 AM Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote: > > On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui <rui.zhang@intel.com> > > wrote: > > > On Tue, 2020-10-27 at 07:23 +0000, Victor Ding wrote: > > > > 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 > > Is this for just energy reporting? No capping of power? Correct, the hardware does not support capping of power. > > Thanks, > Srinivas > > > > > > > > > > Signed-off-by: Victor Ding <victording@google.com> > > > > Acked-by: Kim Phillips <kim.phillips@amd.com> > > > > > > > > > > > > --- > > > > > > > > Changes in v3: > > > > By Victor Ding <victording@google.com> > > > > - 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 <kim.phillips@amd.com>: > > > > - 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(-) > > > > > > > > diff --git a/arch/x86/include/asm/msr-index.h > > > > b/arch/x86/include/asm/msr-index.h > > > > index 21917e134ad4..c36a083c8ec0 100644 > > > > --- a/arch/x86/include/asm/msr-index.h > > > > +++ b/arch/x86/include/asm/msr-index.h > > > > @@ -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 */ > > > > diff --git a/drivers/powercap/intel_rapl_common.c > > > > b/drivers/powercap/intel_rapl_common.c > > > > index 0b2830efc574..bedd780bed12 100644 > > > > --- a/drivers/powercap/intel_rapl_common.c > > > > +++ b/drivers/powercap/intel_rapl_common.c > > > > @@ -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? > > 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. > > > 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. > > > 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. > > > thanks, > > > rui > > > > 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), > > > > @@ -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), > > > > + > > > > + X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd), > > > > {} > > > > }; > > > > MODULE_DEVICE_TABLE(x86cpu, rapl_ids); > > > > diff --git a/drivers/powercap/intel_rapl_msr.c > > > > b/drivers/powercap/intel_rapl_msr.c > > > > index a819b3b89b2f..78213d4b5b16 100644 > > > > --- a/drivers/powercap/intel_rapl_msr.c > > > > +++ b/drivers/powercap/intel_rapl_msr.c > > > > @@ -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; > > > > > > Best regards, > > Victor Ding > Best regards, Victor Ding ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support 2020-11-04 1:43 ` Victor Ding @ 2020-11-04 2:16 ` Srinivas Pandruvada 2020-11-05 3:53 ` Victor Ding 0 siblings, 1 reply; 14+ messages in thread From: Srinivas Pandruvada @ 2020-11-04 2:16 UTC (permalink / raw) To: Victor Ding Cc: Zhang Rui, LKML, Daniel Lezcano, Kim Phillips, linux-pm, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Rafael J. Wysocki, Sean Christopherson, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 On Wed, 2020-11-04 at 12:43 +1100, Victor Ding wrote: > On Wed, Nov 4, 2020 at 4:09 AM Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote: > > > On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui <rui.zhang@intel.com> > > > wrote: > > > > On Tue, 2020-10-27 at 07:23 +0000, Victor Ding wrote: > > > > > 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 > > > > 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 > > Thanks, > > Srinivas > > > > > > > > > Signed-off-by: Victor Ding <victording@google.com> > > > > > Acked-by: Kim Phillips <kim.phillips@amd.com> > > > > > > > > > > > > > > > --- > > > > > > > > > > Changes in v3: > > > > > By Victor Ding <victording@google.com> > > > > > - 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 <kim.phillips@amd.com>: > > > > > - 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(-) > > > > > > > > > > diff --git a/arch/x86/include/asm/msr-index.h > > > > > b/arch/x86/include/asm/msr-index.h > > > > > index 21917e134ad4..c36a083c8ec0 100644 > > > > > --- a/arch/x86/include/asm/msr-index.h > > > > > +++ b/arch/x86/include/asm/msr-index.h > > > > > @@ -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 */ > > > > > diff --git a/drivers/powercap/intel_rapl_common.c > > > > > b/drivers/powercap/intel_rapl_common.c > > > > > index 0b2830efc574..bedd780bed12 100644 > > > > > --- a/drivers/powercap/intel_rapl_common.c > > > > > +++ b/drivers/powercap/intel_rapl_common.c > > > > > @@ -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? > > > 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. > > > > 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. > > > > 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. > > > > thanks, > > > > rui > > > > > 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), > > > > > @@ -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), > > > > > + > > > > > + X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd), > > > > > {} > > > > > }; > > > > > MODULE_DEVICE_TABLE(x86cpu, rapl_ids); > > > > > diff --git a/drivers/powercap/intel_rapl_msr.c > > > > > b/drivers/powercap/intel_rapl_msr.c > > > > > index a819b3b89b2f..78213d4b5b16 100644 > > > > > --- a/drivers/powercap/intel_rapl_msr.c > > > > > +++ b/drivers/powercap/intel_rapl_msr.c > > > > > @@ -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; > > > > > > > > Best regards, > > > Victor Ding > Best regards, > Victor Ding ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support 2020-11-04 2:16 ` Srinivas Pandruvada @ 2020-11-05 3:53 ` Victor Ding 2020-11-05 17:14 ` Srinivas Pandruvada 0 siblings, 1 reply; 14+ messages in thread From: Victor Ding @ 2020-11-05 3:53 UTC (permalink / raw) To: Srinivas Pandruvada Cc: Zhang Rui, LKML, Daniel Lezcano, Kim Phillips, linux-pm, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Rafael J. Wysocki, Sean Christopherson, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 On Wed, Nov 4, 2020 at 1:17 PM Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > > On Wed, 2020-11-04 at 12:43 +1100, Victor Ding wrote: > > On Wed, Nov 4, 2020 at 4:09 AM Srinivas Pandruvada > > <srinivas.pandruvada@linux.intel.com> wrote: > > > On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote: > > > > On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui <rui.zhang@intel.com> > > > > wrote: > > > > > On Tue, 2020-10-27 at 07:23 +0000, Victor Ding wrote: > > > > > > 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 > > > > > > 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. > > 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. > > Thanks, > Srinivas > > > > Thanks, > > > Srinivas > > > > > > > > > > > > Signed-off-by: Victor Ding <victording@google.com> > > > > > > Acked-by: Kim Phillips <kim.phillips@amd.com> > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > Changes in v3: > > > > > > By Victor Ding <victording@google.com> > > > > > > - 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 <kim.phillips@amd.com>: > > > > > > - 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(-) > > > > > > > > > > > > diff --git a/arch/x86/include/asm/msr-index.h > > > > > > b/arch/x86/include/asm/msr-index.h > > > > > > index 21917e134ad4..c36a083c8ec0 100644 > > > > > > --- a/arch/x86/include/asm/msr-index.h > > > > > > +++ b/arch/x86/include/asm/msr-index.h > > > > > > @@ -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 */ > > > > > > diff --git a/drivers/powercap/intel_rapl_common.c > > > > > > b/drivers/powercap/intel_rapl_common.c > > > > > > index 0b2830efc574..bedd780bed12 100644 > > > > > > --- a/drivers/powercap/intel_rapl_common.c > > > > > > +++ b/drivers/powercap/intel_rapl_common.c > > > > > > @@ -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? > > > > 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. > > > > > 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. > > > > > 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. > > > > > thanks, > > > > > rui > > > > > > 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), > > > > > > @@ -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), > > > > > > + > > > > > > + X86_MATCH_VENDOR_FAM(AMD, 0x17, &rapl_defaults_amd), > > > > > > {} > > > > > > }; > > > > > > MODULE_DEVICE_TABLE(x86cpu, rapl_ids); > > > > > > diff --git a/drivers/powercap/intel_rapl_msr.c > > > > > > b/drivers/powercap/intel_rapl_msr.c > > > > > > index a819b3b89b2f..78213d4b5b16 100644 > > > > > > --- a/drivers/powercap/intel_rapl_msr.c > > > > > > +++ b/drivers/powercap/intel_rapl_msr.c > > > > > > @@ -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; > > > > > > > > > > Best regards, > > > > Victor Ding > > Best regards, > > Victor Ding > Best regards, Victor Ding ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support 2020-11-05 3:53 ` Victor Ding @ 2020-11-05 17:14 ` Srinivas Pandruvada 2020-11-05 18:04 ` Rafael J. Wysocki 0 siblings, 1 reply; 14+ messages in thread From: Srinivas Pandruvada @ 2020-11-05 17:14 UTC (permalink / raw) To: Victor Ding Cc: Zhang Rui, LKML, Daniel Lezcano, Kim Phillips, linux-pm, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Rafael J. Wysocki, Sean Christopherson, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 On Thu, 2020-11-05 at 14:53 +1100, Victor Ding wrote: > On Wed, Nov 4, 2020 at 1:17 PM Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> wrote: > > On Wed, 2020-11-04 at 12:43 +1100, Victor Ding wrote: > > > On Wed, Nov 4, 2020 at 4:09 AM Srinivas Pandruvada > > > <srinivas.pandruvada@linux.intel.com> wrote: > > > > On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote: > > > > > On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui < > > > > > rui.zhang@intel.com> > > > > > wrote: > > > > > > On Tue, 2020-10-27 at 07:23 +0000, Victor Ding wrote: > > > > > > > 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 > > > > > > > > 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. > > 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. Fine with me. I think eventually the power capping interface will be supported. Thanks, Srinivas > > Thanks, > > Srinivas > > > > > > Thanks, > > > > Srinivas > > > > > > > > > > > > > > > Signed-off-by: Victor Ding <victording@google.com> > > > > > > > Acked-by: Kim Phillips <kim.phillips@amd.com> > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > Changes in v3: > > > > > > > By Victor Ding <victording@google.com> > > > > > > > - 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 <kim.phillips@amd.com>: > > > > > > > - 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(-) > > > > > > > > > > > > > > diff --git a/arch/x86/include/asm/msr-index.h > > > > > > > b/arch/x86/include/asm/msr-index.h > > > > > > > index 21917e134ad4..c36a083c8ec0 100644 > > > > > > > --- a/arch/x86/include/asm/msr-index.h > > > > > > > +++ b/arch/x86/include/asm/msr-index.h > > > > > > > @@ -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 */ > > > > > > > diff --git a/drivers/powercap/intel_rapl_common.c > > > > > > > b/drivers/powercap/intel_rapl_common.c > > > > > > > index 0b2830efc574..bedd780bed12 100644 > > > > > > > --- a/drivers/powercap/intel_rapl_common.c > > > > > > > +++ b/drivers/powercap/intel_rapl_common.c > > > > > > > @@ -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? > > > > > 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. > > > > > > 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. > > > > > > 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. > > > > > > thanks, > > > > > > rui > > > > > > > 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), > > > > > > > @@ -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), > > > > > > > + > > > > > > > + X86_MATCH_VENDOR_FAM(AMD, 0x17, > > > > > > > &rapl_defaults_amd), > > > > > > > {} > > > > > > > }; > > > > > > > MODULE_DEVICE_TABLE(x86cpu, rapl_ids); > > > > > > > diff --git a/drivers/powercap/intel_rapl_msr.c > > > > > > > b/drivers/powercap/intel_rapl_msr.c > > > > > > > index a819b3b89b2f..78213d4b5b16 100644 > > > > > > > --- a/drivers/powercap/intel_rapl_msr.c > > > > > > > +++ b/drivers/powercap/intel_rapl_msr.c > > > > > > > @@ -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; > > > > > > > > > > > > Best regards, > > > > > Victor Ding > > > Best regards, > > > Victor Ding > Best regards, > Victor Ding ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support 2020-11-05 17:14 ` Srinivas Pandruvada @ 2020-11-05 18:04 ` Rafael J. Wysocki 0 siblings, 0 replies; 14+ messages in thread From: Rafael J. Wysocki @ 2020-11-05 18:04 UTC (permalink / raw) To: Srinivas Pandruvada, Victor Ding Cc: Zhang Rui, LKML, Daniel Lezcano, Kim Phillips, linux-pm, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Sean Christopherson, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, x86 On Thursday, November 5, 2020 6:14:01 PM CET Srinivas Pandruvada wrote: > On Thu, 2020-11-05 at 14:53 +1100, Victor Ding wrote: > > On Wed, Nov 4, 2020 at 1:17 PM Srinivas Pandruvada > > <srinivas.pandruvada@linux.intel.com> wrote: > > > On Wed, 2020-11-04 at 12:43 +1100, Victor Ding wrote: > > > > On Wed, Nov 4, 2020 at 4:09 AM Srinivas Pandruvada > > > > <srinivas.pandruvada@linux.intel.com> wrote: > > > > > On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote: > > > > > > On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui < > > > > > > rui.zhang@intel.com> > > > > > > wrote: > > > > > > > On Tue, 2020-10-27 at 07:23 +0000, Victor Ding wrote: > > > > > > > > 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 > > > > > > > > > > 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. > > > 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. > > Fine with me. OK, I'll queue up the series for 5.11 then if there are no other concerns. Thanks! ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 4/4] powercap: Add AMD Fam19h RAPL support 2020-10-27 7:23 [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h Victor Ding ` (2 preceding siblings ...) 2020-10-27 7:23 ` [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support Victor Ding @ 2020-10-27 7:23 ` Victor Ding 2020-11-10 19:24 ` [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h Rafael J. Wysocki 4 siblings, 0 replies; 14+ messages in thread From: Victor Ding @ 2020-10-27 7:23 UTC (permalink / raw) To: linux-kernel Cc: Daniel Lezcano, Kim Phillips, Zhang Rui, linux-pm, Victor Ding, Rafael J. Wysocki From: Kim Phillips <kim.phillips@amd.com> 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> Signed-off-by: Victor Ding <victording@google.com> --- Changes in v3: By Victor Ding <victording@google.com> - 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. By Victor Ding <victording@google.com> - Changed it to use rapl_defaults_amd. Changes in v2: By Kim Phillips <kim.phillips@amd.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) By Kim Phillips <kim.phillips@amd.com>: - Added Daniel Lezcano to Cc. - (No code changes). 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 bedd780bed12..264872f7f46d 100644 --- a/drivers/powercap/intel_rapl_common.c +++ b/drivers/powercap/intel_rapl_common.c @@ -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); -- 2.29.0.rc2.309.g374f81d7ae-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h 2020-10-27 7:23 [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h Victor Ding ` (3 preceding siblings ...) 2020-10-27 7:23 ` [PATCH v3 4/4] powercap: Add AMD Fam19h " Victor Ding @ 2020-11-10 19:24 ` Rafael J. Wysocki 4 siblings, 0 replies; 14+ messages in thread From: Rafael J. Wysocki @ 2020-11-10 19:24 UTC (permalink / raw) To: Victor Ding Cc: Linux Kernel Mailing List, Daniel Lezcano, Kim Phillips, Zhang Rui, Linux PM, Borislav Petkov, H. Peter Anvin, Ingo Molnar, Joerg Roedel, Kan Liang, Pawan Gupta, Peter Zijlstra (Intel), Rafael J. Wysocki, Sean Christopherson, Srinivas Pandruvada, Thomas Gleixner, Tony Luck, Vineela Tummalapalli, the arch/x86 maintainers On Tue, Oct 27, 2020 at 8:24 AM Victor Ding <victording@google.com> wrote: > > 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 <victording@google.com> > - 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 <kim.phillips@amd.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) > > 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(-) > > -- All patches applied as 5.11 material (which some minor edits in the changelogs), thanks! ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-11-10 19:24 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-27 7:23 [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h Victor Ding 2020-10-27 7:23 ` [PATCH v3 1/4] x86/msr-index: sort AMD RAPL MSRs by address Victor Ding 2020-10-27 7:23 ` [PATCH v3 2/4] powercap/intel_rapl_msr: Convert rapl_msr_priv into pointer Victor Ding 2020-10-27 7:23 ` [PATCH v3 3/4] powercap: Add AMD Fam17h RAPL support Victor Ding 2020-11-02 1:38 ` Zhang Rui 2020-11-03 6:10 ` Victor Ding 2020-11-03 17:09 ` Srinivas Pandruvada 2020-11-04 1:43 ` Victor Ding 2020-11-04 2:16 ` Srinivas Pandruvada 2020-11-05 3:53 ` Victor Ding 2020-11-05 17:14 ` Srinivas Pandruvada 2020-11-05 18:04 ` Rafael J. Wysocki 2020-10-27 7:23 ` [PATCH v3 4/4] powercap: Add AMD Fam19h " Victor Ding 2020-11-10 19:24 ` [PATCH v3 0/4] powercap: Enable RAPL for AMD Fam17h and Fam19h Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).