Messages in this thread | | | From | Ian Campbell <> | Date | Thu, 06 Jan 2011 09:10:30 +0000 | Subject | Re: [PATCH 2/2 v2] xen: HVM X2APIC support |
| |
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
| |