[PATCH][retry 2][2/2] new platform hypervisor call to get APICIDs

[PATCH][retry 2][2/2] new platform hypervisor call to get APICIDs

[Mark Langsdorf]

Some AMD machines have APIC IDs that not equal to CPU IDs.  In
the default Xen configuration, ACPI calls on these machines
can get confused.  This shows up most noticeably when running
AMD PowerNow!.  The only solution is for dom0 to get the
hypervisor's cpuid to apicid table when needed (ie, when dom0
vcpus are pinned).

Make dom0 call a new platform hypercall that returns the
hypervisor's cpuid to apicid table.  The decision logic
(dom0_vcpus_pinned) is currently hard-coded but should be
passed from the hypervisor.  Keir wrote that he would take
care of that when he suggested this solution.

I have tested this on my 4p/16 core machine and it works.  I
would appreciate testing on other boxes.

-Mark Langsdorf
Operating System Research Center
AMD

Signed-off-by: Mark Langsdorf <mark.langsdorf@amd.com>

diff -r 1cf7ba68d855 drivers/xen/core/smpboot.c
--- a/drivers/xen/core/smpboot.c	Mon Mar 03 13:36:57 2008 +0000
+++ b/drivers/xen/core/smpboot.c	Mon Mar 03 14:40:26 2008 -0600
@@ -254,21 +254,39 @@ static void __cpuinit cpu_initialize_con
 	spin_unlock(&ctxt_lock);
 }
 
+int dom0_vcpus_pinned = 1;
 void __init smp_prepare_cpus(unsigned int max_cpus)
 {
 	unsigned int cpu;
 	struct task_struct *idle;
+	int apicid;
+	u8 pinned_apicids[NR_CPUS];
 #ifdef __x86_64__
 	struct desc_ptr *gdt_descr;
 #else
 	struct Xgt_desc_struct *gdt_descr;
 #endif
 
-	boot_cpu_data.apicid = 0;
+	if (dom0_vcpus_pinned) {
+		struct xen_platform_op op;
+		op.cmd = XENPF_getapicids;
+		set_xen_guest_handle(op.u.getapicids.apicids, pinned_apicids);
+		op.u.getapicids.nr_cpus = NR_CPUS;
+		if (HYPERVISOR_platform_op(&op)) {
+			/* failed to get valid data from hypervisor */
+			printk(KERN_ERR "failed to get valid APIC ID information\n");
+			dom0_vcpus_pinned = 0;
+			apicid = 0;
+		}
+		else
+			apicid = pinned_apicids[0];
+	} else
+		apicid = 0;
+	boot_cpu_data.apicid = apicid;
 	cpu_data[0] = boot_cpu_data;
 
-	cpu_2_logical_apicid[0] = 0;
-	x86_cpu_to_apicid[0] = 0;
+	cpu_2_logical_apicid[0] = apicid;
+	x86_cpu_to_apicid[0] = apicid;
 
 	current_thread_info()->cpu = 0;
 
@@ -312,11 +330,15 @@ void __init smp_prepare_cpus(unsigned in
 			(void *)gdt_descr->address,
 			XENFEAT_writable_descriptor_tables);
 
+		if (dom0_vcpus_pinned)
+			apicid = pinned_apicids[cpu];
+		else
+			apicid = cpu;
 		cpu_data[cpu] = boot_cpu_data;
-		cpu_data[cpu].apicid = cpu;
-
-		cpu_2_logical_apicid[cpu] = cpu;
-		x86_cpu_to_apicid[cpu] = cpu;
+		cpu_data[cpu].apicid = apicid;
+
+		cpu_2_logical_apicid[cpu] = apicid;
+		x86_cpu_to_apicid[cpu] = apicid;
 
 		idle = fork_idle(cpu);
 		if (IS_ERR(idle))
diff -r 1cf7ba68d855 include/xen/interface/platform.h
--- a/include/xen/interface/platform.h	Mon Mar 03 13:36:57 2008 +0000
+++ b/include/xen/interface/platform.h	Mon Mar 03 14:40:26 2008 -0600
@@ -198,6 +198,16 @@ struct xenpf_getidletime {
 };
 typedef struct xenpf_getidletime xenpf_getidletime_t;
 DEFINE_XEN_GUEST_HANDLE(xenpf_getidletime_t);
+
+#define XENPF_getapicids	54
+struct xenpf_getapicids {
+    /* IN variables */
+    /* size of apicid table */
+    uint32_t nr_cpus;
+    /* OUT variables */
+    /* Table of cpu to apicids */
+    XEN_GUEST_HANDLE(uint8) apicids;
+};
 
 struct xen_platform_op {
     uint32_t cmd;
@@ -213,6 +223,7 @@ struct xen_platform_op {
         struct xenpf_enter_acpi_sleep  enter_acpi_sleep;
         struct xenpf_change_freq       change_freq;
         struct xenpf_getidletime       getidletime;
+	struct xenpf_getapicids	       getapicids;
         uint8_t                        pad[128];
     } u;
 };

Re: [PATCH][retry 2][2/2] new platform hypervisor call to get APICIDs

[Chris Lalancette]

Mark Langsdorf wrote:
> Some AMD machines have APIC IDs that not equal to CPU IDs.  In
> the default Xen configuration, ACPI calls on these machines
> can get confused.  This shows up most noticeably when running
> AMD PowerNow!.  The only solution is for dom0 to get the
> hypervisor's cpuid to apicid table when needed (ie, when dom0
> vcpus are pinned).
> 
> Make dom0 call a new platform hypercall that returns the
> hypervisor's cpuid to apicid table.  The decision logic
> (dom0_vcpus_pinned) is currently hard-coded but should be
> passed from the hypervisor.  Keir wrote that he would take
> care of that when he suggested this solution.
> 
> I have tested this on my 4p/16 core machine and it works.  I
> would appreciate testing on other boxes.

This patch is much better, although unfortunately the dom0_vcpus_pinned change
doesn't look like it will work as-is.  That is, the only failure case I see on
the hypervisor side is if you fail copy_to_guest, so that means on the dom0 side
the only way you think the dom0 vcpus are unpinned is if you have a major
failure.    It seems you really need to worry about: a)  making the platform op
call on an HV that doesn't support it, b) making the platform op call, and
somehow returning that dom0 is unpinned, c) making the platform op call and
returning that the dom0 is in fact pinned.

Generally, I'm not against the way you've done it here, but originally I thought
you would re-enable the code in dom0 mpparse-xen.c and then just have a
hypercall to determine whether dom0 had the vcpus pinned or not.  The advantage
to that way is that if there is ever any other information needed from the MP
tables, we will just have it available in the dom0.  If we don't think it is
likely we will need additional information from the MP tables, then it is sort
of a wash which way you do it.

Chris Lalancette

Re: [PATCH][retry 2][2/2] new platform hypervisor call to get APICIDs

[Jan Beulich]

In addition to Keir's comment, this

>--- a/drivers/xen/core/smpboot.c	Mon Mar 03 13:36:57 2008 +0000
>+++ b/drivers/xen/core/smpboot.c	Mon Mar 03 14:40:26 2008 -0600
>@@ -254,21 +254,39 @@ static void __cpuinit cpu_initialize_con
> 	spin_unlock(&ctxt_lock);
> }
> 
>+int dom0_vcpus_pinned = 1;
> void __init smp_prepare_cpus(unsigned int max_cpus)
> {
> 	unsigned int cpu;
> 	struct task_struct *idle;
>+	int apicid;
>+	u8 pinned_apicids[NR_CPUS];

is introducing a latent bug. If you follow Keir's suggestion, it'll be gone.
If there's a reason to keep it obtaining all IDs at once, you'll need to
convert this to a static, __initdata array.

The other thing is that you absolutely shouldn't be limiting the APIC IDs
at the Xen interface to 8 bits. With the x2APIC, they'll not be bounded
to that smaller range anymore.

Finally, I'm worried about the way you handle dom0_vcpus_pinned:
By its name, I would assume that if I (later, for whatever reason) use
it elsewhere, I can be certain it means what it says. However, at
present, the hypercall succeeding does not imply that VCPUs are
indeed pinned. Further, under !is_initial_xen_domain() you shouldn't
set it, nor should you have a reason to even attempt the hypercall.

Jan

Re: [PATCH][retry 2][2/2] new platform hypervisor call to get APICIDs

[Keir Fraser]

On 4/3/08 08:59, "Jan Beulich" <jbeulich@novell.com> wrote:

> Finally, I'm worried about the way you handle dom0_vcpus_pinned:
> By its name, I would assume that if I (later, for whatever reason) use
> it elsewhere, I can be certain it means what it says. However, at
> present, the hypercall succeeding does not imply that VCPUs are
> indeed pinned. Further, under !is_initial_xen_domain() you shouldn't
> set it, nor should you have a reason to even attempt the hypercall.

I'm happy to clean up the dom0_vcpus_pinned side of this patch myself.
Anyway, if we make it a singleton-apicid vcpu_op, which fails on anything
other than a pinned dom0, I think we're good. I like the idea of simply
plucking the APICID from CPUID, but it does munge the code and I'm a bit
worried that we couldn't get the APICID until rather later than native Linux
(probably after the AP has booted but before it 'calls in').

 -- Keir

Re: [PATCH][retry 2][2/2] new platform hypervisor call to get APICIDs

[Keir Fraser]

On 4/3/08 09:31, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> Finally, I'm worried about the way you handle dom0_vcpus_pinned:
>> By its name, I would assume that if I (later, for whatever reason) use
>> it elsewhere, I can be certain it means what it says. However, at
>> present, the hypercall succeeding does not imply that VCPUs are
>> indeed pinned. Further, under !is_initial_xen_domain() you shouldn't
>> set it, nor should you have a reason to even attempt the hypercall.
> 
> I'm happy to clean up the dom0_vcpus_pinned side of this patch myself.
> Anyway, if we make it a singleton-apicid vcpu_op, which fails on anything
> other than a pinned dom0, I think we're good. I like the idea of simply
> plucking the APICID from CPUID, but it does munge the code and I'm a bit
> worried that we couldn't get the APICID until rather later than native Linux
> (probably after the AP has booted but before it 'calls in').

Also there is no particular reason not to have this vcpu_op return the
acpiid as well. Call it something like VCPUOP_phys_id, returns an
arch-specific uint64_t phys_id. On x86 we could have 8-bit apicid plus 8-bit
acpiid packed into that uint64_t, with the other 48 bits being MBZ for
future expansion (e.g., x2apic extra id bits should we ever care). Either id
can be 0xff indicating 'not available'.

 -- Keir

RE: [PATCH][retry 2][2/2] new platform hypervisor call to get APICIDs

[Langsdorf, Mark]

> > I have tested this on my 4p/16 core machine and it works.  I
> > would appreciate testing on other boxes.
> 
> This patch is much better, although unfortunately the 
> dom0_vcpus_pinned change doesn't look like it will work as-is. 

I have confidence that Keir will fix that up before he
commits the code. =)

> That is, the only failure case I see on
> the hypervisor side is if you fail copy_to_guest, so that 
> means on the dom0 side the only way you think the dom0
> vcpus are unpinned is if you have a major failure.   

Well, it was more "if there is a major failure, the
information in this array is junk anyway, so assume
we're unpinned and accept there may be errors down
the line."

> It seems you really need to worry about: a) making the
> platform op call on an HV that doesn't support it,

The case statement will handle that already.  I tested
that during my development.

> b) making the platform op call, and somehow returning
> that dom0 is unpinned,

I can add that.  Thanks.

> c) making the platform op call and
> returning that the dom0 is in fact pinned.

That's not an error.  I'm not sure what you mean.
 
> Generally, I'm not against the way you've done it here, but 
> originally I thought you would re-enable the code in dom0
> mpparse-xen.c and then just have a hypercall to determine
> whether dom0 had the vcpus pinned or not.  The advantage
> to that way is that if there is ever any other information 
> needed from the MP tables, we will just have it available
> in the dom0.

I'd rather stick with a minimal approach that does what
needs to be done to address this issue.

-Mark Langsdorf
Operating System Research Center
AMD

RE: [PATCH][retry 2][2/2] new platform hypervisor call to get APICIDs

[Langsdorf, Mark]

> > Anyway, if we make it a singleton-apicid vcpu_op, which 
> > fails on anything other than a pinned dom0

Will do.

> Also there is no particular reason not to have this vcpu_op return the
> acpiid as well. Call it something like VCPUOP_phys_id, returns an
> arch-specific uint64_t phys_id. On x86 we could have 8-bit 
> apicid plus 8-bit acpiid packed into that uint64_t, with the
> other 48 bits being MBZ for future expansion (e.g., x2apic extra
> id bits should we ever care). Either id can be 0xff indicating
> 'not available'.

Okay.  I'll add that.

-Mark Langsdorf
Operating System Research Center
AMD