Re: [RFC PATCH] xen, apic: Setup our own APIC driver and validator for APIC IDs.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Jan 22, 2015 at 10:00:55AM +0000, David Vrabel wrote:
> On 21/01/15 21:56, Konrad Rzeszutek Wilk wrote:
> > +static struct apic xen_apic = {
> > +	.name = "Xen",
> > +	.probe = probe_xen,
> > +	/* The rest is copied from the default. */
> 
> Explicitly initialize all required members here.  memcpy'ing from the
> default makes it far too unclear which ops this apic driver actually
> provides.

That will be hard for two reasons:
 1) if the 'struct apic' expands and we don't - then we will crash.
 2)  we would need to use the default 'apic' functions ones - which are not
     necceesarily exposed to the rest of the system. Hence there will
     be a lot of exposing those.

> 
> > +};
> > +
> > +/*
> > + * This is needed as in enlighten.c we mask the x2APIC bit because we
> > + * do not want PV guests to use anything but most of the default apic routines.
> > + *
> > + * However the default ->apic_id_valid enforces that the APIC ID MUST
> > + * be below 0xFF which is not the case for x2APIC - so we need a way
> > + * to allow that to function properly.
> > + */
> > +static bool __init xen_check_x2apic(void)
> > +{
> > +#ifdef CONFIG_X2APIC
> > +	unsigned int ax, bx, cx, dx;
> > +
> > +	ax = 1;
> > +	cx = 0; /* Don't care about dx, and bx */
> > +	native_cpuid(&ax, &bx, &cx, &dx);
> > +	if (cx & (1 << (X86_FEATURE_X2APIC % 32)))
> > +		return true;
> > +#endif
> > +	return false;
> > +}
> 
> Not needed (see below).
> 
> >  void __init xen_init_apic(void)
> >  {
> >  	x86_io_apic_ops.read = xen_io_apic_read;
> > +
> > +	memcpy(&xen_apic, apic, sizeof(struct apic));
> > +	xen_apic.probe = probe_xen;
> > +	xen_apic.name = "Xen";
> > +
> > +	xen_apic.read = xen_apic_read;
> > +	xen_apic.write = xen_apic_write;
> > +	xen_apic.icr_read = xen_apic_icr_read;
> > +	xen_apic.icr_write = xen_apic_icr_write;
> > +	xen_apic.wait_icr_idle = xen_apic_wait_icr_idle;
> > +	xen_apic.safe_wait_icr_idle = xen_safe_apic_wait_icr_idle;
> > +	xen_apic.set_apic_id = xen_set_apic_id;
> > +	xen_apic.get_apic_id = xen_get_apic_id;
> > +
> > +	xen_apic.send_IPI_allbutself = xen_send_IPI_allbutself;
> > +	xen_apic.send_IPI_mask_allbutself = xen_send_IPI_mask_allbutself;
> > +	xen_apic.send_IPI_mask = xen_send_IPI_mask;
> > +	xen_apic.send_IPI_all = xen_send_IPI_all;
> > +	xen_apic.send_IPI_self = xen_send_IPI_self;
> > +
> > +	if (xen_check_x2apic())
> > +		xen_apic.apic_id_valid = xen_id_always_valid;
> 
> Just always use xen_id_always_valid regardless of whether the machine
> has x2apic or not.  It is possible to have more VCPUs that PCPUs.

In which case perhaps the patch ought to be just simpler and
instead of having our own 'struct apic' we continue over-writting
the default one - and just change 'apic_id_valid' to our own.

> 
> David
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel