Re: [Xen-devel] Workings/effectiveness of the xen-acpi-processor driver

[Stefan Bader <stefan.bader@xxxxxxxxxxxxx>]

On 03.05.2012 19:08, Konrad Rzeszutek Wilk wrote:
>>> Hmmm, so xen_apic_read is still correct...
>>>
>>> [    0.000000] ACPI: Local APIC address 0xfee00000
>>> [    0.000000] xxx xen_apic_read(20)
>>> [    0.000000] xxx xen_apic_read -> 10
>>> [    0.000000] boot_cpu_physical_apicid = 0
>>> [    0.000000] xxx xen_apic_read(30)
>>> [    0.000000] +- apic version = 10
>>>
>>> there seems to be a slightly strange tweak (at least for me) in 
>>> read_apic_id...
>>>
>>> static inline unsigned int read_apic_id(void)
>>> {
>>>         unsigned int reg;
>>>
>>>         reg = apic_read(APIC_ID); // calls apic->read(reg)
>>>
>>>         return apic->get_apic_id(reg);
>>
>> Duh!! Let me spin out a new patch that will do this.
> 
> Meaning bit-shift it. We ended up doing 10 >> 24 (get_apic_id does that)
> which results in zero. So lets be a bit more cautious and over-write
> the get_apic_id and set_apic_id and as well do the proper bit-shifting.
> 

I can confirm that with this patch applied I can load the xen-acpi-processor
driver and see P-states changing in xenpm. No BIOS bug messages.
Also tested on the i7 and it does still work. I am attaching the dmesg output of
both runs. On the i7 the xen apic functions are called in two places and for
cpu#0 only. On AMD, I see additional call later and for all cpus while enabling
them. apic->read bails out, but apic->get_apic_id returns 0 (though I could see
no immediate problem from that). Maybe that info is helpful for the next stage.

-Stefan

> commit 4bb450ea9dca1b8d845f1b53ab6476615a32badf
> Author: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Date:   Wed May 2 15:04:51 2012 -0400
> 
>     xen/apic: Return the APIC ID (and version) for CPU 0.
>     
>     On x86_64 on AMD machines where the first APIC_ID is not zero, we get:
>     
>     ACPI: LAPIC (acpi_id[0x01] lapic_id[0x10] enabled)
>     BIOS bug: APIC version is 0 for CPU 1/0x10, fixing up to 0x10
>     BIOS bug: APIC version mismatch, boot CPU: 0, CPU 1: version 10
>     
>     which means that when the ACPI processor driver loads and
>     tries to parse the _Pxx states it fails to do as, as it
>     ends up calling acpi_get_cpuid which does this:
>     
>     for_each_possible_cpu(i) {
>             if (cpu_physical_id(i) == apic_id)
>                     return i;
>     }
>     
>     And the bootup CPU, has not been found so it fails and returns -1
>     for the first CPU - which then subsequently in the loop that
>     "acpi_processor_get_info" does results in returning an error, which
>     means that "acpi_processor_add" failing and per_cpu(processor)
>     is never set (and is NULL).
>     
>     That means that when xen-acpi-processor tries to load (much much
>     later on) and parse the P-states it gets -ENODEV from
>     acpi_processor_register_performance() (which tries to read
>     the per_cpu(processor)) and fails to parse the data.
>     
>     Reported-by:  Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
>     Suggested-by:  Boris Ostrovsky <boris.ostrovsky@xxxxxxx>
>     [v2: Bit-shift APIC ID by 24 bits]
>     Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Tested-by: Stefan Bader <stefan.bader@xxxxxxxxxxxxx>
> 
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index a8f8844..63d6c22 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -53,6 +53,9 @@
>  #include <asm/processor.h>
>  #include <asm/proto.h>
>  #include <asm/msr-index.h>
> +#ifdef CONFIG_NUMA
> +#include <asm/numa.h>
> +#endif
>  #include <asm/traps.h>
>  #include <asm/setup.h>
>  #include <asm/desc.h>
> @@ -809,9 +812,40 @@ static void xen_io_delay(void)
>  }
>  
>  #ifdef CONFIG_X86_LOCAL_APIC
> +static unsigned long xen_set_apic_id(unsigned int x)
> +{
> +     WARN_ON(1);
> +     return x;
> +}
> +static unsigned int xen_get_apic_id(unsigned long x)
> +{
> +     return (((x)>>24) & 0xFFu);
> +}
>  static u32 xen_apic_read(u32 reg)
>  {
> -     return 0;
> +     struct xen_platform_op op = {
> +             .cmd = XENPF_get_cpuinfo,
> +             .interface_version = XENPF_INTERFACE_VERSION,
> +             .u.pcpu_info.xen_cpuid = 0,
> +     };
> +     int ret = 0;
> +
> +     /* Shouldn't need this as APIC is turned off for PV, and we only
> +      * get called on the bootup processor. But just in case. */
> +     if (!xen_initial_domain() || smp_processor_id())
> +             return 0;
> +
> +     if (reg == APIC_LVR)
> +             return 0x10;
> +
> +     if (reg != APIC_ID)
> +             return 0;
> +
> +     ret = HYPERVISOR_dom0_op(&op);
> +     if (ret)
> +             return 0;
> +
> +     return op.u.pcpu_info.apic_id << 24;
>  }
>  
>  static void xen_apic_write(u32 reg, u32 val)
> @@ -849,6 +883,8 @@ static void set_xen_basic_apic_ops(void)
>       apic->icr_write = xen_apic_icr_write;
>       apic->wait_icr_idle = xen_apic_wait_icr_idle;
>       apic->safe_wait_icr_idle = xen_safe_apic_wait_icr_idle;
> +     apic->set_apic_id = xen_set_apic_id;
> +     apic->get_apic_id = xen_get_apic_id;
>  }
>  
>  #endif
> @@ -1294,6 +1330,9 @@ asmlinkage void __init xen_start_kernel(void)
>        */
>       acpi_numa = -1;
>  #endif
> +#if defined(CONFIG_NUMA) && defined(CONFIG_X86_32)
> +     numa_off = 1;
> +#endif
>  
>       pgd = (pgd_t *)xen_start_info->pt_base;
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel

Attachment: dmesg.i7.txt
Description: Text document

Attachment: dmesg.opt.txt
Description: Text document

Attachment: signature.asc
Description: OpenPGP digital signature

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