DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A72BB356D8
Date: Wed, 6 Sep 2017 12:25:14 +0800
From: Baoquan He <bhe@redhat.com>
To: Dou Liyang <douly.fnst@cn.fujitsu.com>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org, tglx@linutronix.de,
        mingo@kernel.org, hpa@zytor.com, rjw@rjwysocki.net, bp@alien8.de,
        indou.takao@jp.fujitsu.com, izumi.taku@jp.fujitsu.com
Subject: Re: [PATCH v8 06/13] x86/apic: Mark the apic_intr_mode extern for
 sanity check cleanup
Message-ID: <20170906042514.GF30906@x1>
References: <1503890438-27840-1-git-send-email-douly.fnst@cn.fujitsu.com>
 <1503890438-27840-7-git-send-email-douly.fnst@cn.fujitsu.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1503890438-27840-7-git-send-email-douly.fnst@cn.fujitsu.com>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4934
Lines: 181

On 08/28/17 at 11:20am, Dou Liyang wrote:
> Calling native_smp_prepare_cpus() to prepare for SMP bootup, does
> some sanity checking, enables APIC mode and disables SMP feature.
> 
> Now, APIC mode setup has been unified to apic_intr_mode_init(),
> some sanity checks are redundant and need to be cleanup.
> 
> Mark the apic_intr_mode extern to refine the switch and remove
> the redundant sanity check.
> 
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> ---
>  arch/x86/include/asm/apic.h |  9 +++++++
>  arch/x86/kernel/apic/apic.c | 12 ++++------
>  arch/x86/kernel/smpboot.c   | 57 +++++++--------------------------------------
>  3 files changed, 22 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
> index 4e550c7..01f3fc8 100644
> --- a/arch/x86/include/asm/apic.h
> +++ b/arch/x86/include/asm/apic.h
> @@ -53,6 +53,15 @@ extern int local_apic_timer_c2_ok;
>  extern int disable_apic;
>  extern unsigned int lapic_timer_frequency;
>  
> +extern enum apic_intr_mode_id apic_intr_mode;
> +enum apic_intr_mode_id {
> +	APIC_PIC,
> +	APIC_VIRTUAL_WIRE,
> +	APIC_VIRTUAL_WIRE_NO_CONFIG,
> +	APIC_SYMMETRIC_IO,
> +	APIC_SYMMETRIC_IO_NO_ROUTING
> +};
> +
>  #ifdef CONFIG_SMP
>  extern void __inquire_remote_apic(int apicid);
>  #else /* CONFIG_SMP */
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 9038c5f..97bd47b 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1235,13 +1235,7 @@ void __init sync_Arb_IDs(void)
>  			APIC_INT_LEVELTRIG | APIC_DM_INIT);
>  }
>  
> -enum apic_intr_mode {
> -	APIC_PIC,
> -	APIC_VIRTUAL_WIRE,
> -	APIC_VIRTUAL_WIRE_NO_CONFIG,
> -	APIC_SYMMETRIC_IO,
> -	APIC_SYMMETRIC_IO_NO_ROUTING,
> -};
> +enum apic_intr_mode_id apic_intr_mode;
>  
>  static int __init apic_intr_mode_select(void)
>  {
> @@ -1367,7 +1361,9 @@ void __init apic_intr_mode_init(void)
>  {
>  	bool upmode = false;
>  
> -	switch (apic_intr_mode_select()) {
> +	apic_intr_mode = apic_intr_mode_select();
> +
> +	switch (apic_intr_mode) {
>  	case APIC_PIC:
>  		pr_info("APIC: Keep in PIC mode(8259)\n");
>  		return;
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 8301b75..d2160f7 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1187,17 +1187,10 @@ static __init void disable_smp(void)
>  	cpumask_set_cpu(0, topology_core_cpumask(0));
>  }
>  
> -enum {
> -	SMP_OK,
> -	SMP_NO_CONFIG,
> -	SMP_NO_APIC,
> -	SMP_FORCE_UP,
> -};
> -
>  /*
>   * Various sanity checks.
>   */
> -static int __init smp_sanity_check(unsigned max_cpus)
> +static void __init smp_sanity_check(void)
>  {
>  	preempt_disable();
>  
> @@ -1235,16 +1228,6 @@ static int __init smp_sanity_check(unsigned max_cpus)
>  	}
>  
>  	/*
> -	 * If we couldn't find an SMP configuration at boot time,
> -	 * get out of here now!
> -	 */
> -	if (!smp_found_config && !acpi_lapic) {
> -		preempt_enable();
> -		pr_notice("SMP motherboard not detected\n");
> -		return SMP_NO_CONFIG;
> -	}
> -
> -	/*
>  	 * Should not be necessary because the MP table should list the boot
>  	 * CPU too, but we do it for the sake of robustness anyway.
>  	 */
> @@ -1254,29 +1237,6 @@ static int __init smp_sanity_check(unsigned max_cpus)
>  		physid_set(hard_smp_processor_id(), phys_cpu_present_map);
>  	}
>  	preempt_enable();
> -
> -	/*
> -	 * If we couldn't find a local APIC, then get out of here now!
> -	 */
> -	if (APIC_INTEGRATED(boot_cpu_apic_version) &&
> -	    !boot_cpu_has(X86_FEATURE_APIC)) {
> -		if (!disable_apic) {
> -			pr_err("BIOS bug, local APIC #%d not detected!...\n",
> -				boot_cpu_physical_apicid);
> -			pr_err("... forcing use of dummy APIC emulation (tell your hw vendor)\n");
> -		}
> -		return SMP_NO_APIC;
> -	}
> -
> -	/*
> -	 * If SMP should be disabled, then really disable it!
> -	 */
> -	if (!max_cpus) {
> -		pr_info("SMP mode deactivated\n");
> -		return SMP_FORCE_UP;
> -	}
> -
> -	return SMP_OK;
>  }
>  
>  static void __init smp_cpu_index_default(void)
> @@ -1335,19 +1295,20 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)

Please also cleanup the passed in max_cpus since it's not used here any
more. And up to the caller:

static noinline void __init kernel_init_freeable(void)
{
	...
	smp_prepare_cpus(setup_max_cpus);
	...
}

>  
>  	apic_intr_mode_init();
>  
> -	switch (smp_sanity_check(max_cpus)) {
> -	case SMP_NO_CONFIG:
> -		disable_smp();
> -		return;
> -	case SMP_NO_APIC:
> +	smp_sanity_check();
> +
> +	switch (apic_intr_mode) {
> +	case APIC_PIC:
> +	case APIC_VIRTUAL_WIRE_NO_CONFIG:
>  		disable_smp();
>  		return;
> -	case SMP_FORCE_UP:
> +	case APIC_SYMMETRIC_IO_NO_ROUTING:
>  		disable_smp();
>  		/* Setup local timer */
>  		x86_init.timers.setup_percpu_clockev();
>  		return;
> -	case SMP_OK:
> +	case APIC_VIRTUAL_WIRE:
> +	case APIC_SYMMETRIC_IO:
>  		break;
>  	}
>  
> -- 
> 2.5.5
> 
> 
>