All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/4] Add processor to the ignore PSD override list
@ 2020-11-25 14:48 Punit Agrawal
  2020-11-25 14:48 ` [RFC PATCH 1/4] cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD Punit Agrawal
                   ` (4 more replies)
  0 siblings, 5 replies; 27+ messages in thread
From: Punit Agrawal @ 2020-11-25 14:48 UTC (permalink / raw)
  To: rjw; +Cc: Punit Agrawal, wei.huang2, linux-kernel, linux-pm, bp, x86

Hi,

While looking into Giovanni's patches to enable frequency invariance
on AMD systems[0], I noticed an issue with initialising frequency
domain information on a recent AMD APU.

Patch 1 refactors the test to ignore firmware provided frequency
domain into a separate function.

Patch 2 adds said APU (Family: 0x17, Model: 0x60, Stepping: 0x01) to
the list of CPUs for which the PSD override is ignored. I am not quite
happy with having to special case a particular CPU but also couldn't
find any documentation to help identify the CPUs that don't need the
override.

Patch 3 and 4 are somewhat independent and a first step towards
improving the situation with regards to the use of raw identifiers for
AMD processors throughout the kernel.

All feedback welcome.

Thanks,
Punit

[0] https://lore.kernel.org/linux-acpi/20201112182614.10700-1-ggherdovich@suse.cz/

Punit Agrawal (4):
  cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD
  cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  x86/cpu: amd: Define processor families
  cpufreq: acpi-cpufreq: Use identifiers for AMD processor family

 arch/x86/include/asm/amd-family.h    | 18 ++++++++++++++++++
 arch/x86/include/asm/cpu_device_id.h |  2 ++
 drivers/cpufreq/acpi-cpufreq.c       | 24 +++++++++++++++++++++---
 3 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/include/asm/amd-family.h

-- 
2.29.2


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [RFC PATCH 1/4] cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD
  2020-11-25 14:48 [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
@ 2020-11-25 14:48 ` Punit Agrawal
  2020-12-07 19:29   ` Wei Huang
  2020-11-25 14:48 ` [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list Punit Agrawal
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 27+ messages in thread
From: Punit Agrawal @ 2020-11-25 14:48 UTC (permalink / raw)
  To: rjw; +Cc: Punit Agrawal, wei.huang2, linux-kernel, linux-pm, bp, x86

Re-factor the code to override the firmware provided frequency domain
information (via PSD) to localise the checks in one function.

No functional change intended.

Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
Cc: Wei Huang <wei.huang2@amd.com>
---
 drivers/cpufreq/acpi-cpufreq.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 1e4fbb002a31..b1e7df96d428 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -191,6 +191,20 @@ static int check_amd_hwpstate_cpu(unsigned int cpuid)
 	return cpu_has(cpu, X86_FEATURE_HW_PSTATE);
 }
 
+static int override_acpi_psd(unsigned int cpu_id)
+{
+	struct cpuinfo_x86 *c = &boot_cpu_data;
+
+	if (c->x86_vendor == X86_VENDOR_AMD) {
+		if (!check_amd_hwpstate_cpu(cpu_id))
+			return false;
+
+		return c->x86 < 0x19;
+	}
+
+	return false;
+}
+
 static unsigned extract_io(struct cpufreq_policy *policy, u32 value)
 {
 	struct acpi_cpufreq_data *data = policy->driver_data;
@@ -691,8 +705,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 		cpumask_copy(policy->cpus, topology_core_cpumask(cpu));
 	}
 
-	if (check_amd_hwpstate_cpu(cpu) && boot_cpu_data.x86 < 0x19 &&
-	    !acpi_pstate_strict) {
+	if (override_acpi_psd(cpu) && !acpi_pstate_strict) {
 		cpumask_clear(policy->cpus);
 		cpumask_set_cpu(cpu, policy->cpus);
 		cpumask_copy(data->freqdomain_cpus,
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-11-25 14:48 [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
  2020-11-25 14:48 ` [RFC PATCH 1/4] cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD Punit Agrawal
@ 2020-11-25 14:48 ` Punit Agrawal
  2020-12-07 20:20   ` Wei Huang
  2020-11-25 14:48 ` [RFC PATCH 3/4] x86/cpu: amd: Define processor families Punit Agrawal
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 27+ messages in thread
From: Punit Agrawal @ 2020-11-25 14:48 UTC (permalink / raw)
  To: rjw; +Cc: Punit Agrawal, wei.huang2, linux-kernel, linux-pm, bp, x86

Booting Linux on a Zen2 based processor (family: 0x17, model: 0x60,
stepping: 0x1) shows the following message in the logs -

    acpi_cpufreq: overriding BIOS provided _PSD data

Although commit 5368512abe08 ("acpi-cpufreq: Honor _PSD table setting
on new AMD CPUs") indicates that the override is not required for Zen3
onwards, it seems that domain information can be trusted even on
certain earlier systems. Update the check, to skip the override for
Zen2 processors known to work without the override.

Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
Cc: Wei Huang <wei.huang2@amd.com>
---
 drivers/cpufreq/acpi-cpufreq.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index b1e7df96d428..29f1cd93541e 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -198,8 +198,13 @@ static int override_acpi_psd(unsigned int cpu_id)
 	if (c->x86_vendor == X86_VENDOR_AMD) {
 		if (!check_amd_hwpstate_cpu(cpu_id))
 			return false;
-
-		return c->x86 < 0x19;
+		/*
+		 * CPU's before Zen3 (except some Zen2) need the
+		 * override.
+		 */
+		return (c->x86 < 0x19) &&
+			!(c->x86 == 0x17 && c->x86_model == 0x60 &&
+			  c->x86_stepping == 0x1);
 	}
 
 	return false;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [RFC PATCH 3/4] x86/cpu: amd: Define processor families
  2020-11-25 14:48 [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
  2020-11-25 14:48 ` [RFC PATCH 1/4] cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD Punit Agrawal
  2020-11-25 14:48 ` [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list Punit Agrawal
@ 2020-11-25 14:48 ` Punit Agrawal
  2020-11-30 14:00   ` Borislav Petkov
  2020-11-25 14:48 ` [RFC PATCH 4/4] cpufreq: acpi-cpufreq: Use identifiers for AMD processor family Punit Agrawal
  2020-12-04 22:44 ` [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
  4 siblings, 1 reply; 27+ messages in thread
From: Punit Agrawal @ 2020-11-25 14:48 UTC (permalink / raw)
  To: rjw
  Cc: Punit Agrawal, wei.huang2, linux-kernel, linux-pm, bp, x86,
	Thomas Gleixner, Ingo Molnar

So far, the AMD processor identifier (family, models, stepping) are
referred to by raw values making it easy to make mistakes. It is also
harder to read and maintain. Additionally, these values are also being
used in subsystems outside the arch code where not everybody maybe be
as familiar with the processor identifiers.

As a first step towards improving the status quo, add macros for the
AMD processor families and propagate them through the existing
cpu_device_id.h header used for this purpose.

Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org
---
 arch/x86/include/asm/amd-family.h    | 18 ++++++++++++++++++
 arch/x86/include/asm/cpu_device_id.h |  2 ++
 2 files changed, 20 insertions(+)
 create mode 100644 arch/x86/include/asm/amd-family.h

diff --git a/arch/x86/include/asm/amd-family.h b/arch/x86/include/asm/amd-family.h
new file mode 100644
index 000000000000..dff4d13b8e74
--- /dev/null
+++ b/arch/x86/include/asm/amd-family.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_AMD_FAMILY_H
+#define _ASM_X86_AMD_FAMILY_H
+
+#define AMD_FAM_K5			0x04
+#define AMD_FAM_K6			0x05
+#define AMD_FAM_K7			0x06
+#define AMD_FAM_K8			0x0F
+#define AMD_FAM_K10			0x10
+#define AMD_FAM_K8_K10_HYBRID		0x11
+#define AMD_FAM_LLANO			0x12
+#define AMD_FAM_BOBCAT			0x14
+#define AMD_FAM_BULLDOZER		0x15
+#define AMD_FAM_JAGUAR			0x16
+#define AMD_FAM_ZEN			0x17
+#define AMD_FAM_ZEN3			0x19
+
+#endif /* _ASM_X86_AMD_FAMILY_H */
diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
index eb8fcede9e3b..bbb48ba4c7ff 100644
--- a/arch/x86/include/asm/cpu_device_id.h
+++ b/arch/x86/include/asm/cpu_device_id.h
@@ -12,6 +12,8 @@
 #include <linux/mod_devicetable.h>
 /* Get the INTEL_FAM* model defines */
 #include <asm/intel-family.h>
+/* Get the AMD model defines */
+#include <asm/amd-family.h>
 /* And the X86_VENDOR_* ones */
 #include <asm/processor.h>
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [RFC PATCH 4/4] cpufreq: acpi-cpufreq: Use identifiers for AMD processor family
  2020-11-25 14:48 [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
                   ` (2 preceding siblings ...)
  2020-11-25 14:48 ` [RFC PATCH 3/4] x86/cpu: amd: Define processor families Punit Agrawal
@ 2020-11-25 14:48 ` Punit Agrawal
  2020-11-30 14:02   ` Borislav Petkov
  2020-12-04 22:44 ` [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
  4 siblings, 1 reply; 27+ messages in thread
From: Punit Agrawal @ 2020-11-25 14:48 UTC (permalink / raw)
  To: rjw; +Cc: Punit Agrawal, wei.huang2, linux-kernel, linux-pm, bp, x86

Replace the raw values for AMD processor family with recently
introduced identifier macros to improve code readability and
maintainability.

Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
---
 drivers/cpufreq/acpi-cpufreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 29f1cd93541e..d8b8300ae9e0 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -202,8 +202,8 @@ static int override_acpi_psd(unsigned int cpu_id)
 		 * CPU's before Zen3 (except some Zen2) need the
 		 * override.
 		 */
-		return (c->x86 < 0x19) &&
-			!(c->x86 == 0x17 && c->x86_model == 0x60 &&
+		return (c->x86 < AMD_FAM_ZEN3) &&
+			!(c->x86 == AMD_FAM_ZEN && c->x86_model == 0x60 &&
 			  c->x86_stepping == 0x1);
 	}
 
@@ -735,7 +735,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	switch (perf->control_register.space_id) {
 	case ACPI_ADR_SPACE_SYSTEM_IO:
 		if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD &&
-		    boot_cpu_data.x86 == 0xf) {
+		    boot_cpu_data.x86 == AMD_FAM_K8) {
 			pr_debug("AMD K8 systems must use native drivers.\n");
 			result = -ENODEV;
 			goto err_unreg;
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 3/4] x86/cpu: amd: Define processor families
  2020-11-25 14:48 ` [RFC PATCH 3/4] x86/cpu: amd: Define processor families Punit Agrawal
@ 2020-11-30 14:00   ` Borislav Petkov
  2020-12-02 14:13     ` Punit Agrawal
  0 siblings, 1 reply; 27+ messages in thread
From: Borislav Petkov @ 2020-11-30 14:00 UTC (permalink / raw)
  To: Punit Agrawal
  Cc: rjw, wei.huang2, linux-kernel, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Tom Lendacky, Yazen Ghannam

On Wed, Nov 25, 2020 at 11:48:46PM +0900, Punit Agrawal wrote:
> So far, the AMD processor identifier (family, models, stepping) are
> referred to by raw values making it easy to make mistakes. It is also
> harder to read and maintain. Additionally, these values are also being
> used in subsystems outside the arch code where not everybody maybe be
> as familiar with the processor identifiers.
> 
> As a first step towards improving the status quo, add macros for the
> AMD processor families and propagate them through the existing
> cpu_device_id.h header used for this purpose.
> 
> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: x86@kernel.org
> ---
>  arch/x86/include/asm/amd-family.h    | 18 ++++++++++++++++++
>  arch/x86/include/asm/cpu_device_id.h |  2 ++
>  2 files changed, 20 insertions(+)
>  create mode 100644 arch/x86/include/asm/amd-family.h
> 
> diff --git a/arch/x86/include/asm/amd-family.h b/arch/x86/include/asm/amd-family.h
> new file mode 100644
> index 000000000000..dff4d13b8e74
> --- /dev/null
> +++ b/arch/x86/include/asm/amd-family.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_AMD_FAMILY_H
> +#define _ASM_X86_AMD_FAMILY_H
> +
> +#define AMD_FAM_K5			0x04
> +#define AMD_FAM_K6			0x05
> +#define AMD_FAM_K7			0x06
> +#define AMD_FAM_K8			0x0F
> +#define AMD_FAM_K10			0x10

Fam 0x10 is Greyhound and a lot more core names. I'd let the AMD folks
on Cc say what they wanna call it but I don't think K10 was used
anywhere except outside of AMD. :)

> +#define AMD_FAM_K8_K10_HYBRID		0x11

That was called Griffin so AMD_FAM_GRIFFIN I guess. If not used anywhere
in the tree, no need to add the define. Same holds true for the rest of
the defines - add them only when they're used.

> +#define AMD_FAM_LLANO			0x12
> +#define AMD_FAM_BOBCAT			0x14
> +#define AMD_FAM_BULLDOZER		0x15
> +#define AMD_FAM_JAGUAR			0x16
> +#define AMD_FAM_ZEN			0x17

ZEN2 is also 0x17 but different models so this is where the family
matching scheme doesn't work - you'd need the models too.

0x18 is HYGON

> +#define AMD_FAM_ZEN3			0x19
> +
> +#endif /* _ASM_X86_AMD_FAMILY_H */
> diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
> index eb8fcede9e3b..bbb48ba4c7ff 100644
> --- a/arch/x86/include/asm/cpu_device_id.h
> +++ b/arch/x86/include/asm/cpu_device_id.h
> @@ -12,6 +12,8 @@
>  #include <linux/mod_devicetable.h>
>  /* Get the INTEL_FAM* model defines */
>  #include <asm/intel-family.h>
> +/* Get the AMD model defines */
> +#include <asm/amd-family.h>
>  /* And the X86_VENDOR_* ones */
>  #include <asm/processor.h>
>  
> -- 
> 2.29.2
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 4/4] cpufreq: acpi-cpufreq: Use identifiers for AMD processor family
  2020-11-25 14:48 ` [RFC PATCH 4/4] cpufreq: acpi-cpufreq: Use identifiers for AMD processor family Punit Agrawal
@ 2020-11-30 14:02   ` Borislav Petkov
  2020-12-02 14:30     ` Punit Agrawal
  0 siblings, 1 reply; 27+ messages in thread
From: Borislav Petkov @ 2020-11-30 14:02 UTC (permalink / raw)
  To: Punit Agrawal
  Cc: rjw, wei.huang2, linux-kernel, linux-pm, x86, Tom Lendacky,
	Yazen Ghannam

On Wed, Nov 25, 2020 at 11:48:47PM +0900, Punit Agrawal wrote:
> Replace the raw values for AMD processor family with recently
> introduced identifier macros to improve code readability and
> maintainability.
> 
> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 29f1cd93541e..d8b8300ae9e0 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -202,8 +202,8 @@ static int override_acpi_psd(unsigned int cpu_id)
>  		 * CPU's before Zen3 (except some Zen2) need the
>  		 * override.
>  		 */
> -		return (c->x86 < 0x19) &&
> -			!(c->x86 == 0x17 && c->x86_model == 0x60 &&
> +		return (c->x86 < AMD_FAM_ZEN3) &&
> +			!(c->x86 == AMD_FAM_ZEN && c->x86_model == 0x60 &&

This is what I mean - that's Zen2 as the comment above says so having

		c->x86 == AMD_FAM_ZEN

is not enough. And you have a comment above it stating which CPUs are
matched here so I'm not sure those family defines make it any better...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 3/4] x86/cpu: amd: Define processor families
  2020-11-30 14:00   ` Borislav Petkov
@ 2020-12-02 14:13     ` Punit Agrawal
  2020-12-02 16:57       ` Borislav Petkov
  0 siblings, 1 reply; 27+ messages in thread
From: Punit Agrawal @ 2020-12-02 14:13 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: rjw, wei.huang2, linux-kernel, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Tom Lendacky, Yazen Ghannam

Hi Boris,

Borislav Petkov <bp@alien8.de> writes:

> On Wed, Nov 25, 2020 at 11:48:46PM +0900, Punit Agrawal wrote:
>> So far, the AMD processor identifier (family, models, stepping) are
>> referred to by raw values making it easy to make mistakes. It is also
>> harder to read and maintain. Additionally, these values are also being
>> used in subsystems outside the arch code where not everybody maybe be
>> as familiar with the processor identifiers.
>> 
>> As a first step towards improving the status quo, add macros for the
>> AMD processor families and propagate them through the existing
>> cpu_device_id.h header used for this purpose.
>> 
>> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: x86@kernel.org
>> ---
>>  arch/x86/include/asm/amd-family.h    | 18 ++++++++++++++++++
>>  arch/x86/include/asm/cpu_device_id.h |  2 ++
>>  2 files changed, 20 insertions(+)
>>  create mode 100644 arch/x86/include/asm/amd-family.h
>> 
>> diff --git a/arch/x86/include/asm/amd-family.h b/arch/x86/include/asm/amd-family.h
>> new file mode 100644
>> index 000000000000..dff4d13b8e74
>> --- /dev/null
>> +++ b/arch/x86/include/asm/amd-family.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_X86_AMD_FAMILY_H
>> +#define _ASM_X86_AMD_FAMILY_H
>> +
>> +#define AMD_FAM_K5			0x04
>> +#define AMD_FAM_K6			0x05
>> +#define AMD_FAM_K7			0x06
>> +#define AMD_FAM_K8			0x0F
>> +#define AMD_FAM_K10			0x10
>
> Fam 0x10 is Greyhound and a lot more core names. I'd let the AMD folks
> on Cc say what they wanna call it but I don't think K10 was used
> anywhere except outside of AMD. :)

Didn't realize the core was internal only. There are a couple of
instances where the family does shows up arch/x86/kernel/cpu/amd.c so
atleast some of the patches did make it upstream.

>> +#define AMD_FAM_K8_K10_HYBRID		0x11
>
> That was called Griffin so AMD_FAM_GRIFFIN I guess. If not used anywhere
> in the tree, no need to add the define.

I haven't looked to deeply but there's one instance in
arch/x86/kernel/cpu/amd.c - though I wonder if that could be re-written
to rely on a hardware / firmware interface instead.

> Same holds true for the rest of the defines - add them only when
> they're used.

Makes sense - I will follow your suggestion in the next version.

>> +#define AMD_FAM_LLANO			0x12
>> +#define AMD_FAM_BOBCAT			0x14
>> +#define AMD_FAM_BULLDOZER		0x15
>> +#define AMD_FAM_JAGUAR			0x16
>> +#define AMD_FAM_ZEN			0x17
>
> ZEN2 is also 0x17 but different models so this is where the family
> matching scheme doesn't work - you'd need the models too.

Yes, I wasn't sure the best way to handle this so went with the earlier
generation name. I think for such cases, something that looks at the
cpuinfo_x86 structure and decides the family / generation is probably
the way to go.

> 0x18 is HYGON

I missed this one. I'll add it to the list.

Thanks for the review and your comments. I'll wait a bit to see if
there's any further feedback.

Cheers,
Punit

[...]


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 4/4] cpufreq: acpi-cpufreq: Use identifiers for AMD processor family
  2020-11-30 14:02   ` Borislav Petkov
@ 2020-12-02 14:30     ` Punit Agrawal
  0 siblings, 0 replies; 27+ messages in thread
From: Punit Agrawal @ 2020-12-02 14:30 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: rjw, wei.huang2, linux-kernel, linux-pm, x86, Tom Lendacky,
	Yazen Ghannam

Borislav Petkov <bp@alien8.de> writes:

> On Wed, Nov 25, 2020 at 11:48:47PM +0900, Punit Agrawal wrote:
>> Replace the raw values for AMD processor family with recently
>> introduced identifier macros to improve code readability and
>> maintainability.
>> 
>> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
>> ---
>>  drivers/cpufreq/acpi-cpufreq.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index 29f1cd93541e..d8b8300ae9e0 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -202,8 +202,8 @@ static int override_acpi_psd(unsigned int cpu_id)
>>  		 * CPU's before Zen3 (except some Zen2) need the
>>  		 * override.
>>  		 */
>> -		return (c->x86 < 0x19) &&
>> -			!(c->x86 == 0x17 && c->x86_model == 0x60 &&
>> +		return (c->x86 < AMD_FAM_ZEN3) &&
>> +			!(c->x86 == AMD_FAM_ZEN && c->x86_model == 0x60 &&
>
> This is what I mean - that's Zen2 as the comment above says so having
>
> 		c->x86 == AMD_FAM_ZEN
>
> is not enough. And you have a comment above it stating which CPUs are
> matched here so I'm not sure those family defines make it any better...

Hmm.. for this series, it probably doesn't add much value - especially
with the comment and macro mismatch.

The last two patches were posted to check if there's wider interest in
the changes. If the macro conversion is useful, I can split the patches
from this series into a separate set with more sites being updated. I'll
wait to see if there's any further feedback.

Thanks,
Punit


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 3/4] x86/cpu: amd: Define processor families
  2020-12-02 14:13     ` Punit Agrawal
@ 2020-12-02 16:57       ` Borislav Petkov
  0 siblings, 0 replies; 27+ messages in thread
From: Borislav Petkov @ 2020-12-02 16:57 UTC (permalink / raw)
  To: Punit Agrawal
  Cc: rjw, wei.huang2, linux-kernel, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Tom Lendacky, Yazen Ghannam

On Wed, Dec 02, 2020 at 11:13:02PM +0900, Punit Agrawal wrote:
> Didn't realize the core was internal only.

F10h is not internal only - all I'm saying is that "K10" wasn't use
inside AMD AFAIR.

> Makes sense - I will follow your suggestion in the next version.

Well, I don't think it is worth the churn, TBH.

I'd prefer comments over the f/m/s checks which explain what is matched
much better than defines for family numbers which are inadequate when
one needs to match the model too, for one.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 0/4] Add processor to the ignore PSD override list
  2020-11-25 14:48 [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
                   ` (3 preceding siblings ...)
  2020-11-25 14:48 ` [RFC PATCH 4/4] cpufreq: acpi-cpufreq: Use identifiers for AMD processor family Punit Agrawal
@ 2020-12-04 22:44 ` Punit Agrawal
  2020-12-07 13:55   ` Rafael J. Wysocki
  4 siblings, 1 reply; 27+ messages in thread
From: Punit Agrawal @ 2020-12-04 22:44 UTC (permalink / raw)
  To: rjw; +Cc: wei.huang2, linux-kernel, linux-pm, bp, x86

Hi Rafael,

Punit Agrawal <punitagrawal@gmail.com> writes:

> Hi,
>
> While looking into Giovanni's patches to enable frequency invariance
> on AMD systems[0], I noticed an issue with initialising frequency
> domain information on a recent AMD APU.
>
> Patch 1 refactors the test to ignore firmware provided frequency
> domain into a separate function.
>
> Patch 2 adds said APU (Family: 0x17, Model: 0x60, Stepping: 0x01) to
> the list of CPUs for which the PSD override is ignored. I am not quite
> happy with having to special case a particular CPU but also couldn't
> find any documentation to help identify the CPUs that don't need the
> override.

Are you be OK to pick the first two patches if there are no issues?

Thanks,
Punit


> Patch 3 and 4 are somewhat independent and a first step towards
> improving the situation with regards to the use of raw identifiers for
> AMD processors throughout the kernel.
>
> All feedback welcome.
>
> Thanks,
> Punit
>
> [0] https://lore.kernel.org/linux-acpi/20201112182614.10700-1-ggherdovich@suse.cz/
>
> Punit Agrawal (4):
>   cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD
>   cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
>   x86/cpu: amd: Define processor families
>   cpufreq: acpi-cpufreq: Use identifiers for AMD processor family
>
>  arch/x86/include/asm/amd-family.h    | 18 ++++++++++++++++++
>  arch/x86/include/asm/cpu_device_id.h |  2 ++
>  drivers/cpufreq/acpi-cpufreq.c       | 24 +++++++++++++++++++++---
>  3 files changed, 41 insertions(+), 3 deletions(-)
>  create mode 100644 arch/x86/include/asm/amd-family.h

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 0/4] Add processor to the ignore PSD override list
  2020-12-04 22:44 ` [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
@ 2020-12-07 13:55   ` Rafael J. Wysocki
  2020-12-08 23:25     ` Punit Agrawal
  0 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2020-12-07 13:55 UTC (permalink / raw)
  To: Punit Agrawal
  Cc: Rafael J. Wysocki, Wei Huang, Linux Kernel Mailing List,
	Linux PM, Borislav Petkov, the arch/x86 maintainers

On Fri, Dec 4, 2020 at 11:45 PM Punit Agrawal <punitagrawal@gmail.com> wrote:
>
> Hi Rafael,
>
> Punit Agrawal <punitagrawal@gmail.com> writes:
>
> > Hi,
> >
> > While looking into Giovanni's patches to enable frequency invariance
> > on AMD systems[0], I noticed an issue with initialising frequency
> > domain information on a recent AMD APU.
> >
> > Patch 1 refactors the test to ignore firmware provided frequency
> > domain into a separate function.
> >
> > Patch 2 adds said APU (Family: 0x17, Model: 0x60, Stepping: 0x01) to
> > the list of CPUs for which the PSD override is ignored. I am not quite
> > happy with having to special case a particular CPU but also couldn't
> > find any documentation to help identify the CPUs that don't need the
> > override.
>
> Are you be OK to pick the first two patches if there are no issues?

Please send them as non-RFC and change the name of override_acpi_psd()
to indicate that it is AMD-specific.

Thanks!

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 1/4] cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD
  2020-11-25 14:48 ` [RFC PATCH 1/4] cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD Punit Agrawal
@ 2020-12-07 19:29   ` Wei Huang
  2020-12-08 23:31     ` Punit Agrawal
  0 siblings, 1 reply; 27+ messages in thread
From: Wei Huang @ 2020-12-07 19:29 UTC (permalink / raw)
  To: Punit Agrawal, rjw; +Cc: wei.huang2, linux-kernel, linux-pm, bp, x86



On 11/25/20 8:48 AM, Punit Agrawal wrote:
> Re-factor the code to override the firmware provided frequency domain
> information (via PSD) to localise the checks in one function.
> 
> No functional change intended.
> 
> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> Cc: Wei Huang <wei.huang2@amd.com>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 1e4fbb002a31..b1e7df96d428 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -191,6 +191,20 @@ static int check_amd_hwpstate_cpu(unsigned int cpuid)
>  	return cpu_has(cpu, X86_FEATURE_HW_PSTATE);
>  }
>  
> +static int override_acpi_psd(unsigned int cpu_id)
         ^^^^^
int is fine, but it might be better to use bool. Otherwise I don't see
any issues with this patch.

> +{
> +	struct cpuinfo_x86 *c = &boot_cpu_data;
> +
> +	if (c->x86_vendor == X86_VENDOR_AMD) {
> +		if (!check_amd_hwpstate_cpu(cpu_id))
> +			return false;
> +
> +		return c->x86 < 0x19;
> +	}
> +
> +	return false;
> +}
> +
>  static unsigned extract_io(struct cpufreq_policy *policy, u32 value)
>  {
>  	struct acpi_cpufreq_data *data = policy->driver_data;
> @@ -691,8 +705,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  		cpumask_copy(policy->cpus, topology_core_cpumask(cpu));
>  	}
>  
> -	if (check_amd_hwpstate_cpu(cpu) && boot_cpu_data.x86 < 0x19 &&
> -	    !acpi_pstate_strict) {
> +	if (override_acpi_psd(cpu) && !acpi_pstate_strict) {
>  		cpumask_clear(policy->cpus);
>  		cpumask_set_cpu(cpu, policy->cpus);
>  		cpumask_copy(data->freqdomain_cpus,
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-11-25 14:48 ` [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list Punit Agrawal
@ 2020-12-07 20:20   ` Wei Huang
  2020-12-07 20:26     ` Borislav Petkov
  0 siblings, 1 reply; 27+ messages in thread
From: Wei Huang @ 2020-12-07 20:20 UTC (permalink / raw)
  To: Punit Agrawal, rjw; +Cc: wei.huang2, linux-kernel, linux-pm, bp, x86



On 11/25/20 8:48 AM, Punit Agrawal wrote:
> Booting Linux on a Zen2 based processor (family: 0x17, model: 0x60,
> stepping: 0x1) shows the following message in the logs -
> 
>     acpi_cpufreq: overriding BIOS provided _PSD data
> 
> Although commit 5368512abe08 ("acpi-cpufreq: Honor _PSD table setting
> on new AMD CPUs") indicates that the override is not required for Zen3
> onwards, it seems that domain information can be trusted even on

Given that the original quirk acd316248205 ("acpi-cpufreq: Add quirk to
disable _PSD usage on all AMD CPUs") was submitted 8 years ago, it is
not a surprise that some system firmware before family 19h might been
fixed. Unfortunately, like what Punit said, I didn't find any
documentation on the list of existing, fixed CPUs.

In my commit 5368512abe ("acpi-cpufreq: Honor _PSD table setting on new
AMD CPUs"), family 19h was picked because 1) we know BIOS will fix this
problem for this specific generation of CPUs, and 2) without this
commit, it _might_ cause issues on certain CPUs.

In summary, this patch is fine if Punit already verified it. My only
concern is the list can potentially increase over the time, and we will
keep coming back to fix override_acpi_psd() function.

> certain earlier systems. Update the check, to skip the override for
> Zen2 processors known to work without the override.
> 
> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
> Cc: Wei Huang <wei.huang2@amd.com>
> ---
>  drivers/cpufreq/acpi-cpufreq.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index b1e7df96d428..29f1cd93541e 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -198,8 +198,13 @@ static int override_acpi_psd(unsigned int cpu_id)
>  	if (c->x86_vendor == X86_VENDOR_AMD) {
>  		if (!check_amd_hwpstate_cpu(cpu_id))
>  			return false;
> -
> -		return c->x86 < 0x19;
> +		/*
> +		 * CPU's before Zen3 (except some Zen2) need the
> +		 * override.
> +		 */
> +		return (c->x86 < 0x19) &&
> +			!(c->x86 == 0x17 && c->x86_model == 0x60 &&
> +			  c->x86_stepping == 0x1);
>  	}
>  
>  	return false;
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-07 20:20   ` Wei Huang
@ 2020-12-07 20:26     ` Borislav Petkov
  2020-12-07 22:07       ` Wei Huang
  0 siblings, 1 reply; 27+ messages in thread
From: Borislav Petkov @ 2020-12-07 20:26 UTC (permalink / raw)
  To: Wei Huang; +Cc: Punit Agrawal, rjw, wei.huang2, linux-kernel, linux-pm, x86

On Mon, Dec 07, 2020 at 02:20:55PM -0600, Wei Huang wrote:
> In summary, this patch is fine if Punit already verified it. My only
> concern is the list can potentially increase over the time, and we will
> keep coming back to fix override_acpi_psd() function.

Can the detection be done by looking at those _PSD things instead of
comparing f/m/s?

And, alternatively, what is this fixing?

So what if some zen2 boxes have correct _PSD objects? Why do they need
to ignore the override?

Hmmm?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-07 20:26     ` Borislav Petkov
@ 2020-12-07 22:07       ` Wei Huang
  2020-12-07 22:30         ` Borislav Petkov
  0 siblings, 1 reply; 27+ messages in thread
From: Wei Huang @ 2020-12-07 22:07 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Punit Agrawal, rjw, wei.huang2, linux-kernel, linux-pm, x86



On 12/7/20 2:26 PM, Borislav Petkov wrote:
> On Mon, Dec 07, 2020 at 02:20:55PM -0600, Wei Huang wrote:
>> In summary, this patch is fine if Punit already verified it. My only
>> concern is the list can potentially increase over the time, and we will
>> keep coming back to fix override_acpi_psd() function.
> 
> Can the detection be done by looking at those _PSD things instead of
> comparing f/m/s?

Not I am aware of. I don't know the correlation between _PSD
configuration and CPU's f/m/s.

> 
> And, alternatively, what is this fixing?
> 
> So what if some zen2 boxes have correct _PSD objects? Why do they need
> to ignore the override?

I think we shouldn't override zen2 if _PSD is correct. In my opinion,
there are two approaches:

* Keep override_acpi_psd()
Let us keep the original quirk and override_acpi_psd() function. Over
the time, people may want to add new CPUs to override_acpi_psd(). The
maintainer may declare that only CPUs >= family 17h will be fixed, to
avoid exploding the check-list.

* Remove the quirk completely
We can completely remove commit acd316248205 ("acpi-cpufreq: Add quirk
to disable _PSD usage on all AMD CPUs")? I am not sure what "AMD desktop
boards" was referring to in the original commit message of acd316248205.
Maybe such machines aren't in use anymore.

> 
> Hmmm?
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-07 22:07       ` Wei Huang
@ 2020-12-07 22:30         ` Borislav Petkov
  2020-12-08  3:44           ` Wei Huang
  2020-12-08 23:21           ` Punit Agrawal
  0 siblings, 2 replies; 27+ messages in thread
From: Borislav Petkov @ 2020-12-07 22:30 UTC (permalink / raw)
  To: Wei Huang; +Cc: Punit Agrawal, rjw, wei.huang2, linux-kernel, linux-pm, x86

On Mon, Dec 07, 2020 at 04:07:52PM -0600, Wei Huang wrote:
> I think we shouldn't override zen2 if _PSD is correct. In my opinion,
> there are two approaches:
> 
> * Keep override_acpi_psd()
> Let us keep the original quirk and override_acpi_psd() function. Over
> the time, people may want to add new CPUs to override_acpi_psd(). The
> maintainer may declare that only CPUs >= family 17h will be fixed, to
> avoid exploding the check-list.
> 
> * Remove the quirk completely
> We can completely remove commit acd316248205 ("acpi-cpufreq: Add quirk
> to disable _PSD usage on all AMD CPUs")? I am not sure what "AMD desktop
> boards" was referring to in the original commit message of acd316248205.
> Maybe such machines aren't in use anymore.

* Third option: do not do anything. Why?

- Let sleeping dogs lie and leave the workaround acd316248205 for old
machines.

- Make a clear cut that the override is not needed from Zen3 on, i.e.,
your patch

   5368512abe08 ("acpi-cpufreq: Honor _PSD table setting on new AMD CPUs")


Punit's commit message reads "...indicates that the override is not
required for Zen3 onwards, it seems that domain information can be
trusted even on certain earlier systems."

That's not nearly a justification in my book to do this on anything < Zen3.

This way you have a clear cut, you don't need to deal with adding any
more models to override_acpi_psd() and all is good.

Unless there's a better reason to skip the override on machines < Zen3
but I haven't heard any so far...

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-07 22:30         ` Borislav Petkov
@ 2020-12-08  3:44           ` Wei Huang
  2020-12-08 23:21           ` Punit Agrawal
  1 sibling, 0 replies; 27+ messages in thread
From: Wei Huang @ 2020-12-08  3:44 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Punit Agrawal, rjw, wei.huang2, linux-kernel, linux-pm, x86



On 12/7/20 4:30 PM, Borislav Petkov wrote:
> On Mon, Dec 07, 2020 at 04:07:52PM -0600, Wei Huang wrote:
>> I think we shouldn't override zen2 if _PSD is correct. In my opinion,
>> there are two approaches:
>>
>> * Keep override_acpi_psd()
>> Let us keep the original quirk and override_acpi_psd() function. Over
>> the time, people may want to add new CPUs to override_acpi_psd(). The
>> maintainer may declare that only CPUs >= family 17h will be fixed, to
>> avoid exploding the check-list.
>>
>> * Remove the quirk completely
>> We can completely remove commit acd316248205 ("acpi-cpufreq: Add quirk
>> to disable _PSD usage on all AMD CPUs")? I am not sure what "AMD desktop
>> boards" was referring to in the original commit message of acd316248205.
>> Maybe such machines aren't in use anymore.
> 
> * Third option: do not do anything. Why?

I am fine with this option, unless Punit can prove me that I am wrong,
(i.e. some Zen2 is broken because of acd316248205).

> 
> - Let sleeping dogs lie and leave the workaround acd316248205 for old
> machines.
> 
> - Make a clear cut that the override is not needed from Zen3 on, i.e.,
> your patch
> 
>    5368512abe08 ("acpi-cpufreq: Honor _PSD table setting on new AMD CPUs")
> 
> 
> Punit's commit message reads "...indicates that the override is not
> required for Zen3 onwards, it seems that domain information can be
> trusted even on certain earlier systems."
> 
> That's not nearly a justification in my book to do this on anything < Zen3.
> 
> This way you have a clear cut, you don't need to deal with adding any
> more models to override_acpi_psd() and all is good.
> 
> Unless there's a better reason to skip the override on machines < Zen3
> but I haven't heard any so far...
> 
> Thx.
> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-07 22:30         ` Borislav Petkov
  2020-12-08  3:44           ` Wei Huang
@ 2020-12-08 23:21           ` Punit Agrawal
  2020-12-08 23:32             ` Borislav Petkov
  1 sibling, 1 reply; 27+ messages in thread
From: Punit Agrawal @ 2020-12-08 23:21 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Wei Huang, rjw, wei.huang2, linux-kernel, linux-pm, x86

Borislav Petkov <bp@alien8.de> writes:

> On Mon, Dec 07, 2020 at 04:07:52PM -0600, Wei Huang wrote:
>> I think we shouldn't override zen2 if _PSD is correct. In my opinion,
>> there are two approaches:
>> 
>> * Keep override_acpi_psd()
>> Let us keep the original quirk and override_acpi_psd() function. Over
>> the time, people may want to add new CPUs to override_acpi_psd(). The
>> maintainer may declare that only CPUs >= family 17h will be fixed, to
>> avoid exploding the check-list.
>> 
>> * Remove the quirk completely
>> We can completely remove commit acd316248205 ("acpi-cpufreq: Add quirk
>> to disable _PSD usage on all AMD CPUs")? I am not sure what "AMD desktop
>> boards" was referring to in the original commit message of acd316248205.
>> Maybe such machines aren't in use anymore.
>
> * Third option: do not do anything. Why?
>
> - Let sleeping dogs lie and leave the workaround acd316248205 for old
> machines.

According to the commit log, acd316248205 seems to be only targeted at
powernow-K8 -

    "in order to use the hardware to its full potential and keep the
    original powernow-k8 behavior, lets override the _PSD table setting on
    AMD hardware."

It's unfortunate that it has continued to apply to all systems since. An
alternate to this and 5368512abe08 would be to restrict the original
workaround to the system mentioned in the commit log.

> - Make a clear cut that the override is not needed from Zen3 on, i.e.,
> your patch
>
>    5368512abe08 ("acpi-cpufreq: Honor _PSD table setting on new AMD CPUs")
>
>
> Punit's commit message reads "...indicates that the override is not
> required for Zen3 onwards, it seems that domain information can be
> trusted even on certain earlier systems."
>
> That's not nearly a justification in my book to do this on anything <
> Zen3.
>
> This way you have a clear cut, you don't need to deal with adding any
> more models to override_acpi_psd() and all is good.

PSD (P-State Dependency) object is used to express the coupling of
processors that share the same frequency domain. Ignoring the hardware
topology information will cause both power management and scheduling to
make sub-optimal choices.

Wasn't this the motivation for taking it into account for Zen3 based
systems in 5368512abe08? Or the powernow-k8 in the original workaround
(though there it required ignoring firmware)? The exact same argument
applies here to the Zen2 system I have running in a thermally
constrained environment.

In an ideal case, I was hoping AMD folks would come back with
confirmation of how far back the override can be dropped and the
workaround condition could be relaxed further. But if that is not
available, the only way we have is to include systems that have been
verified to not need the override and somebody that cares to send the
patch.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 0/4] Add processor to the ignore PSD override list
  2020-12-07 13:55   ` Rafael J. Wysocki
@ 2020-12-08 23:25     ` Punit Agrawal
  0 siblings, 0 replies; 27+ messages in thread
From: Punit Agrawal @ 2020-12-08 23:25 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rafael J. Wysocki, Wei Huang, Linux Kernel Mailing List,
	Linux PM, Borislav Petkov, the arch/x86 maintainers

Hi Rafael,

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Fri, Dec 4, 2020 at 11:45 PM Punit Agrawal <punitagrawal@gmail.com> wrote:
>>
>> Hi Rafael,
>>
>> Punit Agrawal <punitagrawal@gmail.com> writes:
>>
>> > Hi,
>> >
>> > While looking into Giovanni's patches to enable frequency invariance
>> > on AMD systems[0], I noticed an issue with initialising frequency
>> > domain information on a recent AMD APU.
>> >
>> > Patch 1 refactors the test to ignore firmware provided frequency
>> > domain into a separate function.
>> >
>> > Patch 2 adds said APU (Family: 0x17, Model: 0x60, Stepping: 0x01) to
>> > the list of CPUs for which the PSD override is ignored. I am not quite
>> > happy with having to special case a particular CPU but also couldn't
>> > find any documentation to help identify the CPUs that don't need the
>> > override.
>>
>> Are you be OK to pick the first two patches if there are no issues?
>
> Please send them as non-RFC and change the name of override_acpi_psd()
> to indicate that it is AMD-specific.

Ack - I will incorporate your comments in the next version (once the
ongoing discussion finishes).

Thanks.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 1/4] cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD
  2020-12-07 19:29   ` Wei Huang
@ 2020-12-08 23:31     ` Punit Agrawal
  0 siblings, 0 replies; 27+ messages in thread
From: Punit Agrawal @ 2020-12-08 23:31 UTC (permalink / raw)
  To: Wei Huang; +Cc: rjw, wei.huang2, linux-kernel, linux-pm, bp, x86

Hi Wei,

Wei Huang <whuang2@amd.com> writes:

> On 11/25/20 8:48 AM, Punit Agrawal wrote:
>> Re-factor the code to override the firmware provided frequency domain
>> information (via PSD) to localise the checks in one function.
>> 
>> No functional change intended.
>> 
>> Signed-off-by: Punit Agrawal <punitagrawal@gmail.com>
>> Cc: Wei Huang <wei.huang2@amd.com>
>> ---
>>  drivers/cpufreq/acpi-cpufreq.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index 1e4fbb002a31..b1e7df96d428 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -191,6 +191,20 @@ static int check_amd_hwpstate_cpu(unsigned int cpuid)
>>  	return cpu_has(cpu, X86_FEATURE_HW_PSTATE);
>>  }
>>  
>> +static int override_acpi_psd(unsigned int cpu_id)
>          ^^^^^
> int is fine, but it might be better to use bool. Otherwise I don't see
> any issues with this patch.

Makes sense - I will switch to a boolean in the next update.

Thanks for taking a look.

Punit

>
>> +{
>> +	struct cpuinfo_x86 *c = &boot_cpu_data;
>> +
>> +	if (c->x86_vendor == X86_VENDOR_AMD) {
>> +		if (!check_amd_hwpstate_cpu(cpu_id))
>> +			return false;
>> +
>> +		return c->x86 < 0x19;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>  static unsigned extract_io(struct cpufreq_policy *policy, u32 value)
>>  {
>>  	struct acpi_cpufreq_data *data = policy->driver_data;
>> @@ -691,8 +705,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>  		cpumask_copy(policy->cpus, topology_core_cpumask(cpu));
>>  	}
>>  
>> -	if (check_amd_hwpstate_cpu(cpu) && boot_cpu_data.x86 < 0x19 &&
>> -	    !acpi_pstate_strict) {
>> +	if (override_acpi_psd(cpu) && !acpi_pstate_strict) {
>>  		cpumask_clear(policy->cpus);
>>  		cpumask_set_cpu(cpu, policy->cpus);
>>  		cpumask_copy(data->freqdomain_cpus,
>> 

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-08 23:21           ` Punit Agrawal
@ 2020-12-08 23:32             ` Borislav Petkov
  2020-12-11 23:36               ` Punit Agrawal
  0 siblings, 1 reply; 27+ messages in thread
From: Borislav Petkov @ 2020-12-08 23:32 UTC (permalink / raw)
  To: Punit Agrawal; +Cc: Wei Huang, rjw, wei.huang2, linux-kernel, linux-pm, x86

On Wed, Dec 09, 2020 at 08:21:48AM +0900, Punit Agrawal wrote:
> According to the commit log, acd316248205 seems to be only targeted at
> powernow-K8 -

No, it is not targeted at powernow-k8 - acpi-cpufreq.c is what is used
on AMD hw. He means to make acpi-cpufreq's behavior consistent with
powernow-k8.

> But if that is not available, the only way we have is to include
> systems that have been verified to not need the override

You have verified exactly *one* system - yours. Or are you sure that
*all* family 0x17, model 0x60, stepping 0x1 machines don't need the
override?

Also, you still haven't explained what you're trying to fix here.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-08 23:32             ` Borislav Petkov
@ 2020-12-11 23:36               ` Punit Agrawal
  2020-12-14 12:40                 ` Borislav Petkov
  0 siblings, 1 reply; 27+ messages in thread
From: Punit Agrawal @ 2020-12-11 23:36 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Wei Huang, rjw, wei.huang2, linux-kernel, linux-pm, x86

Borislav Petkov <bp@alien8.de> writes:

> On Wed, Dec 09, 2020 at 08:21:48AM +0900, Punit Agrawal wrote:
>> According to the commit log, acd316248205 seems to be only targeted at
>> powernow-K8 -
>
> No, it is not targeted at powernow-k8 - acpi-cpufreq.c is what is used
> on AMD hw. He means to make acpi-cpufreq's behavior consistent with
> powernow-k8.

So "powernow-k8" is not a cpu but a cpufreq driver. That doesn't change
the fact that the patch causes all AMD systems using acpi-cpufreq to
ignore processor frequency groupings and treat each processor to be an
in independent frequency domain from cpufreq's point of view.

>> But if that is not available, the only way we have is to include
>> systems that have been verified to not need the override
>
> You have verified exactly *one* system - yours. Or are you sure that
> *all* family 0x17, model 0x60, stepping 0x1 machines don't need the
> override?

Unfortunately, I only have access to one system with that F/M/S.

Since posting the non-RFC patches, I was able to inspect the ACPI tables
for more CPUs -

Family: 0x17h, Model: 0x71h (Ryzen 3950X)
Family: 0x17h, Model: 0x18h (Ryzen 3500u)

To me it suggests, that there are likely more systems from the family
that show the characteristic described below.

> Also, you still haven't explained what you're trying to fix here.

All the CPUs here are multi-threaded with 2 threads per core. The _PSD
for the system describes the cores as having a coupling that consist of
a frequency domain per core that contains both the threads. The firmware
description makes sense and seems to accurately describe the hardware
topology.

In all these systems, the override causes this topology information to
be ignored - treating each core to be a separate domain. The proposed
patch removes the override so that _PSD is taken into account.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-11 23:36               ` Punit Agrawal
@ 2020-12-14 12:40                 ` Borislav Petkov
  2020-12-14 13:27                   ` Punit Agrawal
  0 siblings, 1 reply; 27+ messages in thread
From: Borislav Petkov @ 2020-12-14 12:40 UTC (permalink / raw)
  To: Punit Agrawal; +Cc: Wei Huang, rjw, wei.huang2, linux-kernel, linux-pm, x86

On Sat, Dec 12, 2020 at 08:36:48AM +0900, Punit Agrawal wrote:
> To me it suggests, that there are likely more systems from the family
> that show the characteristic described below.

Until we find a *single* system with a broken BIOS which has those
objects kaputt and then this heuristic would need an exception.

VS the clear statement from AMD that from zen3 onwards, all BIOS will be
tested. I hope they boot Linux at least before they ship.

> In all these systems, the override causes this topology information to
> be ignored - treating each core to be a separate domain. The proposed
> patch removes the override so that _PSD is taken into account.

You're still not answering my question: what does the coupling of the
SMT threads bring on those systems? Power savings? Perf improvement?
Anything palpable or measurable?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-14 12:40                 ` Borislav Petkov
@ 2020-12-14 13:27                   ` Punit Agrawal
  2020-12-14 14:25                     ` Borislav Petkov
  0 siblings, 1 reply; 27+ messages in thread
From: Punit Agrawal @ 2020-12-14 13:27 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Wei Huang, rjw, wei.huang2, linux-kernel, linux-pm, x86

Borislav Petkov <bp@alien8.de> writes:

> On Sat, Dec 12, 2020 at 08:36:48AM +0900, Punit Agrawal wrote:
>> To me it suggests, that there are likely more systems from the family
>> that show the characteristic described below.
>
> Until we find a *single* system with a broken BIOS which has those
> objects kaputt and then this heuristic would need an exception.
>
> VS the clear statement from AMD that from zen3 onwards, all BIOS will be
> tested. I hope they boot Linux at least before they ship.

IIUC, this suggests that Linux booting on anything prior to Zen3 is down
to pure luck - I hope that wasn't the case.

>> In all these systems, the override causes this topology information to
>> be ignored - treating each core to be a separate domain. The proposed
>> patch removes the override so that _PSD is taken into account.
>
> You're still not answering my question: what does the coupling of the
> SMT threads bring on those systems? Power savings? Perf improvement?
> Anything palpable or measurable?

At the moment acpi thermals is bust on this and other affected AMD
system I have access to. That'll need fixing before any sensible
measurements can be run.

Tbh, I didn't quite expect the patch to the PSD exclusion list to be so
controversial - especially when a similar change for zen3 had recently
been merged. If you're really not keen on the change, I will carry it
locally for the time being and revisit once the other issues have been
resolved.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-14 13:27                   ` Punit Agrawal
@ 2020-12-14 14:25                     ` Borislav Petkov
  2020-12-17 13:27                       ` Punit Agrawal
  0 siblings, 1 reply; 27+ messages in thread
From: Borislav Petkov @ 2020-12-14 14:25 UTC (permalink / raw)
  To: Punit Agrawal; +Cc: Wei Huang, rjw, wei.huang2, linux-kernel, linux-pm, x86

On Mon, Dec 14, 2020 at 10:27:09PM +0900, Punit Agrawal wrote:
> IIUC, this suggests that Linux booting on anything prior to Zen3 is down
> to pure luck - I hope that wasn't the case.

WTF does this have to do with linux booting?!

> At the moment acpi thermals is bust on this and other affected AMD
> system I have access to. That'll need fixing before any sensible
> measurements can be run.

Nope, still not answering my questions.

> Tbh, I didn't quite expect the patch to the PSD exclusion list to be
> so controversial

It won't be if you explain properly what your patch is fixing. That is,
if it fixes anything.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list
  2020-12-14 14:25                     ` Borislav Petkov
@ 2020-12-17 13:27                       ` Punit Agrawal
  0 siblings, 0 replies; 27+ messages in thread
From: Punit Agrawal @ 2020-12-17 13:27 UTC (permalink / raw)
  To: Borislav Petkov; +Cc: Wei Huang, rjw, wei.huang2, linux-kernel, linux-pm, x86

Borislav Petkov <bp@alien8.de> writes:

> On Mon, Dec 14, 2020 at 10:27:09PM +0900, Punit Agrawal wrote:
>> IIUC, this suggests that Linux booting on anything prior to Zen3 is down
>> to pure luck - I hope that wasn't the case.
>
> WTF does this have to do with linux booting?!

I guess I misunderstood the comment from your previous mail. Pasting
back for context (emphasis mine) -

    VS the clear statement from AMD that from zen3 onwards, all BIOS
    will be tested. *I hope they boot Linux at least before they ship.*

>> At the moment acpi thermals is bust on this and other affected AMD
>> system I have access to. That'll need fixing before any sensible
>> measurements can be run.
>
> Nope, still not answering my questions.
>
>> Tbh, I didn't quite expect the patch to the PSD exclusion list to be
>> so controversial
>
> It won't be if you explain properly what your patch is fixing. That is,
> if it fixes anything.

I stared at the driver code (and the ACPI tables for the platform) to
see if I could provide a better explanation.

That's when I realised that as the platform advertises hardware
frequency co-ordination, even without the override it still skips
setting the policy cpus -

    /*
     * Will let policy->cpus know about dependency only when software
     * coordination is required.
     */
    if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL ||
        policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
            cpumask_copy(policy->cpus, perf->shared_cpu_map);
    }

This ends up treating each CPU as an independent frequency domain
anyways. So even ignoring the override for the CPU, doesn't change
anything other than dropping the message from boot log -

    overriding BIOS provided _PSD data

As such, there's no point in merging the patch as it is.

Apologies for the noise. I should've checked more thoroughly before
sending the patches. 

[ Aside: If Zen3 is using hardware co-ordination it'll probably face the
issue described above as well. ]

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2020-12-17 13:28 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-25 14:48 [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
2020-11-25 14:48 ` [RFC PATCH 1/4] cpufreq: acpi-cpufreq: Re-factor overriding ACPI PSD Punit Agrawal
2020-12-07 19:29   ` Wei Huang
2020-12-08 23:31     ` Punit Agrawal
2020-11-25 14:48 ` [RFC PATCH 2/4] cpufreq: acpi-cpufreq: Add processor to the ignore PSD override list Punit Agrawal
2020-12-07 20:20   ` Wei Huang
2020-12-07 20:26     ` Borislav Petkov
2020-12-07 22:07       ` Wei Huang
2020-12-07 22:30         ` Borislav Petkov
2020-12-08  3:44           ` Wei Huang
2020-12-08 23:21           ` Punit Agrawal
2020-12-08 23:32             ` Borislav Petkov
2020-12-11 23:36               ` Punit Agrawal
2020-12-14 12:40                 ` Borislav Petkov
2020-12-14 13:27                   ` Punit Agrawal
2020-12-14 14:25                     ` Borislav Petkov
2020-12-17 13:27                       ` Punit Agrawal
2020-11-25 14:48 ` [RFC PATCH 3/4] x86/cpu: amd: Define processor families Punit Agrawal
2020-11-30 14:00   ` Borislav Petkov
2020-12-02 14:13     ` Punit Agrawal
2020-12-02 16:57       ` Borislav Petkov
2020-11-25 14:48 ` [RFC PATCH 4/4] cpufreq: acpi-cpufreq: Use identifiers for AMD processor family Punit Agrawal
2020-11-30 14:02   ` Borislav Petkov
2020-12-02 14:30     ` Punit Agrawal
2020-12-04 22:44 ` [RFC PATCH 0/4] Add processor to the ignore PSD override list Punit Agrawal
2020-12-07 13:55   ` Rafael J. Wysocki
2020-12-08 23:25     ` Punit Agrawal

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.