[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 <tglx@linutronix.de>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 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(-)
  

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 <tglx@linutronix.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>   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
  

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
  

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
  

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.
  

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
  

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
  

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
  

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

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.
  

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
  

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
  

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
  

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.
  

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