Re: [Xen-devel] Re: [PATCH v2] xen: HVM X2APIC support

[Sheng Yang <sheng@linux.intel.com>]

On Tuesday 07 December 2010 10:01:48 Sheng Yang wrote:
> On Tuesday 07 December 2010 02:07:23 Jeremy Fitzhardinge wrote:
> > On 12/05/2010 11:49 PM, 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>
> > > ---
> > > 
> > >  arch/x86/include/asm/xen/hypervisor.h |   35
> > >  +++++++++++++++++++++++++++++++++ arch/x86/kernel/apic/apic.c
> > >  
> > >  |    4 ++-
> > >  
> > >  arch/x86/xen/enlighten.c              |   30
> > >  ++++++++++----------------- 3 files changed, 49 insertions(+), 20
> > >  deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/xen/hypervisor.h
> > > b/arch/x86/include/asm/xen/hypervisor.h index 396ff4c..bc5b804 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_para_available(void)
> > > +{
> > > +	return xen_hvm_need_lapic();
> > > +}
> > > +#else
> > > +static inline bool xen_para_available(void)
> > > +{
> > > +	return (xen_cpuid_base() != 0);
> > > +}
> > > +#endif
> > 
> > Surely this should be using the asm/hypervisor.h interface?
> 
> I think it's the proper place. Or any suggestion?

Oh, misread the comment. Seems put all detection there is a good idea.

--
regards
Yang, Sheng

> 
> > > +
> > > 
> > >  #endif /* _ASM_X86_XEN_HYPERVISOR_H */
> > > 
> > > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> > > index 3f838d5..1b68221 100644
> > > --- a/arch/x86/kernel/apic/apic.c
> > > +++ b/arch/x86/kernel/apic/apic.c
> > > @@ -52,6 +52,7 @@
> > > 
> > >  #include <asm/mce.h>
> > >  #include <asm/kvm_para.h>
> > >  #include <asm/tsc.h>
> > > 
> > > +#include <asm/xen/hypervisor.h>
> > > 
> > >  unsigned int num_processors;
> > > 
> > > @@ -1476,7 +1477,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 ||
> > > +		    (!kvm_para_available() && !xen_para_available()))
> > > 
> > >  			goto nox2apic;
> > 
> > What are the downsides of just using x2apic unconditionally?
> > 
> > Assuming there is some downside to using it all the time, what about
> > adding a pseudo-cpu feature flag meaning "use x2apic unconditionally"
> > rather than adding a bunch of ad-hoc tests here?
> 
> Because for native, x2apic should be used with interrupt-remapping,
> otherwise it didn't make much sense for native(the main reason to x2apic
> for bare-metal is support big machine with more than 255 APIC-ID, and
> without interrupt remapping it can't do so). But for hypervisor, we want
> to emulate the accessing faster through MSR, so we want them.
> 
> > >  		/*
> > >  		
> > >  		 * without IR all CPUs can be addressed by IOAPIC/MSI
> > > 
> > > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> > > index 235c0f4..7ef1645 100644
> > > --- a/arch/x86/xen/enlighten.c
> > > +++ b/arch/x86/xen/enlighten.c
> > > @@ -1245,25 +1245,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;
> > > 
> > > @@ -1373,6 +1354,17 @@ static bool __init xen_hvm_platform(void)
> > > 
> > >  	return true;
> > >  
> > >  }
> > > 
> > > +bool xen_hvm_need_lapic(void)
> > 
> > Where does this get used?
> 
> ?... xen_para_available() above...
> 
> --
> regards
> Yang, Sheng
> 
> > > +{
> > > +	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,
> > >  	
> >     J
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel