RE: [patch 51/58] x86/apic: Provide apic_update_callback()

[Michael Kelley (LINUX)]

From: Thomas Gleixner <tglx@xxxxxxxxxxxxx> Sent: Monday, July 17, 2023 4:16 PM
> 
> There are already two variants of update mechanism for particular callbacks
> and virtualization just writes into the data structure.
> 
> Provide an interface and use a shadow data structure to preserve callbacks
> so they can be reapplied when the APIC driver is replaced.
> 
> The extra data structure is intentional as any new callback needs to be
> also updated in the core code. This also prepares for static calls.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  arch/x86/include/asm/apic.h |   25 +++++++++++++++++++++++++
>  arch/x86/kernel/apic/init.c |   39 ++++++++++++++++++++++++++++++++++++++-
>  arch/x86/kernel/setup.c     |    2 ++
>  3 files changed, 65 insertions(+), 1 deletion(-)
> 
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -308,6 +308,23 @@ struct apic {
>  	char	*name;
>  };
> 
> +struct apic_override {
> +	void	(*eoi)(void);
> +	void	(*native_eoi)(void);
> +	void	(*write)(u32 reg, u32 v);
> +	u32	(*read)(u32 reg);
> +	void	(*send_IPI)(int cpu, int vector);
> +	void	(*send_IPI_mask)(const struct cpumask *mask, int vector);
> +	void	(*send_IPI_mask_allbutself)(const struct cpumask *msk, int vec);
> +	void	(*send_IPI_allbutself)(int vector);
> +	void	(*send_IPI_all)(int vector);
> +	void	(*send_IPI_self)(int vector);
> +	u64	(*icr_read)(void);
> +	void	(*icr_write)(u32 low, u32 high);
> +	int	(*wakeup_secondary_cpu)(int apicid, unsigned long start_eip);
> +	int	(*wakeup_secondary_cpu_64)(int apicid, unsigned long start_eip);
> +};
> +
>  /*
>   * Pointer to the local APIC driver in use on this system (there's
>   * always just one such driver in use - the kernel decides via an
> @@ -343,9 +360,17 @@ extern int lapic_can_unplug_cpu(void);
>  #endif
> 
>  #ifdef CONFIG_X86_LOCAL_APIC
> +extern struct apic_override __x86_apic_override;
> 
> +void __init apic_setup_apic_calls(void);
>  void __init apic_install_driver(struct apic *driver);
> 
> +#define apic_update_callback(_callback, _fn) {					\
> +		__x86_apic_override._callback = _fn;				\
> +		apic->_callback = _fn;						\
> +		pr_info("APIC::%s() replaced with %ps()\n", #_callback, _fn);	\

FWIW, when I saw the dmesg output, I thought the double colon in the above
message was a typo.  When juxtaposed against the nearly adjacent APIC-related
messages that have just a single colon, it's a little unexpected.

[    0.914587] APIC: Switch to symmetric I/O mode setup
[    0.918289] APIC: Switched APIC routing to: physical flat
[    0.925577] Hyper-V: Using IPI hypercalls
[    0.928460] APIC::send_IPI() replaced with hv_send_ipi()
[    0.931862] APIC::send_IPI_mask() replaced with hv_send_ipi_mask()
[    0.935960] APIC::send_IPI_mask_allbutself() replaced with hv_send_ipi_mask_allbutself()

Just my $.02 ....

Michael

> +}
> +
>  static inline u32 apic_read(u32 reg)
>  {
>  	return apic->read(reg);
> --- a/arch/x86/kernel/apic/init.c
> +++ b/arch/x86/kernel/apic/init.c
> @@ -5,6 +5,37 @@
> 
>  #include "local.h"
> 
> +/* The container for function call overrides */
> +struct apic_override __x86_apic_override __initdata;
> +
> +#define apply_override(__cb)					\
> +	if (__x86_apic_override.__cb)				\
> +		apic->__cb = __x86_apic_override.__cb
> +
> +static __init void restore_override_callbacks(void)
> +{
> +	apply_override(eoi);
> +	apply_override(native_eoi);
> +	apply_override(write);
> +	apply_override(read);
> +	apply_override(send_IPI);
> +	apply_override(send_IPI_mask);
> +	apply_override(send_IPI_mask_allbutself);
> +	apply_override(send_IPI_allbutself);
> +	apply_override(send_IPI_all);
> +	apply_override(send_IPI_self);
> +	apply_override(icr_read);
> +	apply_override(icr_write);
> +	apply_override(wakeup_secondary_cpu);
> +	apply_override(wakeup_secondary_cpu_64);
> +}
> +
> +void __init apic_setup_apic_calls(void)
> +{
> +	/* Ensure that the default APIC has native_eoi populated */
> +	apic->native_eoi = apic->eoi;
> +}
> +
>  void __init apic_install_driver(struct apic *driver)
>  {
>  	if (apic == driver)
> @@ -15,6 +46,13 @@ void __init apic_install_driver(struct a
>  	if (IS_ENABLED(CONFIG_X86_X2APIC) && apic->x2apic_set_max_apicid)
>  		apic->max_apic_id = x2apic_max_apicid;
> 
> +	/* Copy the original eoi() callback as KVM/HyperV might overwrite it */
> +	if (!apic->native_eoi)
> +		apic->native_eoi = apic->eoi;
> +
> +	/* Apply any already installed callback overrides */
> +	restore_override_callbacks();
> +
>  	pr_info("Switched APIC routing to: %s\n", driver->name);
>  }
> 
> @@ -41,7 +79,6 @@ void __init apic_set_eoi_cb(void (*eoi)(
>  	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/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1018,6 +1018,8 @@ void __init setup_arch(char **cmdline_p)
> 
>  	x86_report_nx();
> 
> +	apic_setup_apic_calls();
> +
>  	if (acpi_mps_check()) {
>  #ifdef CONFIG_X86_LOCAL_APIC
>  		apic_is_disabled = true;