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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/