Konrad Rzeszutek Wilk
2013-Mar-05 19:45 UTC
[PATCH] ACPI: Add fixups for AMD P-state figures.
This a copy-n-paste from two Linux git commits:
- f594065faf4f9067c2283a34619fc0714e79a98d
ACPI: Add fixups for AMD P-state figures
- 9855d8ce41a7801548a05d844db2f46c3e810166
ACPI: Check MSR valid bit before using P-state frequencies
The issue is that "some AMD systems may round the frequencies in
ACPI tables to 100MHz boundaries. We canobtain the real
frequencies from MSRs, so add a quirk to fix these frequencies up
on AMD systems." (from f594065..)
In discussion (around 9855d8..) "it turned out that indeed real
HW/BIOSes may choose to not set the valid bit and thus mark the
P-state as invalid. So this could be considered a fix for broken
BIOSes that also works around the issue on Xen." (from 9855d8..)
I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2
08/20/2008 where this quirk can indeed be observed.
CC: stefan.bader@canonical.com
CC: bp@suse.de
CC: borislav.ostrovsky@oracle.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
xen/arch/x86/acpi/cpufreq/powernow.c | 38 ++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c
b/xen/arch/x86/acpi/cpufreq/powernow.c
index a9b7792..0eaa16c 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -146,7 +146,43 @@ static int powernow_cpufreq_target(struct cpufreq_policy
*policy,
return 0;
}
+#define MSR_AMD_PSTATE_DEF_BASE 0xc0010064
+static void amd_fixup_frequency(struct xen_processor_px *px)
+{
+ u32 hi, lo, fid, did;
+ int index = px->control & 0x00000007;
+
+ if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
+ return;
+
+ if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10)
+ || boot_cpu_data.x86 == 0x11) {
+ rdmsr(MSR_AMD_PSTATE_DEF_BASE + index, lo, hi);
+ /*
+ * MSR C001_0064+:
+ * Bit 63: PstateEn. Read-write. If set, the P-state is valid.
+ */
+ if (!(hi & (1UL << 31)))
+ return;
+
+ fid = lo & 0x3f;
+ did = (lo >> 6) & 7;
+ if (boot_cpu_data.x86 == 0x10)
+ px->core_frequency = (100 * (fid + 0x10)) >> did;
+ else
+ px->core_frequency = (100 * (fid + 8)) >> did;
+ }
+}
+
+static void amd_fixup_freq(struct processor_performance *perf)
+{
+ int i;
+
+ for (i = 0; i < perf->state_count; i++)
+ amd_fixup_frequency(&perf->states[i]);
+
+}
static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
{
struct acpi_cpufreq_data *data;
@@ -253,6 +289,8 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy
*policy)
policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR;
+ amd_fixup_freq(perf);
+
/* table init */
for (i = 0; i < perf->state_count && i <= max_hw_pstate;
i++) {
if (i > 0 && perf->states[i].core_frequency >--
1.8.0.2
Boris Ostrovsky
2013-Mar-05 20:22 UTC
Re: [PATCH] ACPI: Add fixups for AMD P-state figures.
On 03/05/2013 02:45 PM, Konrad Rzeszutek Wilk wrote:> This a copy-n-paste from two Linux git commits: > > - f594065faf4f9067c2283a34619fc0714e79a98d > ACPI: Add fixups for AMD P-state figures > - 9855d8ce41a7801548a05d844db2f46c3e810166 > ACPI: Check MSR valid bit before using P-state frequencies > > The issue is that "some AMD systems may round the frequencies in > ACPI tables to 100MHz boundaries. We canobtain the real > frequencies from MSRs, so add a quirk to fix these frequencies up > on AMD systems." (from f594065..) > > In discussion (around 9855d8..) "it turned out that indeed real > HW/BIOSes may choose to not set the valid bit and thus mark the > P-state as invalid. So this could be considered a fix for broken > BIOSes that also works around the issue on Xen." (from 9855d8..) > > I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 > 08/20/2008 where this quirk can indeed be observed. > > CC: stefan.bader@canonical.com > CC: bp@suse.de > CC: borislav.ostrovsky@oracle.comboris.ostrovsky@oracle.com> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/arch/x86/acpi/cpufreq/powernow.c | 38 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c > index a9b7792..0eaa16c 100644 > --- a/xen/arch/x86/acpi/cpufreq/powernow.c > +++ b/xen/arch/x86/acpi/cpufreq/powernow.c > @@ -146,7 +146,43 @@ static int powernow_cpufreq_target(struct cpufreq_policy *policy, > > return 0; > } > +#define MSR_AMD_PSTATE_DEF_BASE 0xc0010064There is MSR_PSTATE_DEF_BASE at the top of this file which is the same thing.> +static void amd_fixup_frequency(struct xen_processor_px *px) > +{ > + u32 hi, lo, fid, did; > + int index = px->control & 0x00000007; > + > + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > + return; > + > + if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) > + || boot_cpu_data.x86 == 0x11) { > + rdmsr(MSR_AMD_PSTATE_DEF_BASE + index, lo, hi); > + /* > + * MSR C001_0064+: > + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. > + */ > + if (!(hi & (1UL << 31))) > + return;Identation is off. -boris> + > + fid = lo & 0x3f; > + did = (lo >> 6) & 7; > + if (boot_cpu_data.x86 == 0x10) > + px->core_frequency = (100 * (fid + 0x10)) >> did; > + else > + px->core_frequency = (100 * (fid + 8)) >> did; > + } > +} > + > +static void amd_fixup_freq(struct processor_performance *perf) > +{ > > + int i; > + > + for (i = 0; i < perf->state_count; i++) > + amd_fixup_frequency(&perf->states[i]); > + > +} > static int powernow_cpufreq_verify(struct cpufreq_policy *policy) > { > struct acpi_cpufreq_data *data; > @@ -253,6 +289,8 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy *policy) > > policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR; > > + amd_fixup_freq(perf); > + > /* table init */ > for (i = 0; i < perf->state_count && i <= max_hw_pstate; i++) { > if (i > 0 && perf->states[i].core_frequency >=
Konrad Rzeszutek Wilk
2013-Mar-05 21:33 UTC
Re: [PATCH] ACPI: Add fixups for AMD P-state figures.
On Tue, Mar 05, 2013 at 03:22:25PM -0500, Boris Ostrovsky wrote:> On 03/05/2013 02:45 PM, Konrad Rzeszutek Wilk wrote: > >This a copy-n-paste from two Linux git commits: > > > >- f594065faf4f9067c2283a34619fc0714e79a98d > > ACPI: Add fixups for AMD P-state figures > >- 9855d8ce41a7801548a05d844db2f46c3e810166 > > ACPI: Check MSR valid bit before using P-state frequencies > > > >The issue is that "some AMD systems may round the frequencies in > >ACPI tables to 100MHz boundaries. We canobtain the real > >frequencies from MSRs, so add a quirk to fix these frequencies up > >on AMD systems." (from f594065..) > > > >In discussion (around 9855d8..) "it turned out that indeed real > >HW/BIOSes may choose to not set the valid bit and thus mark the > >P-state as invalid. So this could be considered a fix for broken > >BIOSes that also works around the issue on Xen." (from 9855d8..) > > > >I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 > >08/20/2008 where this quirk can indeed be observed. > > > >CC: stefan.bader@canonical.com > >CC: bp@suse.de > >CC: borislav.ostrovsky@oracle.com > > boris.ostrovsky@oracle.comWhoops! Here is an updated version: From 3b7584f0c3c91d073bd760a038d0091b3bf5a19b Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Date: Tue, 5 Mar 2013 14:40:52 -0500 Subject: [PATCH] ACPI: Add fixups for AMD P-state figures. This a copy-n-paste from two Linux git commits: - f594065faf4f9067c2283a34619fc0714e79a98d ACPI: Add fixups for AMD P-state figures - 9855d8ce41a7801548a05d844db2f46c3e810166 ACPI: Check MSR valid bit before using P-state frequencies The issue is that "some AMD systems may round the frequencies in ACPI tables to 100MHz boundaries. We canobtain the real frequencies from MSRs, so add a quirk to fix these frequencies up on AMD systems." (from f594065..) In discussion (around 9855d8..) "it turned out that indeed real HW/BIOSes may choose to not set the valid bit and thus mark the P-state as invalid. So this could be considered a fix for broken BIOSes that also works around the issue on Xen." (from 9855d8..) I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 08/20/2008 where this quirk can indeed be observed. CC: stefan.bader@canonical.com CC: bp@suse.de CC: boris.ostrovsky@oracle.com [v1: Indent, #define, and email changes per Boris''s review] Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/arch/x86/acpi/cpufreq/powernow.c | 38 ++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c index a9b7792..5037c30 100644 --- a/xen/arch/x86/acpi/cpufreq/powernow.c +++ b/xen/arch/x86/acpi/cpufreq/powernow.c @@ -147,6 +147,42 @@ static int powernow_cpufreq_target(struct cpufreq_policy *policy, return 0; } +static void amd_fixup_frequency(struct xen_processor_px *px) +{ + u32 hi, lo, fid, did; + int index = px->control & 0x00000007; + + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) + return; + + if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) + || boot_cpu_data.x86 == 0x11) { + rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi); + /* + * MSR C001_0064+: + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. + */ + if (!(hi & (1UL << 31))) + return; + + fid = lo & 0x3f; + did = (lo >> 6) & 7; + if (boot_cpu_data.x86 == 0x10) + px->core_frequency = (100 * (fid + 0x10)) >> did; + else + px->core_frequency = (100 * (fid + 8)) >> did; + } +} + +static void amd_fixup_freq(struct processor_performance *perf) +{ + + int i; + + for (i = 0; i < perf->state_count; i++) + amd_fixup_frequency(&perf->states[i]); + +} static int powernow_cpufreq_verify(struct cpufreq_policy *policy) { struct acpi_cpufreq_data *data; @@ -253,6 +289,8 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy *policy) policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR; + amd_fixup_freq(perf); + /* table init */ for (i = 0; i < perf->state_count && i <= max_hw_pstate; i++) { if (i > 0 && perf->states[i].core_frequency >-- 1.8.0.2
Boris Ostrovsky
2013-Mar-05 22:12 UTC
Re: [PATCH] ACPI: Add fixups for AMD P-state figures.
On 03/05/2013 04:33 PM, Konrad Rzeszutek Wilk wrote:> On Tue, Mar 05, 2013 at 03:22:25PM -0500, Boris Ostrovsky wrote: >> On 03/05/2013 02:45 PM, Konrad Rzeszutek Wilk wrote: >>> This a copy-n-paste from two Linux git commits: >>> >>> - f594065faf4f9067c2283a34619fc0714e79a98d >>> ACPI: Add fixups for AMD P-state figures >>> - 9855d8ce41a7801548a05d844db2f46c3e810166 >>> ACPI: Check MSR valid bit before using P-state frequencies >>> >>> The issue is that "some AMD systems may round the frequencies in >>> ACPI tables to 100MHz boundaries. We canobtain the real >>> frequencies from MSRs, so add a quirk to fix these frequencies up >>> on AMD systems." (from f594065..) >>> >>> In discussion (around 9855d8..) "it turned out that indeed real >>> HW/BIOSes may choose to not set the valid bit and thus mark the >>> P-state as invalid. So this could be considered a fix for broken >>> BIOSes that also works around the issue on Xen." (from 9855d8..) >>> >>> I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 >>> 08/20/2008 where this quirk can indeed be observed. >>> >>> CC: stefan.bader@canonical.com >>> CC: bp@suse.de >>> CC: borislav.ostrovsky@oracle.com >> boris.ostrovsky@oracle.com > Whoops! > > Here is an updated version: > > From 3b7584f0c3c91d073bd760a038d0091b3bf5a19b Mon Sep 17 00:00:00 2001 > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Date: Tue, 5 Mar 2013 14:40:52 -0500 > Subject: [PATCH] ACPI: Add fixups for AMD P-state figures. > > This a copy-n-paste from two Linux git commits: > > - f594065faf4f9067c2283a34619fc0714e79a98d > ACPI: Add fixups for AMD P-state figures > - 9855d8ce41a7801548a05d844db2f46c3e810166 > ACPI: Check MSR valid bit before using P-state frequencies > > The issue is that "some AMD systems may round the frequencies in > ACPI tables to 100MHz boundaries. We canobtain the real > frequencies from MSRs, so add a quirk to fix these frequencies up > on AMD systems." (from f594065..) > > In discussion (around 9855d8..) "it turned out that indeed real > HW/BIOSes may choose to not set the valid bit and thus mark the > P-state as invalid. So this could be considered a fix for broken > BIOSes that also works around the issue on Xen." (from 9855d8..) > > I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 > 08/20/2008 where this quirk can indeed be observed. > > CC: stefan.bader@canonical.com > CC: bp@suse.de > CC: boris.ostrovsky@oracle.com > [v1: Indent, #define, and email changes per Boris''s review] > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/arch/x86/acpi/cpufreq/powernow.c | 38 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c > index a9b7792..5037c30 100644 > --- a/xen/arch/x86/acpi/cpufreq/powernow.c > +++ b/xen/arch/x86/acpi/cpufreq/powernow.c > @@ -147,6 +147,42 @@ static int powernow_cpufreq_target(struct cpufreq_policy *policy, > return 0; > } > > +static void amd_fixup_frequency(struct xen_processor_px *px) > +{ > + u32 hi, lo, fid, did; > + int index = px->control & 0x00000007; > + > + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > + return; > + > + if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) > + || boot_cpu_data.x86 == 0x11) { > + rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi); > + /* > + * MSR C001_0064+: > + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. > + */ > + if (!(hi & (1UL << 31))) > + return; > + > + fid = lo & 0x3f; > + did = (lo >> 6) & 7; > + if (boot_cpu_data.x86 == 0x10) > + px->core_frequency = (100 * (fid + 0x10)) >> did; > + else > + px->core_frequency = (100 * (fid + 8)) >> did; > + } > +}There is still something wrong with indentation, I think it''s tabs vs. spaces. Everything inside if clause (except rdmsr) is indented by spaces but the rest uses tabs. -boris
>>> On 05.03.13 at 22:33, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > +static void amd_fixup_frequency(struct xen_processor_px *px) > +{ > + u32 hi, lo, fid, did; > + int index = px->control & 0x00000007; > + > + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > + return;This is pointless, the driver as a whole is already AMD specific.> + > + if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) > + || boot_cpu_data.x86 == 0x11) {Instead I wonder whether this could (properly inverted) serve as an early return condition, reducing indentation on the subsequent block.> + rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi); > + /* > + * MSR C001_0064+: > + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. > + */ > + if (!(hi & (1UL << 31))) > + return; > + > + fid = lo & 0x3f; > + did = (lo >> 6) & 7; > + if (boot_cpu_data.x86 == 0x10) > + px->core_frequency = (100 * (fid + 0x10)) >> did; > + else > + px->core_frequency = (100 * (fid + 8)) >> did;0x10 vs 8? Please settle on decimal (preferred) or hex numbers in a calculation like this.> + } > +}And as Boris already pointed out - indentation here should be consistent in itself _and_ with the rest of the file.> + > +static void amd_fixup_freq(struct processor_performance *perf) > +{ > + > + int i;unsigned int Jan> + > + for (i = 0; i < perf->state_count; i++) > + amd_fixup_frequency(&perf->states[i]); > + > +} > static int powernow_cpufreq_verify(struct cpufreq_policy *policy) > { > struct acpi_cpufreq_data *data;
On 05.03.2013 22:33, Konrad Rzeszutek Wilk wrote:> On Tue, Mar 05, 2013 at 03:22:25PM -0500, Boris Ostrovsky wrote: >> On 03/05/2013 02:45 PM, Konrad Rzeszutek Wilk wrote: >>> This a copy-n-paste from two Linux git commits: >>> >>> - f594065faf4f9067c2283a34619fc0714e79a98d >>> ACPI: Add fixups for AMD P-state figures >>> - 9855d8ce41a7801548a05d844db2f46c3e810166 >>> ACPI: Check MSR valid bit before using P-state frequencies >>> >>> The issue is that "some AMD systems may round the frequencies in >>> ACPI tables to 100MHz boundaries. We canobtain the real >>> frequencies from MSRs, so add a quirk to fix these frequencies up >>> on AMD systems." (from f594065..) >>> >>> In discussion (around 9855d8..) "it turned out that indeed real >>> HW/BIOSes may choose to not set the valid bit and thus mark the >>> P-state as invalid. So this could be considered a fix for broken >>> BIOSes that also works around the issue on Xen." (from 9855d8..) >>> >>> I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 >>> 08/20/2008 where this quirk can indeed be observed. >>> >>> CC: stefan.bader@canonical.com >>> CC: bp@suse.de >>> CC: borislav.ostrovsky@oracle.com >> >> boris.ostrovsky@oracle.com > > Whoops! > > Here is an updated version: > > From 3b7584f0c3c91d073bd760a038d0091b3bf5a19b Mon Sep 17 00:00:00 2001 > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Date: Tue, 5 Mar 2013 14:40:52 -0500 > Subject: [PATCH] ACPI: Add fixups for AMD P-state figures. > > This a copy-n-paste from two Linux git commits: > > - f594065faf4f9067c2283a34619fc0714e79a98d > ACPI: Add fixups for AMD P-state figures > - 9855d8ce41a7801548a05d844db2f46c3e810166 > ACPI: Check MSR valid bit before using P-state frequencies > > The issue is that "some AMD systems may round the frequencies in > ACPI tables to 100MHz boundaries. We canobtain the real > frequencies from MSRs, so add a quirk to fix these frequencies up > on AMD systems." (from f594065..) > > In discussion (around 9855d8..) "it turned out that indeed real > HW/BIOSes may choose to not set the valid bit and thus mark the > P-state as invalid. So this could be considered a fix for broken > BIOSes that also works around the issue on Xen." (from 9855d8..)Boris and Jan already pointed out more than I would have spotted. So it seems the only thing left is the commit description. Well maybe it is just my way of reading it but it feels like here the actual description/argument is missing. I think it might be that Xen gets the unmodified values from the ACPI parsing in dom0 because it cannot/does not want to allow dom0 to read the MSR. Instead this patch will cause the frequencies to be adapted in the hypervisor. -Stefan> > I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 > 08/20/2008 where this quirk can indeed be observed. > > CC: stefan.bader@canonical.com > CC: bp@suse.de > CC: boris.ostrovsky@oracle.com > [v1: Indent, #define, and email changes per Boris''s review] > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/arch/x86/acpi/cpufreq/powernow.c | 38 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c > index a9b7792..5037c30 100644 > --- a/xen/arch/x86/acpi/cpufreq/powernow.c > +++ b/xen/arch/x86/acpi/cpufreq/powernow.c > @@ -147,6 +147,42 @@ static int powernow_cpufreq_target(struct cpufreq_policy *policy, > return 0; > } > > +static void amd_fixup_frequency(struct xen_processor_px *px) > +{ > + u32 hi, lo, fid, did; > + int index = px->control & 0x00000007; > + > + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > + return; > + > + if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) > + || boot_cpu_data.x86 == 0x11) { > + rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi); > + /* > + * MSR C001_0064+: > + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. > + */ > + if (!(hi & (1UL << 31))) > + return; > + > + fid = lo & 0x3f; > + did = (lo >> 6) & 7; > + if (boot_cpu_data.x86 == 0x10) > + px->core_frequency = (100 * (fid + 0x10)) >> did; > + else > + px->core_frequency = (100 * (fid + 8)) >> did; > + } > +} > + > +static void amd_fixup_freq(struct processor_performance *perf) > +{ > + > + int i; > + > + for (i = 0; i < perf->state_count; i++) > + amd_fixup_frequency(&perf->states[i]); > + > +} > static int powernow_cpufreq_verify(struct cpufreq_policy *policy) > { > struct acpi_cpufreq_data *data; > @@ -253,6 +289,8 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy *policy) > > policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR; > > + amd_fixup_freq(perf); > + > /* table init */ > for (i = 0; i < perf->state_count && i <= max_hw_pstate; i++) { > if (i > 0 && perf->states[i].core_frequency >>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Borislav Petkov
2013-Mar-06 10:30 UTC
Re: [PATCH] ACPI: Add fixups for AMD P-state figures.
On Wed, Mar 06, 2013 at 09:05:57AM +0000, Jan Beulich wrote:> > + if (boot_cpu_data.x86 == 0x10) > > + px->core_frequency = (100 * (fid + 0x10)) >> did; > > + else > > + px->core_frequency = (100 * (fid + 8)) >> did; > > 0x10 vs 8? Please settle on decimal (preferred) or hex numbers in > a calculation like this.This is directly copied from upstream. Don''t ask me why it was done like that. :-) -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. --
Konrad Rzeszutek Wilk
2013-Mar-06 15:51 UTC
Re: [PATCH] ACPI: Add fixups for AMD P-state figures.
On Wed, Mar 06, 2013 at 10:48:01AM +0100, Stefan Bader wrote:> On 05.03.2013 22:33, Konrad Rzeszutek Wilk wrote: > > On Tue, Mar 05, 2013 at 03:22:25PM -0500, Boris Ostrovsky wrote: > >> On 03/05/2013 02:45 PM, Konrad Rzeszutek Wilk wrote: > >>> This a copy-n-paste from two Linux git commits: > >>> > >>> - f594065faf4f9067c2283a34619fc0714e79a98d > >>> ACPI: Add fixups for AMD P-state figures > >>> - 9855d8ce41a7801548a05d844db2f46c3e810166 > >>> ACPI: Check MSR valid bit before using P-state frequencies > >>> > >>> The issue is that "some AMD systems may round the frequencies in > >>> ACPI tables to 100MHz boundaries. We canobtain the real > >>> frequencies from MSRs, so add a quirk to fix these frequencies up > >>> on AMD systems." (from f594065..) > >>> > >>> In discussion (around 9855d8..) "it turned out that indeed real > >>> HW/BIOSes may choose to not set the valid bit and thus mark the > >>> P-state as invalid. So this could be considered a fix for broken > >>> BIOSes that also works around the issue on Xen." (from 9855d8..) > >>> > >>> I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 > >>> 08/20/2008 where this quirk can indeed be observed. > >>> > >>> CC: stefan.bader@canonical.com > >>> CC: bp@suse.de > >>> CC: borislav.ostrovsky@oracle.com > >> > >> boris.ostrovsky@oracle.com > > > > Whoops! > > > > Here is an updated version: > > > > From 3b7584f0c3c91d073bd760a038d0091b3bf5a19b Mon Sep 17 00:00:00 2001 > > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Date: Tue, 5 Mar 2013 14:40:52 -0500 > > Subject: [PATCH] ACPI: Add fixups for AMD P-state figures. > > > > This a copy-n-paste from two Linux git commits: > > > > - f594065faf4f9067c2283a34619fc0714e79a98d > > ACPI: Add fixups for AMD P-state figures > > - 9855d8ce41a7801548a05d844db2f46c3e810166 > > ACPI: Check MSR valid bit before using P-state frequencies > > > > The issue is that "some AMD systems may round the frequencies in > > ACPI tables to 100MHz boundaries. We canobtain the real > > frequencies from MSRs, so add a quirk to fix these frequencies up > > on AMD systems." (from f594065..) > > > > In discussion (around 9855d8..) "it turned out that indeed real > > HW/BIOSes may choose to not set the valid bit and thus mark the > > P-state as invalid. So this could be considered a fix for broken > > BIOSes that also works around the issue on Xen." (from 9855d8..) > > Boris and Jan already pointed out more than I would have spotted. So it seems > the only thing left is the commit description. Well maybe it is just my way of > reading it but it feels like here the actual description/argument is missing. > > I think it might be that Xen gets the unmodified values from the ACPI parsing in > dom0 because it cannot/does not want to allow dom0 to read the MSR.Right.> Instead this patch will cause the frequencies to be adapted in the hypervisor.Correct. I will update the git commit with such wording and send out an updated patch shortly.
Konrad Rzeszutek Wilk
2013-Mar-06 15:53 UTC
Re: [PATCH] ACPI: Add fixups for AMD P-state figures.
On Wed, Mar 06, 2013 at 09:05:57AM +0000, Jan Beulich wrote:> >>> On 05.03.13 at 22:33, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > +static void amd_fixup_frequency(struct xen_processor_px *px) > > +{ > > + u32 hi, lo, fid, did; > > + int index = px->control & 0x00000007; > > + > > + if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > > + return; > > This is pointless, the driver as a whole is already AMD specific. > > > + > > + if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) > > + || boot_cpu_data.x86 == 0x11) { > > Instead I wonder whether this could (properly inverted) serve as > an early return condition, reducing indentation on the subsequent > block. > > > + rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi); > > + /* > > + * MSR C001_0064+: > > + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. > > + */ > > + if (!(hi & (1UL << 31))) > > + return; > > + > > + fid = lo & 0x3f; > > + did = (lo >> 6) & 7; > > + if (boot_cpu_data.x86 == 0x10) > > + px->core_frequency = (100 * (fid + 0x10)) >> did; > > + else > > + px->core_frequency = (100 * (fid + 8)) >> did; > > 0x10 vs 8? Please settle on decimal (preferred) or hex numbers in > a calculation like this. > > > + } > > +} > > And as Boris already pointed out - indentation here should be > consistent in itself _and_ with the rest of the file.I not sure if it is my editor - but under vim it looks fine. It is just when I send it and look under ''mutt'' then I see it. Either way, let me make the changes you suggested and send out a revised patch shortly.> > > + > > +static void amd_fixup_freq(struct processor_performance *perf) > > +{ > > + > > + int i; > > unsigned int > > Jan > > > + > > + for (i = 0; i < perf->state_count; i++) > > + amd_fixup_frequency(&perf->states[i]); > > + > > +} > > static int powernow_cpufreq_verify(struct cpufreq_policy *policy) > > { > > struct acpi_cpufreq_data *data; > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel >
Konrad Rzeszutek Wilk
2013-Mar-06 21:37 UTC
Re: [PATCH] ACPI: Add fixups for AMD P-state figures.
On Wed, Mar 06, 2013 at 10:51:12AM -0500, Konrad Rzeszutek Wilk wrote:> On Wed, Mar 06, 2013 at 10:48:01AM +0100, Stefan Bader wrote: > > On 05.03.2013 22:33, Konrad Rzeszutek Wilk wrote: > > > On Tue, Mar 05, 2013 at 03:22:25PM -0500, Boris Ostrovsky wrote: > > >> On 03/05/2013 02:45 PM, Konrad Rzeszutek Wilk wrote: > > >>> This a copy-n-paste from two Linux git commits: > > >>> > > >>> - f594065faf4f9067c2283a34619fc0714e79a98d > > >>> ACPI: Add fixups for AMD P-state figures > > >>> - 9855d8ce41a7801548a05d844db2f46c3e810166 > > >>> ACPI: Check MSR valid bit before using P-state frequencies > > >>> > > >>> The issue is that "some AMD systems may round the frequencies in > > >>> ACPI tables to 100MHz boundaries. We canobtain the real > > >>> frequencies from MSRs, so add a quirk to fix these frequencies up > > >>> on AMD systems." (from f594065..) > > >>> > > >>> In discussion (around 9855d8..) "it turned out that indeed real > > >>> HW/BIOSes may choose to not set the valid bit and thus mark the > > >>> P-state as invalid. So this could be considered a fix for broken > > >>> BIOSes that also works around the issue on Xen." (from 9855d8..) > > >>> > > >>> I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 > > >>> 08/20/2008 where this quirk can indeed be observed. > > >>> > > >>> CC: stefan.bader@canonical.com > > >>> CC: bp@suse.de > > >>> CC: borislav.ostrovsky@oracle.com > > >> > > >> boris.ostrovsky@oracle.com > > > > > > Whoops! > > > > > > Here is an updated version:From b704d4419e9e483922382d2339bd41c245f435e1 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Date: Tue, 5 Mar 2013 14:40:52 -0500 Subject: [PATCH] ACPI: Add fixups for AMD P-state figures. In the Linux kernel, these two git commits: - f594065faf4f9067c2283a34619fc0714e79a98d ACPI: Add fixups for AMD P-state figures - 9855d8ce41a7801548a05d844db2f46c3e810166 ACPI: Check MSR valid bit before using P-state frequencies Try to fix the the issue that "some AMD systems may round the frequencies in ACPI tables to 100MHz boundaries. We can obtain the real frequencies from MSRs, so add a quirk to fix these frequencies up on AMD systems." (from f594065..) In discussion (around 9855d8..) "it turned out that indeed real HW/BIOSes may choose to not set the valid bit and thus mark the P-state as invalid. So this could be considered a fix for broken BIOSes." (from 9855d8..) which is great for Linux. Unfortunatly the Linux kernel, when it tries to do the RDMSR under Xen it fails to get the right value (it gets zero) as Xen traps it and returns zero. Hence when dom0 uploads the P-states they will be unmodified and we should take care of updating the frequencies with the right values. I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 08/20/2008 where this quirk can be observed (x86 == 0x10, model == 2). Also on other AMD (x86 == 0x12, A8-3850; x86 = 0x14, AMD E-350) to make sure the quirk is not applied there. CC: stefan.bader@canonical.com CC: bp@suse.de CC: boris.ostrovsky@oracle.com [v1: Indent, #define, and email changes per Boris''s review] [v2: Redid the x86+model check, updated description] Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/arch/x86/acpi/cpufreq/powernow.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c index a9b7792..8c96fe0 100644 --- a/xen/arch/x86/acpi/cpufreq/powernow.c +++ b/xen/arch/x86/acpi/cpufreq/powernow.c @@ -147,6 +147,40 @@ static int powernow_cpufreq_target(struct cpufreq_policy *policy, return 0; } +static void amd_fixup_frequency(struct xen_processor_px *px) +{ + u32 hi, lo, fid, did; + int index = px->control & 0x00000007; + + if (!(boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) && + !(boot_cpu_data.x86 == 0x11)) + return; + + rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi); + /* + * MSR C001_0064+: + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. + */ + if (!(hi & (1UL << 31))) + return; + + fid = lo & 0x3f; + did = (lo >> 6) & 7; + if (boot_cpu_data.x86 == 0x10) + px->core_frequency = (100 * (fid + 16)) >> did; + else + px->core_frequency = (100 * (fid + 8)) >> did; +} + +static void amd_fixup_freq(struct processor_performance *perf) +{ + + unsigned int i; + + for (i = 0; i < perf->state_count; i++) + amd_fixup_frequency(&perf->states[i]); + +} static int powernow_cpufreq_verify(struct cpufreq_policy *policy) { struct acpi_cpufreq_data *data; @@ -253,6 +287,8 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy *policy) policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR; + amd_fixup_freq(perf); + /* table init */ for (i = 0; i < perf->state_count && i <= max_hw_pstate; i++) { if (i > 0 && perf->states[i].core_frequency >-- 1.8.0.2
On 06.03.2013 22:37, Konrad Rzeszutek Wilk wrote:> On Wed, Mar 06, 2013 at 10:51:12AM -0500, Konrad Rzeszutek Wilk wrote: >> On Wed, Mar 06, 2013 at 10:48:01AM +0100, Stefan Bader wrote: >>> On 05.03.2013 22:33, Konrad Rzeszutek Wilk wrote: >>>> On Tue, Mar 05, 2013 at 03:22:25PM -0500, Boris Ostrovsky wrote: >>>>> On 03/05/2013 02:45 PM, Konrad Rzeszutek Wilk wrote: >>>>>> This a copy-n-paste from two Linux git commits: >>>>>> >>>>>> - f594065faf4f9067c2283a34619fc0714e79a98d >>>>>> ACPI: Add fixups for AMD P-state figures >>>>>> - 9855d8ce41a7801548a05d844db2f46c3e810166 >>>>>> ACPI: Check MSR valid bit before using P-state frequencies >>>>>> >>>>>> The issue is that "some AMD systems may round the frequencies in >>>>>> ACPI tables to 100MHz boundaries. We canobtain the real >>>>>> frequencies from MSRs, so add a quirk to fix these frequencies up >>>>>> on AMD systems." (from f594065..) >>>>>> >>>>>> In discussion (around 9855d8..) "it turned out that indeed real >>>>>> HW/BIOSes may choose to not set the valid bit and thus mark the >>>>>> P-state as invalid. So this could be considered a fix for broken >>>>>> BIOSes that also works around the issue on Xen." (from 9855d8..) >>>>>> >>>>>> I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 >>>>>> 08/20/2008 where this quirk can indeed be observed. >>>>>> >>>>>> CC: stefan.bader@canonical.com >>>>>> CC: bp@suse.de >>>>>> CC: borislav.ostrovsky@oracle.com >>>>> >>>>> boris.ostrovsky@oracle.com >>>> >>>> Whoops! >>>> >>>> Here is an updated version: > > > From b704d4419e9e483922382d2339bd41c245f435e1 Mon Sep 17 00:00:00 2001 > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Date: Tue, 5 Mar 2013 14:40:52 -0500 > Subject: [PATCH] ACPI: Add fixups for AMD P-state figures. > > In the Linux kernel, these two git commits: > > - f594065faf4f9067c2283a34619fc0714e79a98d > ACPI: Add fixups for AMD P-state figures > - 9855d8ce41a7801548a05d844db2f46c3e810166 > ACPI: Check MSR valid bit before using P-state frequencies > > Try to fix the the issue that "some AMD systems may round the > frequencies in ACPI tables to 100MHz boundaries. We can obtain the real > frequencies from MSRs, so add a quirk to fix these frequencies up > on AMD systems." (from f594065..) > > In discussion (around 9855d8..) "it turned out that indeed real > HW/BIOSes may choose to not set the valid bit and thus mark the > P-state as invalid. So this could be considered a fix for broken > BIOSes." (from 9855d8..) > > which is great for Linux. Unfortunatly the Linux kernel, when > it tries to do the RDMSR under Xen it fails to get the right > value (it gets zero) as Xen traps it and returns zero. Hence > when dom0 uploads the P-states they will be unmodified and > we should take care of updating the frequencies with the right > values. > > I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 > 08/20/2008 where this quirk can be observed (x86 == 0x10, model == 2). > Also on other AMD (x86 == 0x12, A8-3850; x86 = 0x14, AMD E-350) to > make sure the quirk is not applied there. > > CC: stefan.bader@canonical.com > CC: bp@suse.de > CC: boris.ostrovsky@oracle.com > [v1: Indent, #define, and email changes per Boris''s review] > [v2: Redid the x86+model check, updated description] > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/arch/x86/acpi/cpufreq/powernow.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c > index a9b7792..8c96fe0 100644 > --- a/xen/arch/x86/acpi/cpufreq/powernow.c > +++ b/xen/arch/x86/acpi/cpufreq/powernow.c > @@ -147,6 +147,40 @@ static int powernow_cpufreq_target(struct cpufreq_policy *policy, > return 0; > } > > +static void amd_fixup_frequency(struct xen_processor_px *px) > +{ > + u32 hi, lo, fid, did; > + int index = px->control & 0x00000007; > + > + if (!(boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) && > + !(boot_cpu_data.x86 == 0x11)) > + return; > + > + rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi); > + /* > + * MSR C001_0064+: > + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. > + */ > + if (!(hi & (1UL << 31))) > + return; > + > + fid = lo & 0x3f; > + did = (lo >> 6) & 7; > + if (boot_cpu_data.x86 == 0x10) > + px->core_frequency = (100 * (fid + 16)) >> did; > + else > + px->core_frequency = (100 * (fid + 8)) >> did; > +} > + > +static void amd_fixup_freq(struct processor_performance *perf) > +{ > + > + unsigned int i; > + > + for (i = 0; i < perf->state_count; i++) > + amd_fixup_frequency(&perf->states[i]); > + > +} > static int powernow_cpufreq_verify(struct cpufreq_policy *policy) > { > struct acpi_cpufreq_data *data; > @@ -253,6 +287,8 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy *policy) > > policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR; > > + amd_fixup_freq(perf); > + > /* table init */ > for (i = 0; i < perf->state_count && i <= max_hw_pstate; i++) { > if (i > 0 && perf->states[i].core_frequency >>That would look good to me. Using Thunderbird I do not want to make any statement about indentation. -Stefan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk
2013-Mar-07 14:17 UTC
Re: [PATCH] ACPI: Add fixups for AMD P-state figures.
On Thu, Mar 07, 2013 at 09:45:51AM +0100, Stefan Bader wrote:> On 06.03.2013 22:37, Konrad Rzeszutek Wilk wrote: > > On Wed, Mar 06, 2013 at 10:51:12AM -0500, Konrad Rzeszutek Wilk wrote: > >> On Wed, Mar 06, 2013 at 10:48:01AM +0100, Stefan Bader wrote: > >>> On 05.03.2013 22:33, Konrad Rzeszutek Wilk wrote: > >>>> On Tue, Mar 05, 2013 at 03:22:25PM -0500, Boris Ostrovsky wrote: > >>>>> On 03/05/2013 02:45 PM, Konrad Rzeszutek Wilk wrote: > >>>>>> This a copy-n-paste from two Linux git commits: > >>>>>> > >>>>>> - f594065faf4f9067c2283a34619fc0714e79a98d > >>>>>> ACPI: Add fixups for AMD P-state figures > >>>>>> - 9855d8ce41a7801548a05d844db2f46c3e810166 > >>>>>> ACPI: Check MSR valid bit before using P-state frequencies > >>>>>> > >>>>>> The issue is that "some AMD systems may round the frequencies in > >>>>>> ACPI tables to 100MHz boundaries. We canobtain the real > >>>>>> frequencies from MSRs, so add a quirk to fix these frequencies up > >>>>>> on AMD systems." (from f594065..) > >>>>>> > >>>>>> In discussion (around 9855d8..) "it turned out that indeed real > >>>>>> HW/BIOSes may choose to not set the valid bit and thus mark the > >>>>>> P-state as invalid. So this could be considered a fix for broken > >>>>>> BIOSes that also works around the issue on Xen." (from 9855d8..) > >>>>>> > >>>>>> I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 > >>>>>> 08/20/2008 where this quirk can indeed be observed. > >>>>>> > >>>>>> CC: stefan.bader@canonical.com > >>>>>> CC: bp@suse.de > >>>>>> CC: borislav.ostrovsky@oracle.com > >>>>> > >>>>> boris.ostrovsky@oracle.com > >>>> > >>>> Whoops! > >>>> > >>>> Here is an updated version: > > > > > > From b704d4419e9e483922382d2339bd41c245f435e1 Mon Sep 17 00:00:00 2001 > > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Date: Tue, 5 Mar 2013 14:40:52 -0500 > > Subject: [PATCH] ACPI: Add fixups for AMD P-state figures. > > > > In the Linux kernel, these two git commits: > > > > - f594065faf4f9067c2283a34619fc0714e79a98d > > ACPI: Add fixups for AMD P-state figures > > - 9855d8ce41a7801548a05d844db2f46c3e810166 > > ACPI: Check MSR valid bit before using P-state frequencies > > > > Try to fix the the issue that "some AMD systems may round the > > frequencies in ACPI tables to 100MHz boundaries. We can obtain the real > > frequencies from MSRs, so add a quirk to fix these frequencies up > > on AMD systems." (from f594065..) > > > > In discussion (around 9855d8..) "it turned out that indeed real > > HW/BIOSes may choose to not set the valid bit and thus mark the > > P-state as invalid. So this could be considered a fix for broken > > BIOSes." (from 9855d8..) > > > > which is great for Linux. Unfortunatly the Linux kernel, when > > it tries to do the RDMSR under Xen it fails to get the right > > value (it gets zero) as Xen traps it and returns zero. Hence > > when dom0 uploads the P-states they will be unmodified and > > we should take care of updating the frequencies with the right > > values. > > > > I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 > > 08/20/2008 where this quirk can be observed (x86 == 0x10, model == 2). > > Also on other AMD (x86 == 0x12, A8-3850; x86 = 0x14, AMD E-350) to > > make sure the quirk is not applied there. > > > > CC: stefan.bader@canonical.com > > CC: bp@suse.de > > CC: boris.ostrovsky@oracle.com > > [v1: Indent, #define, and email changes per Boris''s review] > > [v2: Redid the x86+model check, updated description] > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > xen/arch/x86/acpi/cpufreq/powernow.c | 36 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > > diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c > > index a9b7792..8c96fe0 100644 > > --- a/xen/arch/x86/acpi/cpufreq/powernow.c > > +++ b/xen/arch/x86/acpi/cpufreq/powernow.c > > @@ -147,6 +147,40 @@ static int powernow_cpufreq_target(struct cpufreq_policy *policy, > > return 0; > > } > > > > +static void amd_fixup_frequency(struct xen_processor_px *px) > > +{ > > + u32 hi, lo, fid, did; > > + int index = px->control & 0x00000007; > > + > > + if (!(boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) && > > + !(boot_cpu_data.x86 == 0x11)) > > + return; > > + > > + rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi); > > + /* > > + * MSR C001_0064+: > > + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. > > + */ > > + if (!(hi & (1UL << 31))) > > + return; > > + > > + fid = lo & 0x3f; > > + did = (lo >> 6) & 7; > > + if (boot_cpu_data.x86 == 0x10) > > + px->core_frequency = (100 * (fid + 16)) >> did; > > + else > > + px->core_frequency = (100 * (fid + 8)) >> did; > > +} > > + > > +static void amd_fixup_freq(struct processor_performance *perf) > > +{ > > + > > + unsigned int i; > > + > > + for (i = 0; i < perf->state_count; i++) > > + amd_fixup_frequency(&perf->states[i]); > > + > > +} > > static int powernow_cpufreq_verify(struct cpufreq_policy *policy) > > { > > struct acpi_cpufreq_data *data; > > @@ -253,6 +287,8 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy *policy) > > > > policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR; > > > > + amd_fixup_freq(perf); > > + > > /* table init */ > > for (i = 0; i < perf->state_count && i <= max_hw_pstate; i++) { > > if (i > 0 && perf->states[i].core_frequency >> > > > That would look good to me. Using Thunderbird I do not want to make any > statement about indentation.I think it came out right (I hope). I presume I can stick ''Acked-by'' on the patch from you?> > -Stefan >
On 07.03.2013 15:17, Konrad Rzeszutek Wilk wrote:> On Thu, Mar 07, 2013 at 09:45:51AM +0100, Stefan Bader wrote: >> On 06.03.2013 22:37, Konrad Rzeszutek Wilk wrote: >>> On Wed, Mar 06, 2013 at 10:51:12AM -0500, Konrad Rzeszutek Wilk wrote: >>>> On Wed, Mar 06, 2013 at 10:48:01AM +0100, Stefan Bader wrote: >>>>> On 05.03.2013 22:33, Konrad Rzeszutek Wilk wrote: >>>>>> On Tue, Mar 05, 2013 at 03:22:25PM -0500, Boris Ostrovsky wrote: >>>>>>> On 03/05/2013 02:45 PM, Konrad Rzeszutek Wilk wrote: >>>>>>>> This a copy-n-paste from two Linux git commits: >>>>>>>> >>>>>>>> - f594065faf4f9067c2283a34619fc0714e79a98d >>>>>>>> ACPI: Add fixups for AMD P-state figures >>>>>>>> - 9855d8ce41a7801548a05d844db2f46c3e810166 >>>>>>>> ACPI: Check MSR valid bit before using P-state frequencies >>>>>>>> >>>>>>>> The issue is that "some AMD systems may round the frequencies in >>>>>>>> ACPI tables to 100MHz boundaries. We canobtain the real >>>>>>>> frequencies from MSRs, so add a quirk to fix these frequencies up >>>>>>>> on AMD systems." (from f594065..) >>>>>>>> >>>>>>>> In discussion (around 9855d8..) "it turned out that indeed real >>>>>>>> HW/BIOSes may choose to not set the valid bit and thus mark the >>>>>>>> P-state as invalid. So this could be considered a fix for broken >>>>>>>> BIOSes that also works around the issue on Xen." (from 9855d8..) >>>>>>>> >>>>>>>> I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 >>>>>>>> 08/20/2008 where this quirk can indeed be observed. >>>>>>>> >>>>>>>> CC: stefan.bader@canonical.com >>>>>>>> CC: bp@suse.de >>>>>>>> CC: borislav.ostrovsky@oracle.com >>>>>>> >>>>>>> boris.ostrovsky@oracle.com >>>>>> >>>>>> Whoops! >>>>>> >>>>>> Here is an updated version: >>> >>> >>> From b704d4419e9e483922382d2339bd41c245f435e1 Mon Sep 17 00:00:00 2001 >>> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>> Date: Tue, 5 Mar 2013 14:40:52 -0500 >>> Subject: [PATCH] ACPI: Add fixups for AMD P-state figures. >>> >>> In the Linux kernel, these two git commits: >>> >>> - f594065faf4f9067c2283a34619fc0714e79a98d >>> ACPI: Add fixups for AMD P-state figures >>> - 9855d8ce41a7801548a05d844db2f46c3e810166 >>> ACPI: Check MSR valid bit before using P-state frequencies >>> >>> Try to fix the the issue that "some AMD systems may round the >>> frequencies in ACPI tables to 100MHz boundaries. We can obtain the real >>> frequencies from MSRs, so add a quirk to fix these frequencies up >>> on AMD systems." (from f594065..) >>> >>> In discussion (around 9855d8..) "it turned out that indeed real >>> HW/BIOSes may choose to not set the valid bit and thus mark the >>> P-state as invalid. So this could be considered a fix for broken >>> BIOSes." (from 9855d8..) >>> >>> which is great for Linux. Unfortunatly the Linux kernel, when >>> it tries to do the RDMSR under Xen it fails to get the right >>> value (it gets zero) as Xen traps it and returns zero. Hence >>> when dom0 uploads the P-states they will be unmodified and >>> we should take care of updating the frequencies with the right >>> values. >>> >>> I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2 >>> 08/20/2008 where this quirk can be observed (x86 == 0x10, model == 2). >>> Also on other AMD (x86 == 0x12, A8-3850; x86 = 0x14, AMD E-350) to >>> make sure the quirk is not applied there. >>> >>> CC: stefan.bader@canonical.com >>> CC: bp@suse.de >>> CC: boris.ostrovsky@oracle.com >>> [v1: Indent, #define, and email changes per Boris''s review] >>> [v2: Redid the x86+model check, updated description] >>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >>> --- >>> xen/arch/x86/acpi/cpufreq/powernow.c | 36 ++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 36 insertions(+) >>> >>> diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c b/xen/arch/x86/acpi/cpufreq/powernow.c >>> index a9b7792..8c96fe0 100644 >>> --- a/xen/arch/x86/acpi/cpufreq/powernow.c >>> +++ b/xen/arch/x86/acpi/cpufreq/powernow.c >>> @@ -147,6 +147,40 @@ static int powernow_cpufreq_target(struct cpufreq_policy *policy, >>> return 0; >>> } >>> >>> +static void amd_fixup_frequency(struct xen_processor_px *px) >>> +{ >>> + u32 hi, lo, fid, did; >>> + int index = px->control & 0x00000007; >>> + >>> + if (!(boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10) && >>> + !(boot_cpu_data.x86 == 0x11)) >>> + return; >>> + >>> + rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi); >>> + /* >>> + * MSR C001_0064+: >>> + * Bit 63: PstateEn. Read-write. If set, the P-state is valid. >>> + */ >>> + if (!(hi & (1UL << 31))) >>> + return; >>> + >>> + fid = lo & 0x3f; >>> + did = (lo >> 6) & 7; >>> + if (boot_cpu_data.x86 == 0x10) >>> + px->core_frequency = (100 * (fid + 16)) >> did; >>> + else >>> + px->core_frequency = (100 * (fid + 8)) >> did; >>> +} >>> + >>> +static void amd_fixup_freq(struct processor_performance *perf) >>> +{ >>> + >>> + unsigned int i; >>> + >>> + for (i = 0; i < perf->state_count; i++) >>> + amd_fixup_frequency(&perf->states[i]); >>> + >>> +} >>> static int powernow_cpufreq_verify(struct cpufreq_policy *policy) >>> { >>> struct acpi_cpufreq_data *data; >>> @@ -253,6 +287,8 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy *policy) >>> >>> policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR; >>> >>> + amd_fixup_freq(perf); >>> + >>> /* table init */ >>> for (i = 0; i < perf->state_count && i <= max_hw_pstate; i++) { >>> if (i > 0 && perf->states[i].core_frequency >>>> >> >> That would look good to me. Using Thunderbird I do not want to make any >> statement about indentation. > > I think it came out right (I hope). I presume I can stick ''Acked-by'' on the > patch from you? >Sure. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Konrad Rzeszutek Wilk
2013-Mar-07 18:49 UTC
[PATCH] ACPI: Add fixups for AMD P-state figures.
In the Linux kernel, these two git commits:
- f594065faf4f9067c2283a34619fc0714e79a98d
ACPI: Add fixups for AMD P-state figures
- 9855d8ce41a7801548a05d844db2f46c3e810166
ACPI: Check MSR valid bit before using P-state frequencies
Try to fix the the issue that "some AMD systems may round the
frequencies in ACPI tables to 100MHz boundaries. We can obtain the real
frequencies from MSRs, so add a quirk to fix these frequencies up
on AMD systems." (from f594065..)
In discussion (around 9855d8..) "it turned out that indeed real
HW/BIOSes may choose to not set the valid bit and thus mark the
P-state as invalid. So this could be considered a fix for broken
BIOSes." (from 9855d8..)
which is great for Linux. Unfortunatly the Linux kernel, when
it tries to do the RDMSR under Xen it fails to get the right
value (it gets zero) as Xen traps it and returns zero. Hence
when dom0 uploads the P-states they will be unmodified and
we should take care of updating the frequencies with the right
values.
I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2
08/20/2008 where this quirk can be observed (x86 == 0x10, model == 2).
Also on other AMD (x86 == 0x12, A8-3850; x86 = 0x14, AMD E-350) to
make sure the quirk is not applied there.
Acked-by: stefan.bader@canonical.com
CC: bp@suse.de
CC: boris.ostrovsky@oracle.com
[v1: Indent, #define, and email changes per Boris''s review]
[v2: Redid the x86+model check, updated description]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
xen/arch/x86/acpi/cpufreq/powernow.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/xen/arch/x86/acpi/cpufreq/powernow.c
b/xen/arch/x86/acpi/cpufreq/powernow.c
index a9b7792..8c96fe0 100644
--- a/xen/arch/x86/acpi/cpufreq/powernow.c
+++ b/xen/arch/x86/acpi/cpufreq/powernow.c
@@ -147,6 +147,40 @@ static int powernow_cpufreq_target(struct cpufreq_policy
*policy,
return 0;
}
+static void amd_fixup_frequency(struct xen_processor_px *px)
+{
+ u32 hi, lo, fid, did;
+ int index = px->control & 0x00000007;
+
+ if (!(boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10)
&&
+ !(boot_cpu_data.x86 == 0x11))
+ return;
+
+ rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi);
+ /*
+ * MSR C001_0064+:
+ * Bit 63: PstateEn. Read-write. If set, the P-state is valid.
+ */
+ if (!(hi & (1UL << 31)))
+ return;
+
+ fid = lo & 0x3f;
+ did = (lo >> 6) & 7;
+ if (boot_cpu_data.x86 == 0x10)
+ px->core_frequency = (100 * (fid + 16)) >> did;
+ else
+ px->core_frequency = (100 * (fid + 8)) >> did;
+}
+
+static void amd_fixup_freq(struct processor_performance *perf)
+{
+
+ unsigned int i;
+
+ for (i = 0; i < perf->state_count; i++)
+ amd_fixup_frequency(&perf->states[i]);
+
+}
static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
{
struct acpi_cpufreq_data *data;
@@ -253,6 +287,8 @@ static int powernow_cpufreq_cpu_init(struct cpufreq_policy
*policy)
policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR;
+ amd_fixup_freq(perf);
+
/* table init */
for (i = 0; i < perf->state_count && i <= max_hw_pstate;
i++) {
if (i > 0 && perf->states[i].core_frequency >--
1.8.0.2
In the Linux kernel, these two git commits:
- f594065faf4f9067c2283a34619fc0714e79a98d
ACPI: Add fixups for AMD P-state figures
- 9855d8ce41a7801548a05d844db2f46c3e810166
ACPI: Check MSR valid bit before using P-state frequencies
Try to fix the the issue that "some AMD systems may round the
frequencies in ACPI tables to 100MHz boundaries. We can obtain the real
frequencies from MSRs, so add a quirk to fix these frequencies up
on AMD systems." (from f594065..)
In discussion (around 9855d8..) "it turned out that indeed real
HW/BIOSes may choose to not set the valid bit and thus mark the
P-state as invalid. So this could be considered a fix for broken
BIOSes." (from 9855d8..)
which is great for Linux. Unfortunatly the Linux kernel, when
it tries to do the RDMSR under Xen it fails to get the right
value (it gets zero) as Xen traps it and returns zero. Hence
when dom0 uploads the P-states they will be unmodified and
we should take care of updating the frequencies with the right
values.
I''ve tested it under Dell Inc. PowerEdge T105 /0RR825, BIOS 1.3.2
08/20/2008 where this quirk can be observed (x86 == 0x10, model == 2).
Also on other AMD (x86 == 0x12, A8-3850; x86 = 0x14, AMD E-350) to
make sure the quirk is not applied there.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: stefan.bader@canonical.com
Do the MSR access here (and while at it, also the one reading
MSR_PSTATE_CUR_LIMIT) on the target CPU, and bound the loop over
amd_fixup_frequency() by max_hw_pstate (matching the one in
powernow_cpufreq_cpu_init()).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- 2013-02-21.orig/xen/arch/x86/acpi/cpufreq/powernow.c 2012-09-14
13:26:34.000000000 +0200
+++ 2013-02-21/xen/arch/x86/acpi/cpufreq/powernow.c 2013-03-08
10:40:25.000000000 +0100
@@ -147,6 +147,51 @@ static int powernow_cpufreq_target(struc
return 0;
}
+static void amd_fixup_frequency(struct xen_processor_px *px)
+{
+ u32 hi, lo, fid, did;
+ int index = px->control & 0x00000007;
+ const struct cpuinfo_x86 *c = ¤t_cpu_data;
+
+ if ((c->x86 != 0x10 || c->x86_model >= 10) && c->x86 !=
0x11)
+ return;
+
+ rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi);
+ /*
+ * MSR C001_0064+:
+ * Bit 63: PstateEn. Read-write. If set, the P-state is valid.
+ */
+ if (!(hi & (1U << 31)))
+ return;
+
+ fid = lo & 0x3f;
+ did = (lo >> 6) & 7;
+ if (c->x86 == 0x10)
+ px->core_frequency = (100 * (fid + 16)) >> did;
+ else
+ px->core_frequency = (100 * (fid + 8)) >> did;
+}
+
+struct amd_cpu_data {
+ struct processor_performance *perf;
+ u32 max_hw_pstate;
+};
+
+static void get_cpu_data(void *arg)
+{
+ struct amd_cpu_data *data = arg;
+ struct processor_performance *perf = data->perf;
+ uint64_t msr_content;
+ unsigned int i;
+
+ rdmsrl(MSR_PSTATE_CUR_LIMIT, msr_content);
+ data->max_hw_pstate = (msr_content & HW_PSTATE_MAX_MASK) >>
+ HW_PSTATE_MAX_SHIFT;
+
+ for (i = 0; i < perf->state_count && i <=
data->max_hw_pstate; i++)
+ amd_fixup_frequency(&perf->states[i]);
+}
+
static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
{
struct acpi_cpufreq_data *data;
@@ -193,8 +238,7 @@ static int powernow_cpufreq_cpu_init(str
struct acpi_cpufreq_data *data;
unsigned int result = 0;
struct processor_performance *perf;
- u32 max_hw_pstate;
- uint64_t msr_content;
+ struct amd_cpu_data info;
struct cpuinfo_x86 *c = &cpu_data[policy->cpu];
data = xzalloc(struct acpi_cpufreq_data);
@@ -205,7 +249,7 @@ static int powernow_cpufreq_cpu_init(str
data->acpi_data = &processor_pminfo[cpu]->perf;
- perf = data->acpi_data;
+ info.perf = perf = data->acpi_data;
policy->shared_type = perf->shared_type;
if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL ||
@@ -227,8 +271,6 @@ static int powernow_cpufreq_cpu_init(str
result = -ENODEV;
goto err_unreg;
}
- rdmsrl(MSR_PSTATE_CUR_LIMIT, msr_content);
- max_hw_pstate = (msr_content & HW_PSTATE_MAX_MASK) >>
HW_PSTATE_MAX_SHIFT;
if (perf->control_register.space_id !=
perf->status_register.space_id) {
result = -ENODEV;
@@ -253,8 +295,10 @@ static int powernow_cpufreq_cpu_init(str
policy->governor = cpufreq_opt_governor ? : CPUFREQ_DEFAULT_GOVERNOR;
+ on_selected_cpus(cpumask_of(cpu), get_cpu_data, &info, 1);
+
/* table init */
- for (i = 0; i < perf->state_count && i <= max_hw_pstate;
i++) {
+ for (i = 0; i < perf->state_count && i <=
info.max_hw_pstate; i++) {
if (i > 0 && perf->states[i].core_frequency >
data->freq_table[valid_states-1].frequency / 1000)
continue;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel