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

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

[Konrad Rzeszutek Wilk]

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.

Reported-by: Cathy Avery <cathy.avery@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/apic.c      | 131 +++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/xen/enlighten.c |  90 +-------------------------------
 2 files changed, 132 insertions(+), 89 deletions(-)

diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
index 7005ced..3b2bd06 100644
--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -7,6 +7,7 @@
 #include <xen/xen.h>
 #include <xen/interface/physdev.h>
 #include "xen-ops.h"
+#include "smp.h"
 
 static unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
 {
@@ -28,7 +29,137 @@ static unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
 	return 0xfd;
 }
 
+static unsigned long xen_set_apic_id(unsigned int x)
+{
+	WARN_ON(1);
+	return x;
+}
+
+static unsigned int xen_get_apic_id(unsigned long x)
+{
+	return ((x)>>24) & 0xFFu;
+}
+
+static u32 xen_apic_read(u32 reg)
+{
+	struct xen_platform_op op = {
+		.cmd = XENPF_get_cpuinfo,
+		.interface_version = XENPF_INTERFACE_VERSION,
+		.u.pcpu_info.xen_cpuid = 0,
+	};
+	int ret = 0;
+
+	/* Shouldn't need this as APIC is turned off for PV, and we only
+	 * get called on the bootup processor. But just in case. */
+	if (!xen_initial_domain() || smp_processor_id())
+		return 0;
+
+	if (reg == APIC_LVR)
+		return 0x10;
+
+	if (reg != APIC_ID)
+		return 0;
+
+	ret = HYPERVISOR_dom0_op(&op);
+	if (ret)
+		return 0;
+
+	return op.u.pcpu_info.apic_id << 24;
+}
+
+static void xen_apic_write(u32 reg, u32 val)
+{
+	/* Warn to see if there's any stray references */
+	WARN_ON(1);
+}
+
+static u64 xen_apic_icr_read(void)
+{
+	return 0;
+}
+
+static void xen_apic_icr_write(u32 low, u32 id)
+{
+	/* Warn to see if there's any stray references */
+	WARN_ON(1);
+}
+
+static void xen_apic_wait_icr_idle(void)
+{
+        return;
+}
+
+static u32 xen_safe_apic_wait_icr_idle(void)
+{
+        return 0;
+}
+
+
+static int probe_xen(void)
+{
+	if (xen_pv_domain())
+		return 1;
+
+	return 0;
+}
+
+static int xen_id_always_valid(int apicid)
+{
+	return 1;
+}
+
+static struct apic xen_apic = {
+	.name = "Xen",
+	.probe = probe_xen,
+	/* The rest is copied from the default. */
+};
+
+/*
+ * 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;
+}
+
 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;
 }
+apic_driver(xen_apic);
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 78a881b..6c13a45 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -927,92 +927,6 @@ static void xen_io_delay(void)
 {
 }
 
-#ifdef CONFIG_X86_LOCAL_APIC
-static unsigned long xen_set_apic_id(unsigned int x)
-{
-	WARN_ON(1);
-	return x;
-}
-static unsigned int xen_get_apic_id(unsigned long x)
-{
-	return ((x)>>24) & 0xFFu;
-}
-static u32 xen_apic_read(u32 reg)
-{
-	struct xen_platform_op op = {
-		.cmd = XENPF_get_cpuinfo,
-		.interface_version = XENPF_INTERFACE_VERSION,
-		.u.pcpu_info.xen_cpuid = 0,
-	};
-	int ret = 0;
-
-	/* Shouldn't need this as APIC is turned off for PV, and we only
-	 * get called on the bootup processor. But just in case. */
-	if (!xen_initial_domain() || smp_processor_id())
-		return 0;
-
-	if (reg == APIC_LVR)
-		return 0x10;
-
-	if (reg != APIC_ID)
-		return 0;
-
-	ret = HYPERVISOR_dom0_op(&op);
-	if (ret)
-		return 0;
-
-	return op.u.pcpu_info.apic_id << 24;
-}
-
-static void xen_apic_write(u32 reg, u32 val)
-{
-	/* Warn to see if there's any stray references */
-	WARN_ON(1);
-}
-
-static u64 xen_apic_icr_read(void)
-{
-	return 0;
-}
-
-static void xen_apic_icr_write(u32 low, u32 id)
-{
-	/* Warn to see if there's any stray references */
-	WARN_ON(1);
-}
-
-static void xen_apic_wait_icr_idle(void)
-{
-        return;
-}
-
-static u32 xen_safe_apic_wait_icr_idle(void)
-{
-        return 0;
-}
-
-static void set_xen_basic_apic_ops(void)
-{
-	apic->read = xen_apic_read;
-	apic->write = xen_apic_write;
-	apic->icr_read = xen_apic_icr_read;
-	apic->icr_write = xen_apic_icr_write;
-	apic->wait_icr_idle = xen_apic_wait_icr_idle;
-	apic->safe_wait_icr_idle = xen_safe_apic_wait_icr_idle;
-	apic->set_apic_id = xen_set_apic_id;
-	apic->get_apic_id = xen_get_apic_id;
-
-#ifdef CONFIG_SMP
-	apic->send_IPI_allbutself = xen_send_IPI_allbutself;
-	apic->send_IPI_mask_allbutself = xen_send_IPI_mask_allbutself;
-	apic->send_IPI_mask = xen_send_IPI_mask;
-	apic->send_IPI_all = xen_send_IPI_all;
-	apic->send_IPI_self = xen_send_IPI_self;
-#endif
-}
-
-#endif
-
 static void xen_clts(void)
 {
 	struct multicall_space mcs;
@@ -1601,7 +1515,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
 	/*
 	 * set up the basic apic ops.
 	 */
-	set_xen_basic_apic_ops();
+	xen_init_apic();
 #endif
 
 	if (xen_feature(XENFEAT_mmu_pt_update_preserve_ad)) {
@@ -1714,8 +1628,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
 		if (HYPERVISOR_dom0_op(&op) == 0)
 			boot_params.kbd_status = op.u.firmware_info.u.kbd_shift_flags;
 
-		xen_init_apic();
-
 		/* Make sure ACS will be enabled */
 		pci_request_acs();
 
-- 
2.1.0

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

[Konrad Rzeszutek Wilk]

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.

Reported-by: Cathy Avery <cathy.avery@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/apic.c      | 131 +++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/xen/enlighten.c |  90 +-------------------------------
 2 files changed, 132 insertions(+), 89 deletions(-)

diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
index 7005ced..3b2bd06 100644
--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -7,6 +7,7 @@
 #include <xen/xen.h>
 #include <xen/interface/physdev.h>
 #include "xen-ops.h"
+#include "smp.h"
 
 static unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
 {
@@ -28,7 +29,137 @@ static unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
 	return 0xfd;
 }
 
+static unsigned long xen_set_apic_id(unsigned int x)
+{
+	WARN_ON(1);
+	return x;
+}
+
+static unsigned int xen_get_apic_id(unsigned long x)
+{
+	return ((x)>>24) & 0xFFu;
+}
+
+static u32 xen_apic_read(u32 reg)
+{
+	struct xen_platform_op op = {
+		.cmd = XENPF_get_cpuinfo,
+		.interface_version = XENPF_INTERFACE_VERSION,
+		.u.pcpu_info.xen_cpuid = 0,
+	};
+	int ret = 0;
+
+	/* Shouldn't need this as APIC is turned off for PV, and we only
+	 * get called on the bootup processor. But just in case. */
+	if (!xen_initial_domain() || smp_processor_id())
+		return 0;
+
+	if (reg == APIC_LVR)
+		return 0x10;
+
+	if (reg != APIC_ID)
+		return 0;
+
+	ret = HYPERVISOR_dom0_op(&op);
+	if (ret)
+		return 0;
+
+	return op.u.pcpu_info.apic_id << 24;
+}
+
+static void xen_apic_write(u32 reg, u32 val)
+{
+	/* Warn to see if there's any stray references */
+	WARN_ON(1);
+}
+
+static u64 xen_apic_icr_read(void)
+{
+	return 0;
+}
+
+static void xen_apic_icr_write(u32 low, u32 id)
+{
+	/* Warn to see if there's any stray references */
+	WARN_ON(1);
+}
+
+static void xen_apic_wait_icr_idle(void)
+{
+        return;
+}
+
+static u32 xen_safe_apic_wait_icr_idle(void)
+{
+        return 0;
+}
+
+
+static int probe_xen(void)
+{
+	if (xen_pv_domain())
+		return 1;
+
+	return 0;
+}
+
+static int xen_id_always_valid(int apicid)
+{
+	return 1;
+}
+
+static struct apic xen_apic = {
+	.name = "Xen",
+	.probe = probe_xen,
+	/* The rest is copied from the default. */
+};
+
+/*
+ * 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;
+}
+
 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;
 }
+apic_driver(xen_apic);
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 78a881b..6c13a45 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -927,92 +927,6 @@ static void xen_io_delay(void)
 {
 }
 
-#ifdef CONFIG_X86_LOCAL_APIC
-static unsigned long xen_set_apic_id(unsigned int x)
-{
-	WARN_ON(1);
-	return x;
-}
-static unsigned int xen_get_apic_id(unsigned long x)
-{
-	return ((x)>>24) & 0xFFu;
-}
-static u32 xen_apic_read(u32 reg)
-{
-	struct xen_platform_op op = {
-		.cmd = XENPF_get_cpuinfo,
-		.interface_version = XENPF_INTERFACE_VERSION,
-		.u.pcpu_info.xen_cpuid = 0,
-	};
-	int ret = 0;
-
-	/* Shouldn't need this as APIC is turned off for PV, and we only
-	 * get called on the bootup processor. But just in case. */
-	if (!xen_initial_domain() || smp_processor_id())
-		return 0;
-
-	if (reg == APIC_LVR)
-		return 0x10;
-
-	if (reg != APIC_ID)
-		return 0;
-
-	ret = HYPERVISOR_dom0_op(&op);
-	if (ret)
-		return 0;
-
-	return op.u.pcpu_info.apic_id << 24;
-}
-
-static void xen_apic_write(u32 reg, u32 val)
-{
-	/* Warn to see if there's any stray references */
-	WARN_ON(1);
-}
-
-static u64 xen_apic_icr_read(void)
-{
-	return 0;
-}
-
-static void xen_apic_icr_write(u32 low, u32 id)
-{
-	/* Warn to see if there's any stray references */
-	WARN_ON(1);
-}
-
-static void xen_apic_wait_icr_idle(void)
-{
-        return;
-}
-
-static u32 xen_safe_apic_wait_icr_idle(void)
-{
-        return 0;
-}
-
-static void set_xen_basic_apic_ops(void)
-{
-	apic->read = xen_apic_read;
-	apic->write = xen_apic_write;
-	apic->icr_read = xen_apic_icr_read;
-	apic->icr_write = xen_apic_icr_write;
-	apic->wait_icr_idle = xen_apic_wait_icr_idle;
-	apic->safe_wait_icr_idle = xen_safe_apic_wait_icr_idle;
-	apic->set_apic_id = xen_set_apic_id;
-	apic->get_apic_id = xen_get_apic_id;
-
-#ifdef CONFIG_SMP
-	apic->send_IPI_allbutself = xen_send_IPI_allbutself;
-	apic->send_IPI_mask_allbutself = xen_send_IPI_mask_allbutself;
-	apic->send_IPI_mask = xen_send_IPI_mask;
-	apic->send_IPI_all = xen_send_IPI_all;
-	apic->send_IPI_self = xen_send_IPI_self;
-#endif
-}
-
-#endif
-
 static void xen_clts(void)
 {
 	struct multicall_space mcs;
@@ -1601,7 +1515,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
 	/*
 	 * set up the basic apic ops.
 	 */
-	set_xen_basic_apic_ops();
+	xen_init_apic();
 #endif
 
 	if (xen_feature(XENFEAT_mmu_pt_update_preserve_ad)) {
@@ -1714,8 +1628,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
 		if (HYPERVISOR_dom0_op(&op) == 0)
 			boot_params.kbd_status = op.u.firmware_info.u.kbd_shift_flags;
 
-		xen_init_apic();
-
 		/* Make sure ACS will be enabled */
 		pci_request_acs();
 
-- 
2.1.0

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

[David Vrabel]

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.

> +};
> +
> +/*
> + * 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.

David

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

[Konrad Rzeszutek Wilk]

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

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

[David Vrabel]

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?

>  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

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

[Konrad Rzeszutek Wilk]

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

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

[Konrad Rzeszutek Wilk]

> > >>> +	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.

With said patch on an 288 CPUs machine I see it going over
this obstacle:

[    0.000000] smpboot: Allowing 288 CPUs, 0 hotplug CPUs                     
[    0.000000] setup_percpu: NR_CPUS:512 nr_cpumask_bits:512 nr_cpu_ids:288 nr_node_ids:1

.. thought something is off as we get:

  0.000000] Memory: 16019064K/16777212K available (7346K kernel code, 857K rwdata, 2336K rodata, 1108K init, 1260K bss, 758148K reserved, 0K cma-reserved)
[    0.000000] Hierarchical RCU implementation.                               
[    0.000000]  RCU restricting CPUs from NR_CPUS=512 to nr_cpu_ids=256.      
[    0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=256 
[    0.000000] NR_IRQS:33024 nr_irqs:5736 16                                  
[    0.000000] xen:events: Using FIFO-based ABI                               

And then it cuts off at 256 and 

[   74.474269] x86: Booted up 1 node, 256 CPUs                              
..
and things get bad:

[  134.624399] initcall init_scsi+0x0/0x92 [scsi_mod] returned 0 after 9359 usecs
udevd-work[10793]: error opening ATTR{/sys/devices/system/cpu/cpu0/online} for writing: Permission denied
                                                                           
[  155.781031] INFO: rcu_sched detected stalls on CPUs/tasks: { 42} (detected by 191, t=21011 jiffies, g=96, c=95, q=29554)
[  155.793168] Task dump for CPU 42:                                         
[  155.796924] modprobe        R  running task    13656 11124  11119 0x00000008
[  155.855305]  ffff88045fcffbc8 ffff880463fa1b00 ffff880013ed5e00 ffff88045fcffc08
[  155.865749]  ffffffff81c5a1b0 ffff88048674a310 ffff88045fcffc60 0000000000000000
[  155.875994]  0000000000010290 ffff8804881f01f8 ffffffff81821408 ffff88045fcffca8
[  155.888154] Call Trace:                                                  
[  155.891052]  [<ffffffff811c3c01>] ? vm_unmap_aliases+0x171/0x180         
[  155.897765]  [<ffffffff810413d9>] ? __raw_callee_save_xen_pmd_val+0x11/0x1e
[  155.909788]  [<ffffffff8109e0ac>] ? change_page_attr_set_clr+0xcc/0x530  
[  155.917208]  [<ffffffff811a4383>] ? pcpu_free_area+0x163/0x1e0           
[  155.923809]  [<ffffffff8109e623>] ? set_memory_x+0x43/0x50               
[  155.930022]  [<ffffffff8111b230>] ? set_page_attributes+0x20/0x30        
[  155.938717]  [<ffffffff8111b273>] ? unset_module_core_ro_nx+0x33/0x60    
[  155.945980]  [<ffffffff8111d2e2>] ? free_module+0x212/0x280              
[  155.952297]  [<ffffffff81120544>] ? load_module+0x1874/0x1920            
[  155.958776]  [<ffffffff8111ce50>] ? mod_kobject_put+0x50/0x50            
[  155.967476]  [<ffffffff810ccb0a>] ? __might_sleep+0x3a/0xa0              
[  155.973770]  [<ffffffff81120779>] ? SyS_init_module+0xe9/0x110           
[  155.980376]  [<ffffffff817274e9>] ? system_call_fastpath+0x12/0x17       
[  160.778754] NMI watchdog: BUG: soft lockup - CPU#42 stuck for 22s! [modprobe:11124]


> > 
> > Please stop pretending that Xen PV guests have a "native" apic with
> > "specials". We should provide a complete PV-specific apic driver.
> > 
> > David

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

[Konrad Rzeszutek Wilk]

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):

>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.

Reported-by: Cathy Avery <cathy.avery@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/apic.c      | 158 +++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/xen/enlighten.c |  90 +--------------------------
 2 files changed, 159 insertions(+), 89 deletions(-)

diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
index 7005ced..e7c2145 100644
--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -7,6 +7,7 @@
 #include <xen/xen.h>
 #include <xen/interface/physdev.h>
 #include "xen-ops.h"
+#include "smp.h"
 
 static unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
 {
@@ -28,7 +29,164 @@ static unsigned int xen_io_apic_read(unsigned apic, unsigned reg)
 	return 0xfd;
 }
 
+static unsigned long xen_set_apic_id(unsigned int x)
+{
+	WARN_ON(1);
+	return x;
+}
+
+static unsigned int xen_get_apic_id(unsigned long x)
+{
+	return ((x)>>24) & 0xFFu;
+}
+
+static u32 xen_apic_read(u32 reg)
+{
+	struct xen_platform_op op = {
+		.cmd = XENPF_get_cpuinfo,
+		.interface_version = XENPF_INTERFACE_VERSION,
+		.u.pcpu_info.xen_cpuid = 0,
+	};
+	int ret = 0;
+
+	/* Shouldn't need this as APIC is turned off for PV, and we only
+	 * get called on the bootup processor. But just in case. */
+	if (!xen_initial_domain() || smp_processor_id())
+		return 0;
+
+	if (reg == APIC_LVR)
+		return 0x10;
+
+	if (reg != APIC_ID)
+		return 0;
+
+	ret = HYPERVISOR_dom0_op(&op);
+	if (ret)
+		return 0;
+
+	return op.u.pcpu_info.apic_id << 24;
+}
+
+static void xen_apic_write(u32 reg, u32 val)
+{
+	/* Warn to see if there's any stray references */
+	WARN_ON(1);
+}
+
+static u64 xen_apic_icr_read(void)
+{
+	return 0;
+}
+
+static void xen_apic_icr_write(u32 low, u32 id)
+{
+	/* Warn to see if there's any stray references */
+	WARN_ON(1);
+}
+
+static u32 xen_safe_apic_wait_icr_idle(void)
+{
+        return 0;
+}
+
+
+static int probe_xen(void)
+{
+	if (xen_pv_domain())
+		return 1;
+
+	return 0;
+}
+
+static int xen_madt_oem_check(char *oem_id, char *oem_table_id)
+{
+	return 1;
+}
+
+static int xen_id_always_valid(int apicid)
+{
+	return 1;
+}
+
+static int xen_id_always_registered(void)
+{
+	return 1;
+}
+
+static int xen_phys_pkg_id(int initial_apic_id, int index_msb)
+{
+	return initial_apic_id >> index_msb;
+}
+
+static void xen_noop(void)
+{
+}
+
+static void xen_silent_inquire(int apicid)
+{
+}
+
+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 */
+
+	.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 */
+	.inquire_remote_apic		= xen_silent_inquire,
+
+	.read				= xen_apic_read,
+	.write				= xen_apic_write,
+	.eoi_write			= xen_apic_write,
+
+	.icr_read 			= xen_apic_icr_read,
+	.icr_write 			= xen_apic_icr_write,
+	.wait_icr_idle 			= xen_noop,
+	.safe_wait_icr_idle 		= xen_safe_apic_wait_icr_idle,
+
+#ifdef CONFIG_X86_32
+	/* generic_processor_info */
+	.x86_32_early_logical_apicid	= default_x86_32_early_logical_apicid,
+#endif
+};
+
 void __init xen_init_apic(void)
 {
 	x86_io_apic_ops.read = xen_io_apic_read;
+	/* On PV guests the APIC CPUID bit is disabled so none of the
+	 * routines end up executing. */
+	if (!xen_initial_domain())
+		apic = &xen_apic;
 }
+apic_driver(xen_apic);
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 78a881b..6c13a45 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -927,92 +927,6 @@ static void xen_io_delay(void)
 {
 }
 
-#ifdef CONFIG_X86_LOCAL_APIC
-static unsigned long xen_set_apic_id(unsigned int x)
-{
-	WARN_ON(1);
-	return x;
-}
-static unsigned int xen_get_apic_id(unsigned long x)
-{
-	return ((x)>>24) & 0xFFu;
-}
-static u32 xen_apic_read(u32 reg)
-{
-	struct xen_platform_op op = {
-		.cmd = XENPF_get_cpuinfo,
-		.interface_version = XENPF_INTERFACE_VERSION,
-		.u.pcpu_info.xen_cpuid = 0,
-	};
-	int ret = 0;
-
-	/* Shouldn't need this as APIC is turned off for PV, and we only
-	 * get called on the bootup processor. But just in case. */
-	if (!xen_initial_domain() || smp_processor_id())
-		return 0;
-
-	if (reg == APIC_LVR)
-		return 0x10;
-
-	if (reg != APIC_ID)
-		return 0;
-
-	ret = HYPERVISOR_dom0_op(&op);
-	if (ret)
-		return 0;
-
-	return op.u.pcpu_info.apic_id << 24;
-}
-
-static void xen_apic_write(u32 reg, u32 val)
-{
-	/* Warn to see if there's any stray references */
-	WARN_ON(1);
-}
-
-static u64 xen_apic_icr_read(void)
-{
-	return 0;
-}
-
-static void xen_apic_icr_write(u32 low, u32 id)
-{
-	/* Warn to see if there's any stray references */
-	WARN_ON(1);
-}
-
-static void xen_apic_wait_icr_idle(void)
-{
-        return;
-}
-
-static u32 xen_safe_apic_wait_icr_idle(void)
-{
-        return 0;
-}
-
-static void set_xen_basic_apic_ops(void)
-{
-	apic->read = xen_apic_read;
-	apic->write = xen_apic_write;
-	apic->icr_read = xen_apic_icr_read;
-	apic->icr_write = xen_apic_icr_write;
-	apic->wait_icr_idle = xen_apic_wait_icr_idle;
-	apic->safe_wait_icr_idle = xen_safe_apic_wait_icr_idle;
-	apic->set_apic_id = xen_set_apic_id;
-	apic->get_apic_id = xen_get_apic_id;
-
-#ifdef CONFIG_SMP
-	apic->send_IPI_allbutself = xen_send_IPI_allbutself;
-	apic->send_IPI_mask_allbutself = xen_send_IPI_mask_allbutself;
-	apic->send_IPI_mask = xen_send_IPI_mask;
-	apic->send_IPI_all = xen_send_IPI_all;
-	apic->send_IPI_self = xen_send_IPI_self;
-#endif
-}
-
-#endif
-
 static void xen_clts(void)
 {
 	struct multicall_space mcs;
@@ -1601,7 +1515,7 @@ asmlinkage __visible void __init xen_start_kernel(void)
 	/*
 	 * set up the basic apic ops.
 	 */
-	set_xen_basic_apic_ops();
+	xen_init_apic();
 #endif
 
 	if (xen_feature(XENFEAT_mmu_pt_update_preserve_ad)) {
@@ -1714,8 +1628,6 @@ asmlinkage __visible void __init xen_start_kernel(void)
 		if (HYPERVISOR_dom0_op(&op) == 0)
 			boot_params.kbd_status = op.u.firmware_info.u.kbd_shift_flags;
 
-		xen_init_apic();
-
 		/* Make sure ACS will be enabled */
 		pci_request_acs();
 
-- 
2.1.0

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

[David Vrabel]

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):
> 
> 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?
 Do we need a mechanism to ensure that this one is tried before any for
real hardware?

> +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

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

[Konrad Rzeszutek Wilk]

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