Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Xen: Devel

Workings/effectiveness of the xen-acpi-processor driver

 

 

First page Previous page 1 2 Next page Last page  View All Xen devel RSS feed   Index | Next | Previous | View Threaded


konrad at darnok

May 3, 2012, 8:46 AM

Post #26 of 33 (227 views)
Permalink
Re: Workings/effectiveness of the xen-acpi-processor driver [In reply to]

On Thu, May 03, 2012 at 04:47:46PM +0200, Stefan Bader wrote:
> On 03.05.2012 14:58, Stefan Bader wrote:
>
> >>>>> So this shoudl solve the problem for the bootup processor.
> >>>>>>
> >>>>>> -boris
> >>>>>>
> >>>>>>
> >>>>>>> + };
> >>>>>>> + 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;
> >>>>>>> }
> >>>>>>>
> >>>>>>> static void xen_apic_write(u32 reg, u32 val)
> >
> > I added debugging to all exit paths that could return 0 (which is what the
> > boot_cpu_physical_apicid is set to with that patch. Which would only leave the
> > case of the HV call returning the wrong value somehow...
> >
> 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.
> }
>
>



> _______________________________________________
> Xen-devel mailing list
> Xen-devel [at] lists
> http://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


konrad.wilk at oracle

May 3, 2012, 9:14 AM

Post #27 of 33 (223 views)
Permalink
Re: Workings/effectiveness of the xen-acpi-processor driver [In reply to]

On Wed, May 02, 2012 at 06:09:26PM -0400, Boris Ostrovsky wrote:
> On 05/02/2012 05:41 PM, Konrad Rzeszutek Wilk wrote:
> >On Wed, May 02, 2012 at 02:31:07PM -0700, Boris Ostrovsky wrote:
> >>On 05/02/2012 01:14 PM, Konrad Rzeszutek Wilk wrote:
> >>>On Wed, May 02, 2012 at 01:06:34PM -0400, Boris Ostrovsky wrote:
> >>>>On 05/02/2012 12:08 PM, Konrad Rzeszutek Wilk wrote:
> >>>>>diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> >>>>>index a8f8844..d816448 100644
> >>>>>--- a/arch/x86/xen/enlighten.c
> >>>>>+++ b/arch/x86/xen/enlighten.c
> >>>>>@@ -811,7 +811,29 @@ static void xen_io_delay(void)
> >>>>> #ifdef CONFIG_X86_LOCAL_APIC
> >>>>> 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,
> >>>>
> >>>>
> >>>>Is this always zero? This will probably solve the current problem
> >>>
> >>>Its a CPU number (not tied in to APIC or ACPI IDs).
> >>
> >>Why not use CPU number instead of zero here?
> >
> >The issue was only with the bootup CPU - so was using the Xen's
> >bootup CPU number - which is zero (as is Linux's).
>
> I agree that for this particular problem this may be sufficient.
>
> My concern is that in the future someone may decide to use
> apic_read(APIC_ID) or read_apic_id() for some other purpose and they
> won't get expected result (i.e. on all CPUs they will get the same
> answer).

Good point. Let's get this particular bug fixed for v3.5, and then will
do a more comprehensive fix for v3.6.

_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


boris.ostrovsky at amd

May 3, 2012, 10:02 AM

Post #28 of 33 (224 views)
Permalink
Re: Workings/effectiveness of the xen-acpi-processor driver [In reply to]

On 05/03/2012 11:46 AM, Konrad Rzeszutek Wilk wrote:
> On Thu, May 03, 2012 at 04:47:46PM +0200, Stefan Bader wrote:
>> On 03.05.2012 14:58, Stefan Bader wrote:
>>
>>>>>>> So this shoudl solve the problem for the bootup processor.
>>>>>>>>
>>>>>>>> -boris
>>>>>>>>
>>>>>>>>
>>>>>>>>> + };
>>>>>>>>> + 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;


return (op.u.pcpu_info.apic_id << 24);

indeed fixes the problem.

-boris


>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> static void xen_apic_write(u32 reg, u32 val)
>>>
>>> I added debugging to all exit paths that could return 0 (which is what the
>>> boot_cpu_physical_apicid is set to with that patch. Which would only leave the
>>> case of the HV call returning the wrong value somehow...
>>>
>> 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.
>> }
>>
>>
>
>
>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel [at] lists
>> http://lists.xen.org/xen-devel
>
>



_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


konrad.wilk at oracle

May 3, 2012, 10:08 AM

Post #29 of 33 (224 views)
Permalink
Re: Workings/effectiveness of the xen-acpi-processor driver [In reply to]

> > 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.

commit 4bb450ea9dca1b8d845f1b53ab6476615a32badf
Author: Konrad Rzeszutek Wilk <konrad.wilk [at] oracle>
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 [at] canonical>
Suggested-by: Boris Ostrovsky <boris.ostrovsky [at] amd>
[v2: Bit-shift APIC ID by 24 bits]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk [at] oracle>

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 [at] lists
http://lists.xen.org/xen-devel


stefan.bader at canonical

May 4, 2012, 1:00 AM

Post #30 of 33 (223 views)
Permalink
Re: Workings/effectiveness of the xen-acpi-processor driver [In reply to]

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 [at] oracle>
> 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 [at] canonical>
> Suggested-by: Boris Ostrovsky <boris.ostrovsky [at] amd>
> [v2: Bit-shift APIC ID by 24 bits]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk [at] oracle>
Tested-by: Stefan Bader <stefan.bader [at] canonical>
>
> 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 [at] lists
> http://lists.xen.org/xen-devel
Attachments: dmesg.i7.txt (72.6 KB)
  dmesg.opt.txt (65.0 KB)
  signature.asc (0.88 KB)


pasik at iki

May 6, 2012, 8:23 AM

Post #31 of 33 (216 views)
Permalink
Re: Workings/effectiveness of the xen-acpi-processor driver [In reply to]

On Thu, Apr 26, 2012 at 01:04:26PM -0400, Konrad Rzeszutek Wilk wrote:
> > >>
> > >> This I would take as C3 and C6 really not being used and the frequency scaling
> > >
> > > To go in deeper modes there is also a need to backport a Xen unstable
> > > hypercall which will allow the kernel to detect the other states besides
> > > C0-C2.
> > >
> > > "XEN_SET_PDC query was implemented in c/s 23783:
> > > "ACPI: add _PDC input override mechanism".
> > >
> >
> > I see. There is a kernel patch about enabling MWAIT that refers to that...
>
> Yeah, I should see about back-porting it in Xen 4.1..
>

Should this patch be backported and merged to xen-4.1-testing.hg for Xen 4.1.3 release ?

Thanks,

-- Pasi



_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


konrad.wilk at oracle

May 7, 2012, 10:33 AM

Post #32 of 33 (215 views)
Permalink
Re: Workings/effectiveness of the xen-acpi-processor driver [In reply to]

On Sun, May 06, 2012 at 06:23:41PM +0300, Pasi Kärkkäinen wrote:
> On Thu, Apr 26, 2012 at 01:04:26PM -0400, Konrad Rzeszutek Wilk wrote:
> > > >>
> > > >> This I would take as C3 and C6 really not being used and the frequency scaling
> > > >
> > > > To go in deeper modes there is also a need to backport a Xen unstable
> > > > hypercall which will allow the kernel to detect the other states besides
> > > > C0-C2.
> > > >
> > > > "XEN_SET_PDC query was implemented in c/s 23783:
> > > > "ACPI: add _PDC input override mechanism".
> > > >
> > >
> > > I see. There is a kernel patch about enabling MWAIT that refers to that...
> >
> > Yeah, I should see about back-porting it in Xen 4.1..
> >
>
> Should this patch be backported and merged to xen-4.1-testing.hg for Xen 4.1.3 release ?

It is a new feature so no. Which does mean that the MWAIT states won't be uploaded
to the hypervisor. But the legacy ones (so the ones that are in the ACPI _CST)
are still uploaded. And TurboMode kicks in without the MWAIT states.


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel


pasik at iki

May 7, 2012, 10:44 AM

Post #33 of 33 (222 views)
Permalink
Re: Workings/effectiveness of the xen-acpi-processor driver [In reply to]

On Mon, May 07, 2012 at 01:33:06PM -0400, Konrad Rzeszutek Wilk wrote:
> On Sun, May 06, 2012 at 06:23:41PM +0300, Pasi Kärkkäinen wrote:
> > On Thu, Apr 26, 2012 at 01:04:26PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > >>
> > > > >> This I would take as C3 and C6 really not being used and the frequency scaling
> > > > >
> > > > > To go in deeper modes there is also a need to backport a Xen unstable
> > > > > hypercall which will allow the kernel to detect the other states besides
> > > > > C0-C2.
> > > > >
> > > > > "XEN_SET_PDC query was implemented in c/s 23783:
> > > > > "ACPI: add _PDC input override mechanism".
> > > > >
> > > >
> > > > I see. There is a kernel patch about enabling MWAIT that refers to that...
> > >
> > > Yeah, I should see about back-porting it in Xen 4.1..
> > >
> >
> > Should this patch be backported and merged to xen-4.1-testing.hg for Xen 4.1.3 release ?
>
> It is a new feature so no. Which does mean that the MWAIT states won't be uploaded
> to the hypervisor. But the legacy ones (so the ones that are in the ACPI _CST)
> are still uploaded. And TurboMode kicks in without the MWAIT states.
>

Ah, thanks for clearing that up.

-- Pasi


_______________________________________________
Xen-devel mailing list
Xen-devel [at] lists
http://lists.xen.org/xen-devel

First page Previous page 1 2 Next page Last page  View All Xen devel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.