2023-07-24 13:52:13

by Thomas Gleixner

Subject: [patch V2 50/58] x86/apic: Provide common init infrastructure

In preparation for converting the hotpath APIC callbacks to static keys,
provide common initialization inforastructure.

Lift apic_install_drivers() from probe_64.c and convert all places which
switch the apic instance by storing the pointer to use apic_install_driver()
as a first step.

Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Peter Zijlstra (Intel) <[email protected]>
---
arch/x86/include/asm/apic.h | 2 +
arch/x86/kernel/apic/Makefile | 2 -
arch/x86/kernel/apic/apic.c | 31 -----------------------
arch/x86/kernel/apic/apic_flat_64.c | 6 ----
arch/x86/kernel/apic/bigsmp_32.c | 6 +---
arch/x86/kernel/apic/init.c | 47 ++++++++++++++++++++++++++++++++++++
arch/x86/kernel/apic/probe_32.c | 5 +--
arch/x86/kernel/apic/probe_64.c | 13 ---------
arch/x86/xen/apic.c | 10 ++-----
9 files changed, 59 insertions(+), 63 deletions(-)

--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -344,6 +344,8 @@ extern int lapic_can_unplug_cpu(void);

#ifdef CONFIG_X86_LOCAL_APIC

+void __init apic_install_driver(struct apic *driver);
+
static inline u32 apic_read(u32 reg)
{
return apic->read(reg);
--- a/arch/x86/kernel/apic/Makefile
+++ b/arch/x86/kernel/apic/Makefile
@@ -7,7 +7,7 @@
# In particualr, smp_apic_timer_interrupt() is called in random places.
KCOV_INSTRUMENT := n

-obj-$(CONFIG_X86_LOCAL_APIC) += apic.o apic_common.o apic_noop.o ipi.o vector.o
+obj-$(CONFIG_X86_LOCAL_APIC) += apic.o apic_common.o apic_noop.o ipi.o vector.o init.o
obj-y += hw_nmi.o

obj-$(CONFIG_X86_IO_APIC) += io_apic.o
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -236,8 +236,7 @@ static int modern_apic(void)
*/
static void __init apic_disable(void)
{
- pr_info("APIC: switched to apic NOOP\n");
- apic = &apic_noop;
+ apic_install_driver(&apic_noop);
}

void native_apic_icr_write(u32 low, u32 id)
@@ -2486,34 +2485,6 @@ u32 x86_msi_msg_get_destid(struct msi_ms
}
EXPORT_SYMBOL_GPL(x86_msi_msg_get_destid);

-#ifdef CONFIG_X86_64
-void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
-{
- struct apic **drv;
-
- for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
- (*drv)->wakeup_secondary_cpu_64 = handler;
-}
-#endif
-
-/*
- * Override the generic EOI implementation with an optimized version.
- * Only called during early boot when only one CPU is active and with
- * interrupts disabled, so we know this does not race with actual APIC driver
- * use.
- */
-void __init apic_set_eoi_cb(void (*eoi)(void))
-{
- struct apic **drv;
-
- for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
- /* Should happen once for each apic */
- WARN_ON((*drv)->eoi == eoi);
- (*drv)->native_eoi = (*drv)->eoi;
- (*drv)->eoi = eoi;
- }
-}
-
static void __init apic_bsp_up_setup(void)
{
#ifdef CONFIG_X86_64
--- a/arch/x86/kernel/apic/apic_flat_64.c
+++ b/arch/x86/kernel/apic/apic_flat_64.c
@@ -143,11 +143,7 @@ static int physflat_acpi_madt_oem_check(

static int physflat_probe(void)
{
- if (apic == &apic_physflat || num_possible_cpus() > 8 ||
- jailhouse_paravirt())
- return 1;
-
- return 0;
+ return apic == &apic_physflat || num_possible_cpus() > 8 || jailhouse_paravirt();
}

static struct apic apic_physflat __ro_after_init = {
--- a/arch/x86/kernel/apic/bigsmp_32.c
+++ b/arch/x86/kernel/apic/bigsmp_32.c
@@ -119,10 +119,8 @@ bool __init apic_bigsmp_possible(bool cm

void __init apic_bigsmp_force(void)
{
- if (apic != &apic_bigsmp) {
- apic = &apic_bigsmp;
- pr_info("Overriding APIC driver with bigsmp\n");
- }
+ if (apic != &apic_bigsmp)
+ apic_install_driver(&apic_bigsmp);
}

apic_driver(apic_bigsmp);
--- /dev/null
+++ b/arch/x86/kernel/apic/init.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#define pr_fmt(fmt) "APIC: " fmt
+
+#include <asm/apic.h>
+
+#include "local.h"
+
+void __init apic_install_driver(struct apic *driver)
+{
+ if (apic == driver)
+ return;
+
+ apic = driver;
+
+ if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid)
+ apic->max_apic_id = x2apic_max_apicid;
+
+ pr_info("Switched APIC routing to: %s\n", driver->name);
+}
+
+#ifdef CONFIG_X86_64
+void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
+{
+ struct apic **drv;
+
+ for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
+ (*drv)->wakeup_secondary_cpu_64 = handler;
+}
+#endif
+
+/*
+ * Override the generic EOI implementation with an optimized version.
+ * Only called during early boot when only one CPU is active and with
+ * interrupts disabled, so we know this does not race with actual APIC driver
+ * use.
+ */
+void __init apic_set_eoi_cb(void (*eoi)(void))
+{
+ struct apic **drv;
+
+ for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
+ /* Should happen once for each apic */
+ WARN_ON((*drv)->eoi == eoi);
+ (*drv)->native_eoi = (*drv)->eoi;
+ (*drv)->eoi = eoi;
+ }
+}
--- a/arch/x86/kernel/apic/probe_32.c
+++ b/arch/x86/kernel/apic/probe_32.c
@@ -82,7 +82,7 @@ static int __init parse_apic(char *arg)

for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
if (!strcmp((*drv)->name, arg)) {
- apic = *drv;
+ apic_install_driver(*drv);
cmdline_apic = 1;
return 0;
}
@@ -129,7 +129,7 @@ void __init x86_32_probe_apic(void)

for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
if ((*drv)->probe()) {
- apic = *drv;
+ apic_install_driver(*drv);
break;
}
}
@@ -137,5 +137,4 @@ void __init x86_32_probe_apic(void)
if (drv == __apicdrivers_end)
panic("Didn't find an APIC driver");
}
- printk(KERN_INFO "Using APIC driver %s\n", apic->name);
}
--- a/arch/x86/kernel/apic/probe_64.c
+++ b/arch/x86/kernel/apic/probe_64.c
@@ -13,19 +13,6 @@

#include "local.h"

-static __init void apic_install_driver(struct apic *driver)
-{
- if (apic == driver)
- return;
-
- apic = driver;
-
- if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid)
- apic->max_apic_id = x2apic_max_apicid;
-
- pr_info("Switched APIC routing to %s:\n", apic->name);
-}
-
/* Select the appropriate APIC driver */
void __init x86_64_probe_apic(void)
{
--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -160,20 +160,16 @@ static struct apic xen_pv_apic = {

static void __init xen_apic_check(void)
{
- if (apic == &xen_pv_apic)
- return;
-
- pr_info("Switched APIC routing from %s to %s.\n", apic->name,
- xen_pv_apic.name);
- apic = &xen_pv_apic;
+ apic_install_driver(&xen_pv_apic);
}
+
void __init xen_init_apic(void)
{
x86_apic_ops.io_apic_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_pv_apic;
+ apic_install_driver(&xen_pv_apic);

x86_platform.apic_post_init = xen_apic_check;
}

2023-07-31 13:17:41

by Juergen Gross

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On 24.07.23 15:35, Thomas Gleixner wrote:
> In preparation for converting the hotpath APIC callbacks to static keys,
> provide common initialization inforastructure.
>
> Lift apic_install_drivers() from probe_64.c and convert all places which
> switch the apic instance by storing the pointer to use apic_install_driver()
> as a first step.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Acked-by: Peter Zijlstra (Intel) <[email protected]>
> ---
> arch/x86/include/asm/apic.h | 2 +
> arch/x86/kernel/apic/Makefile | 2 -
> arch/x86/kernel/apic/apic.c | 31 -----------------------
> arch/x86/kernel/apic/apic_flat_64.c | 6 ----
> arch/x86/kernel/apic/bigsmp_32.c | 6 +---
> arch/x86/kernel/apic/init.c | 47 ++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/apic/probe_32.c | 5 +--
> arch/x86/kernel/apic/probe_64.c | 13 ---------
> arch/x86/xen/apic.c | 10 ++-----
> 9 files changed, 59 insertions(+), 63 deletions(-)
>
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -344,6 +344,8 @@ extern int lapic_can_unplug_cpu(void);
>
> #ifdef CONFIG_X86_LOCAL_APIC
>
> +void __init apic_install_driver(struct apic *driver);
> +
> static inline u32 apic_read(u32 reg)
> {
> return apic->read(reg);
> --- a/arch/x86/kernel/apic/Makefile
> +++ b/arch/x86/kernel/apic/Makefile
> @@ -7,7 +7,7 @@
> # In particualr, smp_apic_timer_interrupt() is called in random places.
> KCOV_INSTRUMENT := n
>
> -obj-$(CONFIG_X86_LOCAL_APIC) += apic.o apic_common.o apic_noop.o ipi.o vector.o
> +obj-$(CONFIG_X86_LOCAL_APIC) += apic.o apic_common.o apic_noop.o ipi.o vector.o init.o
> obj-y += hw_nmi.o
>
> obj-$(CONFIG_X86_IO_APIC) += io_apic.o
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -236,8 +236,7 @@ static int modern_apic(void)
> */
> static void __init apic_disable(void)
> {
> - pr_info("APIC: switched to apic NOOP\n");
> - apic = &apic_noop;
> + apic_install_driver(&apic_noop);
> }
>
> void native_apic_icr_write(u32 low, u32 id)
> @@ -2486,34 +2485,6 @@ u32 x86_msi_msg_get_destid(struct msi_ms
> }
> EXPORT_SYMBOL_GPL(x86_msi_msg_get_destid);
>
> -#ifdef CONFIG_X86_64
> -void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
> -{
> - struct apic **drv;
> -
> - for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
> - (*drv)->wakeup_secondary_cpu_64 = handler;
> -}
> -#endif
> -
> -/*
> - * Override the generic EOI implementation with an optimized version.
> - * Only called during early boot when only one CPU is active and with
> - * interrupts disabled, so we know this does not race with actual APIC driver
> - * use.
> - */
> -void __init apic_set_eoi_cb(void (*eoi)(void))
> -{
> - struct apic **drv;
> -
> - for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
> - /* Should happen once for each apic */
> - WARN_ON((*drv)->eoi == eoi);
> - (*drv)->native_eoi = (*drv)->eoi;
> - (*drv)->eoi = eoi;
> - }
> -}
> -
> static void __init apic_bsp_up_setup(void)
> {
> #ifdef CONFIG_X86_64
> --- a/arch/x86/kernel/apic/apic_flat_64.c
> +++ b/arch/x86/kernel/apic/apic_flat_64.c
> @@ -143,11 +143,7 @@ static int physflat_acpi_madt_oem_check(
>
> static int physflat_probe(void)
> {
> - if (apic == &apic_physflat || num_possible_cpus() > 8 ||
> - jailhouse_paravirt())
> - return 1;
> -
> - return 0;
> + return apic == &apic_physflat || num_possible_cpus() > 8 || jailhouse_paravirt();
> }
>
> static struct apic apic_physflat __ro_after_init = {
> --- a/arch/x86/kernel/apic/bigsmp_32.c
> +++ b/arch/x86/kernel/apic/bigsmp_32.c
> @@ -119,10 +119,8 @@ bool __init apic_bigsmp_possible(bool cm
>
> void __init apic_bigsmp_force(void)
> {
> - if (apic != &apic_bigsmp) {
> - apic = &apic_bigsmp;
> - pr_info("Overriding APIC driver with bigsmp\n");
> - }
> + if (apic != &apic_bigsmp)
> + apic_install_driver(&apic_bigsmp);
> }
>
> apic_driver(apic_bigsmp);
> --- /dev/null
> +++ b/arch/x86/kernel/apic/init.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#define pr_fmt(fmt) "APIC: " fmt
> +
> +#include <asm/apic.h>
> +
> +#include "local.h"
> +
> +void __init apic_install_driver(struct apic *driver)
> +{
> + if (apic == driver)
> + return;
> +
> + apic = driver;
> +
> + if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid)
> + apic->max_apic_id = x2apic_max_apicid;
> +
> + pr_info("Switched APIC routing to: %s\n", driver->name);
> +}
> +
> +#ifdef CONFIG_X86_64
> +void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
> +{
> + struct apic **drv;
> +
> + for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
> + (*drv)->wakeup_secondary_cpu_64 = handler;
> +}
> +#endif
> +
> +/*
> + * Override the generic EOI implementation with an optimized version.
> + * Only called during early boot when only one CPU is active and with
> + * interrupts disabled, so we know this does not race with actual APIC driver
> + * use.
> + */
> +void __init apic_set_eoi_cb(void (*eoi)(void))
> +{
> + struct apic **drv;
> +
> + for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
> + /* Should happen once for each apic */
> + WARN_ON((*drv)->eoi == eoi);
> + (*drv)->native_eoi = (*drv)->eoi;
> + (*drv)->eoi = eoi;
> + }
> +}
> --- a/arch/x86/kernel/apic/probe_32.c
> +++ b/arch/x86/kernel/apic/probe_32.c
> @@ -82,7 +82,7 @@ static int __init parse_apic(char *arg)
>
> for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
> if (!strcmp((*drv)->name, arg)) {
> - apic = *drv;
> + apic_install_driver(*drv);
> cmdline_apic = 1;
> return 0;
> }
> @@ -129,7 +129,7 @@ void __init x86_32_probe_apic(void)
>
> for (drv = __apicdrivers; drv < __apicdrivers_end; drv++) {
> if ((*drv)->probe()) {
> - apic = *drv;
> + apic_install_driver(*drv);
> break;
> }
> }
> @@ -137,5 +137,4 @@ void __init x86_32_probe_apic(void)
> if (drv == __apicdrivers_end)
> panic("Didn't find an APIC driver");
> }
> - printk(KERN_INFO "Using APIC driver %s\n", apic->name);
> }
> --- a/arch/x86/kernel/apic/probe_64.c
> +++ b/arch/x86/kernel/apic/probe_64.c
> @@ -13,19 +13,6 @@
>
> #include "local.h"
>
> -static __init void apic_install_driver(struct apic *driver)
> -{
> - if (apic == driver)
> - return;
> -
> - apic = driver;
> -
> - if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid)
> - apic->max_apic_id = x2apic_max_apicid;
> -
> - pr_info("Switched APIC routing to %s:\n", apic->name);
> -}
> -
> /* Select the appropriate APIC driver */
> void __init x86_64_probe_apic(void)
> {
> --- a/arch/x86/xen/apic.c
> +++ b/arch/x86/xen/apic.c
> @@ -160,20 +160,16 @@ static struct apic xen_pv_apic = {
>
> static void __init xen_apic_check(void)
> {
> - if (apic == &xen_pv_apic)
> - return;
> -
> - pr_info("Switched APIC routing from %s to %s.\n", apic->name,
> - xen_pv_apic.name);
> - apic = &xen_pv_apic;
> + apic_install_driver(&xen_pv_apic);
> }
> +
> void __init xen_init_apic(void)
> {
> x86_apic_ops.io_apic_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_pv_apic;
> + apic_install_driver(&xen_pv_apic);

This is working, but it produces a WARN() splat when booting as an unprivileged
Xen PV guest from static_call patching (static_call_init() hasn't been called
yet).

The diff below on top is fixing the issue:

diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
index 1838aefc632f..84f24268670b 100644
--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -163,14 +163,18 @@ static void __init xen_apic_check(void)
apic_install_driver(&xen_pv_apic);
}

+void __init xen_apic_install(void)
+{
+ /*
+ * On PV guests the APIC CPUID bit is disabled so none of the
+ * routines end up executing.
+ */
+ apic_install_driver(&xen_pv_apic);
+}
+
void __init xen_init_apic(void)
{
x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
-
x86_platform.apic_post_init = xen_apic_check;
}
apic_driver(xen_pv_apic);
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index c6b42c66c60c..ff2d0754ce62 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -188,6 +188,9 @@ static void __init _get_smp_config(unsigned int early)
static void __init xen_pv_smp_prepare_boot_cpu(void)
{
BUG_ON(smp_processor_id() != 0);
+
+ xen_apic_install();
+
native_smp_prepare_boot_cpu();

if (!xen_feature(XENFEAT_writable_page_tables))
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 408a2aa66c69..217e4b625e4d 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -119,6 +119,7 @@ static inline void __init xen_init_vga(const struct
dom0_vga_console_info *info,
void xen_add_preferred_consoles(void);

void __init xen_init_apic(void);
+void __init xen_apic_install(void);

#ifdef CONFIG_XEN_EFI
extern void xen_efi_init(struct boot_params *boot_params);


Juergen

Attachments:

2023-07-31 14:11:16

by Thomas Gleixner

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On Mon, Jul 31 2023 at 13:31, Juergen Gross wrote:
> On 24.07.23 15:35, Thomas Gleixner wrote:
>> static void __init xen_apic_check(void)
>> {
>> - if (apic == &xen_pv_apic)
>> - return;
>> -
>> - pr_info("Switched APIC routing from %s to %s.\n", apic->name,
>> - xen_pv_apic.name);
>> - apic = &xen_pv_apic;
>> + apic_install_driver(&xen_pv_apic);
>> }
>> +
>> void __init xen_init_apic(void)
>> {
>> x86_apic_ops.io_apic_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_pv_apic;
>> + apic_install_driver(&xen_pv_apic);
>
> This is working, but it produces a WARN() splat when booting as an unprivileged
> Xen PV guest from static_call patching (static_call_init() hasn't been called
> yet).

Duh, yes. It's too early.

> The diff below on top is fixing the issue:
>
> diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
> index 1838aefc632f..84f24268670b 100644
> --- a/arch/x86/xen/apic.c
> +++ b/arch/x86/xen/apic.c
> @@ -163,14 +163,18 @@ static void __init xen_apic_check(void)
> apic_install_driver(&xen_pv_apic);
> }
>
> +void __init xen_apic_install(void)
> +{
> + /*
> + * On PV guests the APIC CPUID bit is disabled so none of the
> + * routines end up executing.
> + */
> + apic_install_driver(&xen_pv_apic);
> +}
> +
> void __init xen_init_apic(void)
> {
> x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
> -
> x86_platform.apic_post_init = xen_apic_check;
> }
> apic_driver(xen_pv_apic);

I wonder whether this explicit install is actually needed at all.
Shouldn't the driver be installed via the APIC probing mechanism
automagically?

Thanks,

tglx

2023-07-31 15:27:53

by Juergen Gross

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On 31.07.23 15:01, Thomas Gleixner wrote:
> On Mon, Jul 31 2023 at 13:31, Juergen Gross wrote:
>> On 24.07.23 15:35, Thomas Gleixner wrote:
>>> static void __init xen_apic_check(void)
>>> {
>>> - if (apic == &xen_pv_apic)
>>> - return;
>>> -
>>> - pr_info("Switched APIC routing from %s to %s.\n", apic->name,
>>> - xen_pv_apic.name);
>>> - apic = &xen_pv_apic;
>>> + apic_install_driver(&xen_pv_apic);
>>> }
>>> +
>>> void __init xen_init_apic(void)
>>> {
>>> x86_apic_ops.io_apic_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_pv_apic;
>>> + apic_install_driver(&xen_pv_apic);
>>
>> This is working, but it produces a WARN() splat when booting as an unprivileged
>> Xen PV guest from static_call patching (static_call_init() hasn't been called
>> yet).
>
> Duh, yes. It's too early.
>
>> The diff below on top is fixing the issue:
>>
>> diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
>> index 1838aefc632f..84f24268670b 100644
>> --- a/arch/x86/xen/apic.c
>> +++ b/arch/x86/xen/apic.c
>> @@ -163,14 +163,18 @@ static void __init xen_apic_check(void)
>> apic_install_driver(&xen_pv_apic);
>> }
>>
>> +void __init xen_apic_install(void)
>> +{
>> + /*
>> + * On PV guests the APIC CPUID bit is disabled so none of the
>> + * routines end up executing.
>> + */
>> + apic_install_driver(&xen_pv_apic);
>> +}
>> +
>> void __init xen_init_apic(void)
>> {
>> x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
>> -
>> x86_platform.apic_post_init = xen_apic_check;
>> }
>> apic_driver(xen_pv_apic);
>
> I wonder whether this explicit install is actually needed at all.
> Shouldn't the driver be installed via the APIC probing mechanism
> automagically?

Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is
a nop for Xen PV, but that can be changed. I'll have a look.


Juergen

Attachments:

2023-07-31 16:30:41

by Thomas Gleixner

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote:
> On 31.07.23 15:01, Thomas Gleixner wrote:
>>> apic_driver(xen_pv_apic);
>>
>> I wonder whether this explicit install is actually needed at all.
>> Shouldn't the driver be installed via the APIC probing mechanism
>> automagically?
>
> Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is
> a nop for Xen PV, but that can be changed. I'll have a look.

You could simply set that callback to default_setup_apic_routing() and
be done with it.

2023-08-01 07:52:10

by Juergen Gross

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On 01.08.23 08:49, Juergen Gross wrote:
> On 01.08.23 08:41, Thomas Gleixner wrote:
>> On Mon, Jul 31 2023 at 17:48, Thomas Gleixner wrote:
>>
>>> On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote:
>>>> On 31.07.23 15:01, Thomas Gleixner wrote:
>>>>>>     apic_driver(xen_pv_apic);
>>>>>
>>>>> I wonder whether this explicit install is actually needed at all.
>>>>> Shouldn't the driver be installed via the APIC probing mechanism
>>>>> automagically?
>>>>
>>>> Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is
>>>> a nop for Xen PV, but that can be changed. I'll have a look.
>>>
>>> You could simply set that callback to default_setup_apic_routing() and
>>> be done with it.
>>
>> Doesn't work because XEN overrides it already. So sure, lets just go
>
> It is overriding it with x86_init_noop().
>
>> with the solution you proposed. One more ugly or less in XEN/PV does not
>> really matter much :)
>>
>> Let me grab this and put it into the right position in the queue.
>
> Wait a few minutes, please. I'm just about to test your suggestion.

Using the following diff on top of your patch is working fine:

diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
index 804a26b7c85e..d7743ba0212d 100644
--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -166,11 +166,6 @@ static void __init xen_apic_check(void)
void __init xen_init_apic(void)
{
x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
-
x86_platform.apic_post_init = xen_apic_check;
}
apic_driver(xen_pv_apic);
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 93b658248d01..164e5be23a45 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1326,7 +1326,7 @@ asmlinkage __visible void __init xen_start_kernel(struct
start_info *si)

x86_init.resources.memory_setup = xen_memory_setup;
x86_init.irqs.intr_mode_select = x86_init_noop;
- x86_init.irqs.intr_mode_init = x86_init_noop;
+ x86_init.irqs.intr_mode_init = x86_64_probe_apic;
x86_init.oem.arch_setup = xen_arch_setup;
x86_init.oem.banner = xen_banner;
x86_init.hyper.init_platform = xen_pv_init_platform;


Juergen

Attachments:

2023-08-01 07:55:50

by Juergen Gross

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On 01.08.23 09:32, Thomas Gleixner wrote:
> On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote:
>> On 01.08.23 08:49, Juergen Gross wrote:
>> void __init xen_init_apic(void)
>> {
>> x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
>> -
>> x86_platform.apic_post_init = xen_apic_check;
>
> I don't think this one is needed.

Indeed.


Juergen

Attachments:

2023-08-01 08:14:32

by Thomas Gleixner

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On Mon, Jul 31 2023 at 17:48, Thomas Gleixner wrote:

> On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote:
>> On 31.07.23 15:01, Thomas Gleixner wrote:
>>>> apic_driver(xen_pv_apic);
>>>
>>> I wonder whether this explicit install is actually needed at all.
>>> Shouldn't the driver be installed via the APIC probing mechanism
>>> automagically?
>>
>> Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is
>> a nop for Xen PV, but that can be changed. I'll have a look.
>
> You could simply set that callback to default_setup_apic_routing() and
> be done with it.

Doesn't work because XEN overrides it already. So sure, lets just go
with the solution you proposed. One more ugly or less in XEN/PV does not
really matter much :)

Let me grab this and put it into the right position in the queue.

Thanks,

tglx

2023-08-01 08:22:14

by Juergen Gross

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On 01.08.23 08:41, Thomas Gleixner wrote:
> On Mon, Jul 31 2023 at 17:48, Thomas Gleixner wrote:
>
>> On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote:
>>> On 31.07.23 15:01, Thomas Gleixner wrote:
>>>>> apic_driver(xen_pv_apic);
>>>>
>>>> I wonder whether this explicit install is actually needed at all.
>>>> Shouldn't the driver be installed via the APIC probing mechanism
>>>> automagically?
>>>
>>> Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is
>>> a nop for Xen PV, but that can be changed. I'll have a look.
>>
>> You could simply set that callback to default_setup_apic_routing() and
>> be done with it.
>
> Doesn't work because XEN overrides it already. So sure, lets just go

It is overriding it with x86_init_noop().

> with the solution you proposed. One more ugly or less in XEN/PV does not
> really matter much :)
>
> Let me grab this and put it into the right position in the queue.

Wait a few minutes, please. I'm just about to test your suggestion.


Juergen

Attachments:

2023-08-01 08:26:52

by Thomas Gleixner

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On Tue, Aug 01 2023 at 08:41, Thomas Gleixner wrote:

> On Mon, Jul 31 2023 at 17:48, Thomas Gleixner wrote:
>
>> On Mon, Jul 31 2023 at 15:10, Juergen Gross wrote:
>>> On 31.07.23 15:01, Thomas Gleixner wrote:
>>>>> apic_driver(xen_pv_apic);
>>>>
>>>> I wonder whether this explicit install is actually needed at all.
>>>> Shouldn't the driver be installed via the APIC probing mechanism
>>>> automagically?
>>>
>>> Only in case x86_init.irq.intr_mode_init is set appropriately. Today it is
>>> a nop for Xen PV, but that can be changed. I'll have a look.
>>
>> You could simply set that callback to default_setup_apic_routing() and
>> be done with it.
>
> Doesn't work because XEN overrides it already. So sure, lets just go
> with the solution you proposed. One more ugly or less in XEN/PV does not
> really matter much :)
>
> Let me grab this and put it into the right position in the queue.

Spoke too early. The below should work AFAICT from the code.


--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -99,10 +99,7 @@ static void xen_apic_icr_write(u32 low,

static int xen_apic_probe_pv(void)
{
- if (xen_pv_domain())
- return 1;
-
- return 0;
+ return xen_pv_domain();
}

static int xen_madt_oem_check(char *oem_id, char *oem_table_id)
@@ -157,20 +154,14 @@ static struct apic xen_pv_apic = {
.icr_read = xen_apic_icr_read,
.icr_write = xen_apic_icr_write,
};
-
-static void __init xen_apic_check(void)
-{
- apic_install_driver(&xen_pv_apic);
-}
+apic_driver(xen_pv_apic);

void __init xen_init_apic(void)
{
x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
-
- x86_platform.apic_post_init = xen_apic_check;
+ /*
+ * On PV guests the APIC CPUID bit is disabled so none of the
+ * routines end up executing.
+ */
+ apic_install_driver(&xen_pv_apic);
}
-apic_driver(xen_pv_apic);
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1325,7 +1325,7 @@ asmlinkage __visible void __init xen_sta
x86_platform.realmode_init = x86_init_noop;

x86_init.resources.memory_setup = xen_memory_setup;
- x86_init.irqs.intr_mode_select = x86_init_noop;
+ x86_init.irqs.intr_mode_select = xen_init_apic;
x86_init.irqs.intr_mode_init = x86_init_noop;
x86_init.oem.arch_setup = xen_arch_setup;
x86_init.oem.banner = xen_banner;
@@ -1366,13 +1366,6 @@ asmlinkage __visible void __init xen_sta

xen_init_capabilities();

-#ifdef CONFIG_X86_LOCAL_APIC
- /*
- * set up the basic apic ops.
- */
- xen_init_apic();
-#endif
-
machine_ops = xen_machine_ops;

/*
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -118,7 +118,11 @@ static inline void __init xen_init_vga(c

void xen_add_preferred_consoles(void);

+#ifdef CONFIG_X86_LOCAL_APIC
void __init xen_init_apic(void);
+#else
+# define xen_init_apic x86_init_noop
+#endif

#ifdef CONFIG_XEN_EFI
extern void xen_efi_init(struct boot_params *boot_params);

2023-08-01 08:55:46

by Thomas Gleixner

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On Tue, Aug 01 2023 at 09:37, Juergen Gross wrote:
> On 01.08.23 09:32, Thomas Gleixner wrote:
>> On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote:
>>> On 01.08.23 08:49, Juergen Gross wrote:
>>> void __init xen_init_apic(void)
>>> {
>>> x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
>>> -
>>> x86_platform.apic_post_init = xen_apic_check;
>>
>> I don't think this one is needed.
>
> Indeed.

Can you send a real patch please which I can add to that pile at the
right place?

Thanks,

tglx

2023-08-01 09:02:16

by Juergen Gross

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On 01.08.23 10:23, Thomas Gleixner wrote:
> On Tue, Aug 01 2023 at 09:37, Juergen Gross wrote:
>> On 01.08.23 09:32, Thomas Gleixner wrote:
>>> On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote:
>>>> On 01.08.23 08:49, Juergen Gross wrote:
>>>> void __init xen_init_apic(void)
>>>> {
>>>> x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
>>>> -
>>>> x86_platform.apic_post_init = xen_apic_check;
>>>
>>> I don't think this one is needed.
>>
>> Indeed.
>
> Can you send a real patch please which I can add to that pile at the
> right place?

I think adding it right after patch 50 should be fine?

The WARN() will be issued only with patch 58.


Juergen

Attachments:

2023-08-01 09:26:00

by Thomas Gleixner

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote:
> On 01.08.23 08:49, Juergen Gross wrote:
> void __init xen_init_apic(void)
> {
> x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
> -
> x86_platform.apic_post_init = xen_apic_check;

I don't think this one is needed.

2023-08-01 10:19:08

by Juergen Gross

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On 01.08.23 10:54, Thomas Gleixner wrote:
> On Tue, Aug 01 2023 at 10:25, Juergen Gross wrote:
>> On 01.08.23 10:23, Thomas Gleixner wrote:
>>> On Tue, Aug 01 2023 at 09:37, Juergen Gross wrote:
>>>> On 01.08.23 09:32, Thomas Gleixner wrote:
>>>>> On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote:
>>>>>> On 01.08.23 08:49, Juergen Gross wrote:
>>>>>> void __init xen_init_apic(void)
>>>>>> {
>>>>>> x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
>>>>>> -
>>>>>> x86_platform.apic_post_init = xen_apic_check;
>>>>>
>>>>> I don't think this one is needed.
>>>>
>>>> Indeed.
>>>
>>> Can you send a real patch please which I can add to that pile at the
>>> right place?
>>
>> I think adding it right after patch 50 should be fine?
>>
>> The WARN() will be issued only with patch 58.
>
> Correct.

Here it is.


Juergen

Attachments:

2023-08-01 11:18:42

by Thomas Gleixner

Subject: Re: [patch V2 50/58] x86/apic: Provide common init infrastructure

On Tue, Aug 01 2023 at 10:25, Juergen Gross wrote:
> On 01.08.23 10:23, Thomas Gleixner wrote:
>> On Tue, Aug 01 2023 at 09:37, Juergen Gross wrote:
>>> On 01.08.23 09:32, Thomas Gleixner wrote:
>>>> On Tue, Aug 01 2023 at 09:08, Juergen Gross wrote:
>>>>> On 01.08.23 08:49, Juergen Gross wrote:
>>>>> void __init xen_init_apic(void)
>>>>> {
>>>>> x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
>>>>> -
>>>>> x86_platform.apic_post_init = xen_apic_check;
>>>>
>>>> I don't think this one is needed.
>>>
>>> Indeed.
>>
>> Can you send a real patch please which I can add to that pile at the
>> right place?
>
> I think adding it right after patch 50 should be fine?
>
> The WARN() will be issued only with patch 58.

Correct.

2023-08-09 22:13:48

by tip-bot2 for Jacob Pan

Subject: [tip: x86/apic] x86/xen/apic: Use standard apic driver mechanism for Xen PV

The following commit has been merged into the x86/apic branch of tip:

Commit-ID: 3b5244bef15e0ec2b51ae5ea4182e1b674d01551
Gitweb: https://git.kernel.org/tip/3b5244bef15e0ec2b51ae5ea4182e1b674d01551
Author: Juergen Gross <[email protected]>
AuthorDate: Tue, 08 Aug 2023 15:04:18 -07:00
Committer: Dave Hansen <[email protected]>
CommitterDate: Wed, 09 Aug 2023 12:00:41 -07:00

x86/xen/apic: Use standard apic driver mechanism for Xen PV

Instead of setting the Xen PV apic driver very early during boot, just use
the standard apic driver probing by setting an appropriate
x86_init.irqs.intr_mode_init callback.

At the same time eliminate xen_apic_check() which has never been used.

The #ifdef CONFIG_X86_LOCAL_APIC around the call of xen_init_apic()
can be removed, too, as CONFIG_XEN depends on CONFIG_X86_LOCAL_APIC.

Signed-off-by: Juergen Gross <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: Dave Hansen <[email protected]>
Tested-by: Michael Kelley <[email protected]>
Tested-by: Sohil Mehta <[email protected]>
Tested-by: Juergen Gross <[email protected]> # Xen PV (dom0 and unpriv. guest)
Link: https://lore.kernel.org/lkml/[email protected]
---
arch/x86/xen/apic.c | 11 -----------
arch/x86/xen/enlighten_pv.c | 4 +---
2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/arch/x86/xen/apic.c b/arch/x86/xen/apic.c
index 1838aef..315ffd8 100644
--- a/arch/x86/xen/apic.c
+++ b/arch/x86/xen/apic.c
@@ -158,19 +158,8 @@ static struct apic xen_pv_apic = {
.icr_write = xen_apic_icr_write,
};

-static void __init xen_apic_check(void)
-{
- apic_install_driver(&xen_pv_apic);
-}
-
void __init xen_init_apic(void)
{
x86_apic_ops.io_apic_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_install_driver(&xen_pv_apic);
-
- x86_platform.apic_post_init = xen_apic_check;
}
apic_driver(xen_pv_apic);
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 93b6582..c393c44 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1326,7 +1326,7 @@ asmlinkage __visible void __init xen_start_kernel(struct start_info *si)

x86_init.resources.memory_setup = xen_memory_setup;
x86_init.irqs.intr_mode_select = x86_init_noop;
- x86_init.irqs.intr_mode_init = x86_init_noop;
+ x86_init.irqs.intr_mode_init = x86_64_probe_apic;
x86_init.oem.arch_setup = xen_arch_setup;
x86_init.oem.banner = xen_banner;
x86_init.hyper.init_platform = xen_pv_init_platform;
@@ -1366,12 +1366,10 @@ asmlinkage __visible void __init xen_start_kernel(struct start_info *si)

xen_init_capabilities();

-#ifdef CONFIG_X86_LOCAL_APIC
/*
* set up the basic apic ops.
*/
xen_init_apic();
-#endif

machine_ops = xen_machine_ops;