Date: Wed, 17 Jan 2018 17:31:05 +0800
From: Baoquan He <bhe@redhat.com>
To: linux-kernel@vger.kernel.org, ebiederm@xmission.com
Cc: mingo@redhat.com, tglx@linutronix.de, hpa@zytor.com,
        x86@kernel.org, douly.fnst@cn.fujitsu.com, rostedt@goodmis.org,
        jgross@suse.com, peterz@infradead.org, uobergfe@redhat.com,
        joro@8bytes.org
Subject: Re: [RESEND PATCH 3/3] x86/apic: Clean up the names of legacy irq
 mode setting related functions
Message-ID: <20180117093105.GD2321@localhost.localdomain>
References: <1515123732-28908-1-git-send-email-bhe@redhat.com>
 <20180105043929.GL7235@x1>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20180105043929.GL7235@x1>
User-Agent: Mutt/1.9.1 (2017-09-22)
Sender: linux-kernel-owner@vger.kernel.org

CC Eric

On 01/05/18 at 12:39pm, Baoquan He wrote:
> X86 MP spec defines 3 different interrupt modes:
>   1) PIC Mode—bypasses all APIC components and forces the system to
>      operate in single-processor mode.
>   2) Virtual Wire Mode—uses an APIC as a virtual wire, but otherwise
>      operates the same as PIC Mode.
>   3) Symmetric I/O Mode—enables the system to operate with more than
>      one processor.
> 
> The current disconnect_bsp_APIC includes two parts: one is to set system
> as PIC mode if it's available, the other is to change system back to
> Virtual Wire mode. Only PIC mode will detach the APIC from the interrupt
> system, Virtual Wire mode doesn't.
> 
> Besides Virutal Wire mode has two kinds: one is only setting Local APIC
> as Virtual Wire mode and interrupts are delivered from the PIC to the
> CPU which Local APIC connected to, the other is both Loca APIC and IO-APIC
> need be set as Virtual Wire mode.
> 
> So based on above knowledge, take IO-APIC Virtual Wire mode setting code
> out and wrap it inot a new function ioapic_set_virtual_wire_mode. Meanwhile
> change the name of disconnect_bsp_APIC as lapic_set_legacy_irq_mode. These
> makes the legacy irq mode setting more understandable.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  arch/x86/include/asm/apic.h    |  2 +-
>  arch/x86/include/asm/io_apic.h |  5 ++---
>  arch/x86/kernel/apic/apic.c    | 11 ++++++-----
>  arch/x86/kernel/apic/io_apic.c | 17 ++++++++++-------
>  arch/x86/kernel/x86_init.c     |  2 +-
>  drivers/iommu/irq_remapping.c  |  2 +-
>  6 files changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index a9e57f08bfa6..004c48bc8bc8 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -132,7 +132,7 @@ extern int get_physical_broadcast(void);
>  
>  extern int lapic_get_maxlvt(void);
>  extern void clear_local_APIC(void);
> -extern void disconnect_bsp_APIC(int virt_wire_setup);
> +extern void lapic_set_legacy_irq_mode(int virt_wire_setup);
>  extern void disable_local_APIC(void);
>  extern void lapic_shutdown(void);
>  extern void sync_Arb_IDs(void);
> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> index e38ad3863a2c..6800dcea1d21 100644
> --- a/arch/x86/include/asm/io_apic.h
> +++ b/arch/x86/include/asm/io_apic.h
> @@ -183,7 +183,7 @@ extern void disable_ioapic_support(void);
>  
>  extern void __init io_apic_init_mappings(void);
>  extern unsigned int native_io_apic_read(unsigned int apic, unsigned int reg);
> -extern void native_disable_io_apic(void);
> +extern void switch_to_legacy_irq_mode(void);
>  
>  static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
>  {
> @@ -193,7 +193,6 @@ static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
>  extern void setup_IO_APIC(void);
>  extern void enable_IO_APIC(void);
>  extern void clear_IO_APIC (void);
> -extern void switch_to_legacy_irq_mode(void);
>  extern int IO_APIC_get_PCI_irq_vector(int bus, int devfn, int pin);
>  extern void print_IO_APICs(void);
>  #else  /* !CONFIG_X86_IO_APIC */
> @@ -229,7 +228,7 @@ static inline void mp_save_irq(struct mpc_intsrc *m) { }
>  static inline void disable_ioapic_support(void) { }
>  static inline void io_apic_init_mappings(void) { }
>  #define native_io_apic_read		NULL
> -#define native_disable_io_apic		NULL
> +#define switch_to_legacy_irq_mode	NULL
>  
>  static inline void setup_IO_APIC(void) { }
>  static inline void enable_IO_APIC(void) { }
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 7e613fb90630..301d90d4a0c3 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -2050,13 +2050,14 @@ static void __init connect_bsp_APIC(void)
>  }
>  
>  /**
> - * disconnect_bsp_APIC - detach the APIC from the interrupt system
> - * @virt_wire_setup:	indicates, whether virtual wire mode is selected
> + * lapic_set_legacy_irq_mode - switch Local APIC back to be legacy irq mode.
> + * @virt_wire_setup:   indicates, whether virtual wire mode is selected
>   *
> - * Virtual wire mode is necessary to deliver legacy interrupts even when the
> - * APIC is disabled.
> + * If PIC mode is available, LAPIC need be disconnected with CPU. Otherwise
> + * enable LAPIC and set it to be virtual wire mode. However if IO-APIC has
> + * been virtual wire mode, LVT0 of LAPIC need be masked.
>   */
> -void disconnect_bsp_APIC(int virt_wire_setup)
> +void lapic_set_legacy_irq_mode(int virt_wire_setup)
>  {
>  	unsigned int value;
>  
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index a47aa915d18c..b7fd4236b0e5 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -1410,7 +1410,7 @@ void __init enable_IO_APIC(void)
>  	clear_IO_APIC();
>  }
>  
> -void native_disable_io_apic(void)
> +static void ioapic_set_virtual_wire_mode(void)
>  {
>  	/*
>  	 * If the i8259 is routed through an IOAPIC
> @@ -1433,21 +1433,24 @@ void native_disable_io_apic(void)
>  		 */
>  		ioapic_write_entry(ioapic_i8259.apic, ioapic_i8259.pin, entry);
>  	}
> -
> -	if (boot_cpu_has(X86_FEATURE_APIC) || apic_from_smp_config())
> -		disconnect_bsp_APIC(ioapic_i8259.pin != -1);
>  }
>  
>  /*
> - * Not an __init, needed by kexec/kdump code.
> - * For safety IO-APIC and Local APIC need be cleared before this.
> + * In legacy irq mode, full DOS compatibility with the uniprocessor PC/AT is
> + * provided by using the APICs in conjunction with standard 8259A-equivalent
> + * programmable interrupt controllers (PICs). It's necessary to deliver legacy
> + * interrupts even when APIC mode is not enabled. This is required by kexec/
> + * kdump before enter into the 2nd kernel.
>   */
>  void switch_to_legacy_irq_mode(void)
>  {
>  	if (!nr_legacy_irqs())
>  		return;
>  
> -	x86_io_apic_ops.disable();
> +	ioapic_set_virtual_wire_mode();
> +
> +	if (boot_cpu_has(X86_FEATURE_APIC) || apic_from_smp_config())
> +		lapic_set_legacy_irq_mode(ioapic_i8259.pin != -1);
>  }
>  
>  #ifdef CONFIG_X86_32
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index 1151ccd72ce9..c30f0f273dbd 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -148,5 +148,5 @@ void arch_restore_msi_irqs(struct pci_dev *dev)
>  
>  struct x86_io_apic_ops x86_io_apic_ops __ro_after_init = {
>  	.read			= native_io_apic_read,
> -	.disable		= native_disable_io_apic,
> +	.disable		= switch_to_legacy_irq_mode,
>  };
> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> index 49721b4e1975..751472ddf536 100644
> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -37,7 +37,7 @@ static void irq_remapping_disable_io_apic(void)
>  	 * now.
>  	 */
>  	if (boot_cpu_has(X86_FEATURE_APIC) || apic_from_smp_config())
> -		disconnect_bsp_APIC(0);
> +		lapic_set_legacy_irq_mode(0);
>  }
>  
>  static void __init irq_remapping_modify_x86_ops(void)
> -- 
> 2.5.5
>