Re: [Xen-devel] [PATCH] ACPI: Add fixups for AMD P-state figures.

[Stefan Bader <stefan.bader@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxx
>>> CC: bp@xxxxxxx
>>> CC: borislav.ostrovsky@xxxxxxxxxx
>>
>> boris.ostrovsky@xxxxxxxxxx
> 
> Whoops!
> 
> Here is an updated version:
> 
> From 3b7584f0c3c91d073bd760a038d0091b3bf5a19b Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 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@xxxxxxxxxxxxx
> CC: bp@xxxxxxx
> CC: boris.ostrovsky@xxxxxxxxxx
> [v1: Indent, #define, and email changes per Boris's review]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  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 >=
>

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel