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

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

On Wed, Feb 11, 2015 at 09:53:26AM +0000, David Vrabel wrote:
> On 10/02/15 20:33, 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.
> > 
> > RFC (boots under PV, PVHVM, PV dom0):

And it boots under the 288 CPU machine (the original problem)
.. thought it exposes two other issues:


(XEN) SMP: Allowing 288 CPUs (0 hotplug CPUs)
(XEN) Brought up 288 CPUs
..
(XEN) Dom0 has maximum 255 VCPUs
(XEN) xentrace: p157 mfn 225524 offset 35896
(XEN) xentrace: p255 mfn 21fe3e offset 58142
[    0.000000] smpboot: Allowing 288 CPUs, 0 hotplug CPUs
[    0.000000] xen_filter_cpu_maps: CPU255 is not up!
..
[    0.000000] xen_filter_cpu_maps: CPU287 is not up!
[    0.000000] xen_filter_cpu_maps: nr_cpu_ids: 288, subtract: 33
[    0.000000] 	RCU restricting CPUs from NR_CPUS=512 to nr_cpu_ids=255.

... with the result that we can't bring up the 256->287 CPUs up.

It looks as if we a limiting Dom0 to 255. That seems to be due to:

> > 
> > From 27702ef618af068736d13aeadcbcacd2a6780e82 Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Fri, 9 Jan 2015 17:55:52 -0500
> > Subject: [PATCH] xen,apic: Setup our own APIC driver and validator for APIC
> >  IDs.
> > 
> > Via CPUID masking and the different apic-> overrides we
> > effectively make PV guests only but with the default APIC
> > driver. That is OK as an PV guest should never access any
> > APIC registers. However, the APIC is also used to limit the
> > amount of CPUs if the APIC IDs are incorrect - and since we
> > mask the x2APIC from the CPUID - any APIC IDs above 0xFF
> > are deemed incorrect by the default APIC routines.
> > 
> > As such add a new routine to check for APIC ID which will
> > be only used if the CPUID (native one) tells us the system
> > is using x2APIC.
> > 
> > This allows us to boot with more than 255 CPUs if running
> > as initial domain.
> 
> This looks quite reasonable to me.  What order are apic driver tried in?

No order. Or rather the order is based on how the compiler stashes
them in.

>  Do we need a mechanism to ensure that this one is tried before any for
> real hardware?

There are two probe mechanism - the .probe and then later it is:
x86_platform.apic_post_init which we can also utilize to make sure
the APIC is set to Xen.

Let me add that in.

> 
> > +static struct apic xen_apic = {
> > +	.name 				= "Xen PV",
> > +	.probe 				= probe_xen,
> > +	.acpi_madt_oem_check		= xen_madt_oem_check,
> > +	.apic_id_valid 			= xen_id_always_valid,
> > +	.apic_id_registered 		= xen_id_always_registered,
> > +
> > +	.irq_delivery_mode		= 0xbeef, /* used in native_compose_msi_msg only */
> > +	.irq_dest_mode			= 0xbeef, /* used in native_compose_msi_msg only */
> 
> Omit members that are unused, leaving them as 0 or NULL.
> 
> > +	.target_cpus			= default_target_cpus,
> > +	.disable_esr			= 0,
> > +	.dest_logical			= 0, /* default_send_IPI_ use it but we use our own. */
> > +	.check_apicid_used		= default_check_apicid_used, /* Used on 32-bit */
> > +
> > +	.vector_allocation_domain	= flat_vector_allocation_domain,
> > +	.init_apic_ldr			= xen_noop, /* setup_local_APIC calls it */
> > +
> > +	.ioapic_phys_id_map		= default_ioapic_phys_id_map, /* Used on 32-bit */
> > +	.setup_apic_routing		= NULL,
> > +	.cpu_present_to_apicid		= default_cpu_present_to_apicid,
> > +	.apicid_to_cpu_present		= physid_set_mask_of_physid, /* Used on 32-bit */
> > +	.check_phys_apicid_present	= default_check_phys_apicid_present, /* smp_sanity_check needs it */
> > +	.phys_pkg_id			= xen_phys_pkg_id, /* detect_ht */
> > +
> > +	.get_apic_id 			= xen_get_apic_id,
> > +	.set_apic_id 			= xen_set_apic_id, /* Can be NULL on 32-bit. */
> > +	.apic_id_mask			= 0xFF << 24, /* Used by verify_local_APIC. Match with what xen_get_apic_id does. */
> > +
> > +	.cpu_mask_to_apicid_and		= flat_cpu_mask_to_apicid_and,
> > +
> > +	.send_IPI_mask 			= xen_send_IPI_mask,
> > +	.send_IPI_mask_allbutself 	= xen_send_IPI_mask_allbutself,
> > +	.send_IPI_allbutself 		= xen_send_IPI_allbutself,
> > +	.send_IPI_all 			= xen_send_IPI_all,
> > +	.send_IPI_self 			= xen_send_IPI_self,
> > +
> > +	.wait_for_init_deassert		= false, /* Used by AP bootup - smp_callin which we don't use */
> 
> Omit.
> 
> David