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 <[email protected]>
Cc: Wei Huang <[email protected]>
---
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
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 <[email protected]>
> Cc: Wei Huang <[email protected]>
> ---
> 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;
>
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
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?
>
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
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.
>
Borislav Petkov <[email protected]> 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.
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
Borislav Petkov <[email protected]> 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.
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
Borislav Petkov <[email protected]> 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.
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
Borislav Petkov <[email protected]> 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. ]