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 03:14:50PM +0000, David Vrabel wrote:
> On 22/01/15 15:09, Konrad Rzeszutek Wilk wrote:
> > 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.
> 
> This is easy to identify and fix, right?

Perhaps. Some of the ops are not used until you run say 'perf' and then
some of the ops get invoked.
> 
> >  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.
> 
> I think you should try this anyway and then we can evaluate the result.
>  I don't think it will be as bad as you fear.
> 
> >>>  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.
> 
> Please stop pretending that Xen PV guests have a "native" apic with
> "specials". We should provide a complete PV-specific apic driver.
> 
> David