[PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[Sheng Yang]

Then we can reuse it for Xen later.

Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/include/asm/hypervisor.h |    9 +++++++++
 arch/x86/kernel/apic/apic.c       |    5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index ff2546c..0c6f7af 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -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
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 3f838d5..8408f2d 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -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
-- 
1.7.0.1

[PATCH 2/2 v2] xen: HVM X2APIC support

[Sheng Yang]

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 <jeremy.fitzhardinge@citrix.com>
Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 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(-)

diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 0c6f7af..7a15153 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -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;
 }
 
diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
index 396ff4c..66d0fff 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -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 */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 44dcad4..bb8aa31 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -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,
-- 
1.7.0.1

Re: [PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[Jeremy Fitzhardinge]

On 12/20/2010 10:18 PM, Sheng Yang wrote:
> Then we can reuse it for Xen later.

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

    J

> Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/include/asm/hypervisor.h |    9 +++++++++
>  arch/x86/kernel/apic/apic.c       |    5 +++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
> index ff2546c..0c6f7af 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -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
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 3f838d5..8408f2d 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -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

Re: [PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[Avi Kivity]

On 12/21/2010 07:15 PM, Jeremy Fitzhardinge wrote:
> On 12/20/2010 10:18 PM, Sheng Yang wrote:
> >  Then we can reuse it for Xen later.
>
> You should probably get someone kvm-ish to ack this too.

Looks good to me.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[Sheng Yang]

On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:
> On 12/21/2010 07:15 PM, Jeremy Fitzhardinge wrote:
> > On 12/20/2010 10:18 PM, Sheng Yang wrote:
> > >  Then we can reuse it for Xen later.
> > 
> > You should probably get someone kvm-ish to ack this too.
> 
> Looks good to me.

Ingo/Peter?

--
regards
Yang, Sheng

Re: [PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[Sheng Yang]

On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:
> On 12/21/2010 07:15 PM, Jeremy Fitzhardinge wrote:
> > On 12/20/2010 10:18 PM, Sheng Yang wrote:
> > >  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?

--
regards
Yang, Sheng

Re: [PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[Ingo Molnar]

* Sheng Yang <sheng@linux.intel.com> wrote:

> On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:
> > On 12/21/2010 07:15 PM, Jeremy Fitzhardinge wrote:
> > > On 12/20/2010 10:18 PM, Sheng Yang wrote:
> > > >  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 <mingo@elte.hu>

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

Re: [PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[Sheng Yang]

On Tuesday 04 January 2011 17:24:18 Ingo Molnar wrote:
> * Sheng Yang <sheng@linux.intel.com> wrote:
> > On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:
> > > On 12/21/2010 07:15 PM, Jeremy Fitzhardinge wrote:
> > > > On 12/20/2010 10:18 PM, Sheng Yang wrote:
> > > > >  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 <mingo@elte.hu>
> 
> 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?

--
regards
Yang, Sheng

> 
> Thanks,
> 
> 	Ingo

Re: [PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[Jeremy Fitzhardinge]

On 01/05/2011 12:51 PM, Sheng Yang wrote:
> On Tuesday 04 January 2011 17:24:18 Ingo Molnar wrote:
>> * Sheng Yang <sheng@linux.intel.com> wrote:
>>> On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:
>>>> On 12/21/2010 07:15 PM, Jeremy Fitzhardinge wrote:
>>>>> On 12/20/2010 10:18 PM, Sheng Yang wrote:
>>>>>>  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 <mingo@elte.hu>
>>
>> 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

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Sheng Yang]

On Tuesday 21 December 2010 14:18:49 Sheng Yang wrote:
> 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 <jeremy.fitzhardinge@citrix.com>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---

Jeremy & Konrad, 

We also need this one.

Thanks!

--
regards
Yang, Sheng

>  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(-)
> 
> diff --git a/arch/x86/include/asm/hypervisor.h
> b/arch/x86/include/asm/hypervisor.h index 0c6f7af..7a15153 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -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;
>  }
> 
> diff --git a/arch/x86/include/asm/xen/hypervisor.h
> b/arch/x86/include/asm/xen/hypervisor.h index 396ff4c..66d0fff 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -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 */
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 44dcad4..bb8aa31 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -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,

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Ian Campbell]

> > @@ -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);
> > +

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.

-- 
Ian Campbell
Current Noise: Taint - Mass Appeal Sadness

What's a cult?  It just means not enough people to make a minority.
		-- Robert Altman

Re: [PATCH 1/2] apic: Move hypervisor detection of x2apic to hypervisor.h

[Konrad Rzeszutek Wilk]

On Wed, Jan 05, 2011 at 06:40:32PM +1100, Jeremy Fitzhardinge wrote:
> On 01/05/2011 12:51 PM, Sheng Yang wrote:
> > On Tuesday 04 January 2011 17:24:18 Ingo Molnar wrote:
> >> * Sheng Yang <sheng@linux.intel.com> wrote:
> >>> On Wednesday 22 December 2010 02:15:44 Avi Kivity wrote:
> >>>> On 12/21/2010 07:15 PM, Jeremy Fitzhardinge wrote:
> >>>>> On 12/20/2010 10:18 PM, Sheng Yang wrote:
> >>>>>>  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 <mingo@elte.hu>
> >>
> >> 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?

Done.
> 
> Thanks,
>     J

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Sheng Yang]

On Wednesday 05 January 2011 22:56:28 Ian Campbell wrote:
> > > @@ -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);
> > > +
> 
> 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;

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.

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

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

--
regards
Yang, Sheng

> 
> Ian.

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Ian Campbell]

On Thu, 2011-01-06 at 09:20 +0800, Sheng Yang wrote:
> On Wednesday 05 January 2011 22:56:28 Ian Campbell wrote:
> > > > @@ -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);
> > > > +
> > 
> > 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;
> 
> 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.

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

-- 
Ian Campbell
Current Noise: Ramesses - Before The Jackals

Women complain about sex more than men.  Their gripes fall into two
categories: (1) Not enough and (2) Too much.
		-- Ann Landers

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Sheng Yang]

On Thursday 06 January 2011 17:10:30 Ian Campbell wrote:
> On Thu, 2011-01-06 at 09:20 +0800, Sheng Yang wrote:
> > On Wednesday 05 January 2011 22:56:28 Ian Campbell wrote:
> > > > > @@ -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);
> > > > > +
> > > 
> > > 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;
> > 
> > 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.

I am not familiar with these dependence, and just followed Stefano's comments.
> 
> 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.

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

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.

--
regards
Yang, Sheng

> 
> Thanks,
> Ian.

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Ian Campbell]

On Thu, 2011-01-06 at 17:23 +0800, Sheng Yang wrote:

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


-- 
Ian Campbell
Current Noise: Death - Spirit Crusher

I know you're in search of yourself, I just haven't seen you anywhere.

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Sheng Yang]

On Thursday 06 January 2011 17:35:57 Ian Campbell wrote:
> On Thu, 2011-01-06 at 17:23 +0800, Sheng Yang wrote:
> > 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.

--
regards
Yang, Sheng

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

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Konrad Rzeszutek Wilk]

On Thu, Jan 06, 2011 at 06:05:27PM +0800, Sheng Yang wrote:
> On Thursday 06 January 2011 17:35:57 Ian Campbell wrote:
> > On Thu, 2011-01-06 at 17:23 +0800, Sheng Yang wrote:
> > > 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.

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

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Sheng Yang]

On Thursday 06 January 2011 23:00:58 Konrad Rzeszutek Wilk wrote:
> On Thu, Jan 06, 2011 at 06:05:27PM +0800, Sheng Yang wrote:
> > On Thursday 06 January 2011 17:35:57 Ian Campbell wrote:
> > > On Thu, 2011-01-06 at 17:23 +0800, Sheng Yang wrote:
> > > > 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.
> 
> Please test that
> git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git works for
> you.

Works well for me, and thanks for updating with Ian's comments!

--
regards
Yang, Sheng

Re: [PATCH 2/2 v2] xen: HVM X2APIC support

[Sheng Yang]

On Wednesday 08 December 2010 14:04:00 Sheng Yang wrote:
> 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.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>

Jeremy, any comments? 

Keir has already checked the hypervisor part in.

--
regards
Yang, Sheng

> ---
>  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(-)
> 
> diff --git a/arch/x86/include/asm/hypervisor.h
> b/arch/x86/include/asm/hypervisor.h index 0c6f7af..7a15153 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -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;
>  }
> 
> diff --git a/arch/x86/include/asm/xen/hypervisor.h
> b/arch/x86/include/asm/xen/hypervisor.h index 396ff4c..66d0fff 100644
> --- a/arch/x86/include/asm/xen/hypervisor.h
> +++ b/arch/x86/include/asm/xen/hypervisor.h
> @@ -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 */
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 44dcad4..bb8aa31 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -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,

[PATCH 2/2 v2] xen: HVM X2APIC support

[Sheng Yang]

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.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 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(-)

diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 0c6f7af..7a15153 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -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;
 }
 
diff --git a/arch/x86/include/asm/xen/hypervisor.h b/arch/x86/include/asm/xen/hypervisor.h
index 396ff4c..66d0fff 100644
--- a/arch/x86/include/asm/xen/hypervisor.h
+++ b/arch/x86/include/asm/xen/hypervisor.h
@@ -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 */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 44dcad4..bb8aa31 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -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,
-- 
1.7.0.1