apic: Move hypervisor detection of x2apic to hypervisor.h

This patch is similiar to Gleb Natapov's patch for KVM, which enable the hypervisor to emulate x2apic feature for the guest. By this way, the emulation of lapic would be simpler with x2apic interface(MSR), and faster.

Acked-by Jeremy Fitzhardinge <[email protected]>

Signed-off-by Sheng Yang <[email protected]>

---

 arch/x86/include/asm/hypervisor.h     |    3 ++
 arch/x86/include/asm/xen/hypervisor.h |   35

+++++++++++++++++++++++++++++++++ arch/x86/xen/enlighten.c |
30 ++++++++++----------------- 3 files changed, 49 insertions(+), 19
deletions(-)

(quoted diff) +0 -0

b/arch/x86/include/asm/hypervisor.h index 0c6f7af..7a15153 100644

arch/x86/include/asm/hypervisor.h +3 -0

@@ -21,6 +21,7 @@
 #define _ASM_X86_HYPERVISOR_H
 
 #include <asm/kvm_para.h>
+#include <asm/xen/hypervisor.h>
 
 extern void init_hypervisor(struct cpuinfo_x86 *c);
 extern void init_hypervisor_platform(void);

@@ -53,6 +54,8 @@ static inline bool hypervisor_x2apic_available(void)
 {
 	if (kvm_para_available())
 		return true;
+	if (xen_x2apic_para_available())
+		return true;
 	return false;
 }
 

(quoted diff) +0 -0

b/arch/x86/include/asm/xen/hypervisor.h index 396ff4c..66d0fff 100644

arch/x86/include/asm/xen/hypervisor.h +35 -0

@@ -37,4 +37,39 @@
 extern struct shared_info *HYPERVISOR_shared_info;
 extern struct start_info *xen_start_info;
 
+#include <asm/processor.h>
+
+static inline uint32_t xen_cpuid_base(void)
+{
+	uint32_t base, eax, ebx, ecx, edx;
+	char signature[13];
+
+	for (base = 0x40000000; base < 0x40010000; base += 0x100) {
+		cpuid(base, &eax, &ebx, &ecx, &edx);
+		*(uint32_t *)(signature + 0) = ebx;
+		*(uint32_t *)(signature + 4) = ecx;
+		*(uint32_t *)(signature + 8) = edx;
+		signature[12] = 0;
+
+		if (!strcmp("XenVMMXenVMM", signature) && ((eax - base) >= 2))
+			return base;
+	}
+
+	return 0;
+}
+
+#ifdef CONFIG_XEN
+extern bool xen_hvm_need_lapic(void);
+
+static inline bool xen_x2apic_para_available(void)
+{
+	return xen_hvm_need_lapic();
+}
+#else
+static inline bool xen_x2apic_para_available(void)
+{
+	return (xen_cpuid_base() != 0);
+}
+#endif
+
 #endif /* _ASM_X86_XEN_HYPERVISOR_H */

arch/x86/xen/enlighten.c +11 -19

index 44dcad4..bb8aa31 100644
@@ -1256,25 +1256,6 @@ asmlinkage void __init xen_start_kernel(void)
 #endif
 }
 
-static uint32_t xen_cpuid_base(void)
-{
-	uint32_t base, eax, ebx, ecx, edx;
-	char signature[13];
-
-	for (base = 0x40000000; base < 0x40010000; base += 0x100) {
-		cpuid(base, &eax, &ebx, &ecx, &edx);
-		*(uint32_t *)(signature + 0) = ebx;
-		*(uint32_t *)(signature + 4) = ecx;
-		*(uint32_t *)(signature + 8) = edx;
-		signature[12] = 0;
-
-		if (!strcmp("XenVMMXenVMM", signature) && ((eax - base) >= 2))
-			return base;
-	}
-
-	return 0;
-}
-
 static int init_hvm_pv_info(int *major, int *minor)
 {
 	uint32_t eax, ebx, ecx, edx, pages, msr, base;

@@ -1384,6 +1365,17 @@ static bool __init xen_hvm_platform(void)
 	return true;
 }
 
+bool xen_hvm_need_lapic(void)
+{
+	if (xen_pv_domain())
+		return false;
+	if (xen_hvm_domain() && xen_feature(XENFEAT_hvm_pirqs) &&
+			xen_have_vector_callback)
+		return false;
+	return (xen_cpuid_base() != 0);
+}
+EXPORT_SYMBOL_GPL(xen_hvm_need_lapic);
+
 const __refconst struct hypervisor_x86 x86_hyper_xen_hvm = {
 	.name			= "Xen HVM",
 	.detect			= xen_hvm_platform,

Quoted 4 lines

arch/x86/include/asm/hypervisor.h +0 -0

@@ -1384,6 +1365,17 @@ static bool __init xen_hvm_platform(void)
      return true;
 }
 

arch/x86/xen/enlighten.c static bool __init xen_hvm_platform(void) · line 1378

Quoted 11 lines

arch/x86/xen/enlighten.c +11 -0

+bool xen_hvm_need_lapic(void)
+{
+     if (xen_pv_domain())
+             return false;
+     if (xen_hvm_domain() && xen_feature(XENFEAT_hvm_pirqs) &&
+                     xen_have_vector_callback)
+             return false;
+     return (xen_cpuid_base() != 0);
+}
+EXPORT_SYMBOL_GPL(xen_hvm_need_lapic);
+

Quoted 6 lines

Quoted 6 lines

(quoted diff) +0 -0

@@ -1384,6 +1365,17 @@ static bool __init xen_hvm_platform(void)
 
      return true;
 
 }
 

Quoted 11 lines

arch/x86/xen/enlighten.c static bool __init xen_hvm_platform(void) · line 1378

Quoted 11 lines

arch/x86/xen/enlighten.c +11 -0

+bool xen_hvm_need_lapic(void)
+{
+     if (xen_pv_domain())
+             return false;
+     if (xen_hvm_domain() && xen_feature(XENFEAT_hvm_pirqs) &&
+                     xen_have_vector_callback)
+             return false;
+     return (xen_cpuid_base() != 0);
+}
+EXPORT_SYMBOL_GPL(xen_hvm_need_lapic);
+

Since xen_hvm_domain() is always true if xen_cpuid_base() != 0, isn't this more obviously written as:

	if (!xen_hvm_domain())
		return false;

	if (xen_feature(XENFEAT_hvm_pirqs) && xen_have_vector_callback)
		return false;
	return true;

?

Also, checking for the XenVMMXenVMM signature alone seems like a very broad test for checking the availability of a specific feature, is there nothing more specific which we could/should be testing?

Ian.

Quoted 20 lines · On Wednesday 05 January 2011 22:56:28 Ian Campbell wrote:

Quoted 6 lines

Quoted 6 lines

(quoted diff) +0 -0

@@ -1384,6 +1365,17 @@ static bool __init xen_hvm_platform(void)
 
      return true;
 
 }
 

Quoted 11 lines

arch/x86/xen/enlighten.c static bool __init xen_hvm_platform(void) · line 1378

Quoted 11 lines

arch/x86/xen/enlighten.c +11 -0

+bool xen_hvm_need_lapic(void)
+{
+     if (xen_pv_domain())
+             return false;
+     if (xen_hvm_domain() && xen_feature(XENFEAT_hvm_pirqs) &&
+                     xen_have_vector_callback)
+             return false;
+     return (xen_cpuid_base() != 0);
+}
+EXPORT_SYMBOL_GPL(xen_hvm_need_lapic);
+

Since xen_hvm_domain() is always true if xen_cpuid_base() != 0, isn't this more obviously written as:

Quoted 2 lines

	if (!xen_hvm_domain())
		return false;

XEN_HVM_DOMAIN works only when kernel built with CONFIG_XEN. This patch can also support kernel built without CONFIG_XEN but with CONFIG_X86_X2APIC.

Quoted 4 lines

Also, checking for the XenVMMXenVMM signature alone seems like a very broad test for checking the availability of a specific feature, is there nothing more specific which we could/should be testing?

The CPU flag x2apic is checked when we want to enable x2apic, and only Xen which supported x2apic emulation would show this flag.

Quoted 21 lines · Sheng Yang

Quoted 20 lines · On Wednesday 05 January 2011 22:56:28 Ian Campbell wrote:

Quoted 6 lines

Quoted 6 lines

(quoted diff) +0 -0

@@ -1384,6 +1365,17 @@ static bool __init xen_hvm_platform(void)
 
      return true;
 
 }
 

Quoted 11 lines

arch/x86/xen/enlighten.c static bool __init xen_hvm_platform(void) · line 1378

Quoted 11 lines

arch/x86/xen/enlighten.c +11 -0

+bool xen_hvm_need_lapic(void)
+{
+     if (xen_pv_domain())
+             return false;
+     if (xen_hvm_domain() && xen_feature(XENFEAT_hvm_pirqs) &&
+                     xen_have_vector_callback)
+             return false;
+     return (xen_cpuid_base() != 0);
+}
+EXPORT_SYMBOL_GPL(xen_hvm_need_lapic);
+

Since xen_hvm_domain() is always true if xen_cpuid_base() != 0, isn't

Quoted 8 lines

Quoted 4 lines

this more obviously written as:

	if (!xen_hvm_domain())
	
		return false;

XEN_HVM_DOMAIN works only when kernel built with CONFIG_XEN. This patch can also support kernel built without CONFIG_XEN but with CONFIG_X86_X2APIC.

This function is only compiled when CONFIG_XEN=y, you have a different variant for the CONFIG_XEN=n case which just does the xen_cpuid_base() check.

It's actually a bit confusing to have xen_x2apic_para_available() defer to xen_hvm_need_lapic() when CONFIG_XEN is enabled but do the check itself when it is not. Can we not simply have:

static inline bool xen_x2apic_para_available(void) {

#ifdef CONFIG_XEN
	if (!xen_hvm_domain())
		return false;
	if (xen_have_vector_callback)
		return false;
	return true;
#else
	return xen_cpuid_base() != 0;

#endif
}

(either in include/asm/xen/hypervisor.h or out of line in arch/x86/kernel/cpu/hypervisor.c if this leads to include dependency hell)

Note that xen_have_vector_callback can be true only if xen_feature(XENFEAT_hvm_callback_vector) so I think that bit of the check was redundant.

Maybe even better would be to separate the general Xen presence logic from the decision to use x2apic, e.g.:

static inline bool xen_para_available(void)
{

#ifdef CONFIG_XEN
	return xen_hvm_domain();
#else
	return xen_cpuid_base() != 0;

#endif
}

static inline bool xen_x2apic_para_available(void)

{
	if (!xen_para_available())
		return false;
#ifdef CONFIG_XEN
	if (xen_have_vector_callback)
		return false;
#endif
	return true;
}

This could be simplified further if xen_have_vector_callback was #define to 0 when CONFIG_XEN=n.

Quoted 6 lines

Quoted 3 lines

Also, checking for the XenVMMXenVMM signature alone seems like a very broad test for checking the availability of a specific feature, is there nothing more specific which we could/should be testing?

The CPU flag x2apic is checked when we want to enable x2apic, and only Xen which supported x2apic emulation would show this flag.

A comment to that effect, in the checkin commentary if not the code, would be a useful reminder of this.

Thanks,
Ian.

Thanks for the comments, but seems it's a little late. The patches have been there for more than a month since the first version, and now they are finally in the tree... And since it's not a bug, could we leave it to the later clean up?

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Also, checking for the XenVMMXenVMM signature alone seems like a very broad test for checking the availability of a specific feature, is there nothing more specific which we could/should be testing?

Quoted 5 lines

Quoted 2 lines

The CPU flag x2apic is checked when we want to enable x2apic, and only Xen which supported x2apic emulation would show this flag.

A comment to that effect, in the checkin commentary if not the code, would be a useful reminder of this.

The caller of the function indicate so, it's in the x2apic enabling code(which is the same as KVM). So I think that maybe enough.

Quoted 4 lines · Sheng Yang

Thanks for the comments, but seems it's a little late. The patches have been there for more than a month since the first version, and now they are finally in the tree... And since it's not a bug, could we leave it to the later clean up?

Sure. Are you going to do that?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Also, checking for the XenVMMXenVMM signature alone seems like a very broad test for checking the availability of a specific feature, is there nothing more specific which we could/should be testing?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

The CPU flag x2apic is checked when we want to enable x2apic, and only Xen which supported x2apic emulation would show this flag.

A comment to that effect, in the checkin commentary if not the code, would be a useful reminder of this.

The caller of the function indicate so, it's in the x2apic enabling code(which is the same as KVM). So I think that maybe enough.

It's not obvious to the likes of me but if it makes sense to people who know x2apic (but not necessarily Xen or KVM) then I guess it's ok.

Ian.

Quoted 7 lines · On Thursday 06 January 2011 17:35:57 Ian Campbell wrote:

Quoted 4 lines · Sheng Yang

Thanks for the comments, but seems it's a little late. The patches have been there for more than a month since the first version, and now they are finally in the tree... And since it's not a bug, could we leave it to the later clean up?

Sure. Are you going to do that?

OK, I would do it later.

Quoted 7 lines · Sheng Yang

Quoted 6 lines · On Thursday 06 January 2011 17:35:57 Ian Campbell wrote:

Quoted 4 lines · Sheng Yang

Thanks for the comments, but seems it's a little late. The patches have been there for more than a month since the first version, and now they are finally in the tree... And since it's not a bug, could we leave it to the later clean up?

Quoted 3 lines

Quoted 1 line

Sure. Are you going to do that?

OK, I would do it later.

Please test that
git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git works for you.

Then we can reuse it for Xen later.

Acked-by Jeremy Fitzhardinge <[email protected]>

Signed-off-by Sheng Yang <[email protected]>

---
 arch/x86/include/asm/hypervisor.h |    9 +++++++++
 arch/x86/kernel/apic/apic.c       |    5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)

arch/x86/include/asm/hypervisor.h +9 -0

index ff2546c..0c6f7af 100644
@@ -20,6 +20,8 @@
 #ifndef _ASM_X86_HYPERVISOR_H
 #define _ASM_X86_HYPERVISOR_H
 
+#include <asm/kvm_para.h>
+
 extern void init_hypervisor(struct cpuinfo_x86 *c);
 extern void init_hypervisor_platform(void);
 

@@ -47,4 +49,11 @@ extern const struct hypervisor_x86 x86_hyper_vmware;
 extern const struct hypervisor_x86 x86_hyper_ms_hyperv;
 extern const struct hypervisor_x86 x86_hyper_xen_hvm;
 
+static inline bool hypervisor_x2apic_available(void)
+{
+	if (kvm_para_available())
+		return true;
+	return false;
+}
+
 #endif

arch/x86/kernel/apic/apic.c +3 -2

index 3f838d5..8408f2d 100644
@@ -50,8 +50,8 @@
 #include <asm/mtrr.h>
 #include <asm/smp.h>
 #include <asm/mce.h>
-#include <asm/kvm_para.h>
 #include <asm/tsc.h>
+#include <asm/hypervisor.h>
 
 unsigned int num_processors;
 

@@ -1476,7 +1476,8 @@ void __init enable_IR_x2apic(void)
 		/* IR is required if there is APIC ID > 255 even when running
 		 * under KVM
 		 */
-		if (max_physical_apicid > 255 || !kvm_para_available())
+		if (max_physical_apicid > 255 ||
+		    !hypervisor_x2apic_available())
 			goto nox2apic;
 		/*
 		 * without IR all CPUs can be addressed by IOAPIC/MSI

Quoted 1 line · Sheng Yang

Then we can reuse it for Xen later.

You should probably get someone kvm-ish to ack this too.

Quoted 4 lines · Jeremy Fitzhardinge

Quoted 1 line · Sheng Yang

Then we can reuse it for Xen later.

You should probably get someone kvm-ish to ack this too.

Looks good to me.

Quoted 4 lines · Jeremy Fitzhardinge

Quoted 1 line · Sheng Yang

Then we can reuse it for Xen later.

You should probably get someone kvm-ish to ack this too.

Looks good to me.

Quoted 4 lines · On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:

Quoted 3 lines · Jeremy Fitzhardinge

Quoted 1 line · Sheng Yang

Then we can reuse it for Xen later.

Quoted 3 lines

Quoted 1 line

You should probably get someone kvm-ish to ack this too.

Looks good to me.

Ingo,

Comments?

Quoted 11 lines · Sheng Yang

Quoted 7 lines · On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:

Quoted 4 lines · Jeremy Fitzhardinge

Quoted 1 line · Sheng Yang

Then we can reuse it for Xen later.

You should probably get someone kvm-ish to ack this too.

Looks good to me.

Ingo,

Quoted 1 line

Comments?

Acked-by Ingo Molnar <[email protected]>

I suspect these patches want to be carried in the Xen tree, right? Shouldnt cause too bad conflicts, as long as the Xen tree is pushed after the x86 tree in the merge window.

Thanks,

Ingo

Quoted 9 lines · On Tuesday 04 January 2011 17:24:18 Ingo Molnar wrote:

Quoted 8 lines · Sheng Yang

Quoted 5 lines · On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:

Quoted 3 lines · Jeremy Fitzhardinge

Quoted 1 line · Sheng Yang

Then we can reuse it for Xen later.

You should probably get someone kvm-ish to ack this too.

Looks good to me.

Ingo,

Quoted 6 lines

Quoted 1 line

Comments?

Acked-by Ingo Molnar <[email protected]>

I suspect these patches want to be carried in the Xen tree, right? Shouldnt cause too bad conflicts, as long as the Xen tree is pushed after the x86 tree in the merge window.

Jeremy?

Quoted 13 lines · Sheng Yang

Quoted 12 lines · On Tuesday 04 January 2011 17:24:18 Ingo Molnar wrote:

Quoted 9 lines · Sheng Yang

Quoted 5 lines · On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:

Quoted 3 lines · Jeremy Fitzhardinge

Quoted 1 line · Sheng Yang

Then we can reuse it for Xen later.

You should probably get someone kvm-ish to ack this too.

Looks good to me.

Ingo,

Comments?

Acked-by Ingo Molnar <[email protected]>

Quoted 4 lines

Quoted 3 lines

I suspect these patches want to be carried in the Xen tree, right? Shouldnt cause too bad conflicts, as long as the Xen tree is pushed after the x86 tree in the merge window.

Jeremy?

Sure, I'm OK with that, but I'm on vacation until the 15th. Konrad, can you pick this up?

Thanks,
J