On Tue, 2020-11-03 at 17:10 +1100, Victor Ding wrote:

On Mon, Nov 2, 2020 at 12:39 PM Zhang Rui [off-list ref]
wrote:

quoted

On Tue, 2020-10-27 at 07:23 +0000, Victor Ding wrote:

quoted

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

quoted

quoted

Signed-off-by: Victor Ding <redacted>
Acked-by: Kim Phillips <redacted>


---

Changes in v3:
By Victor Ding [off-list ref]
 - 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 [off-list ref]:
 - 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.

quoted

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

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

quoted

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

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

quoted

thanks,
rui

quoted

 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