Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753377AbdIGETh (ORCPT ); Thu, 7 Sep 2017 00:19:37 -0400 Received: from mail.cn.fujitsu.com ([183.91.158.132]:22103 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752142AbdIGETg (ORCPT ); Thu, 7 Sep 2017 00:19:36 -0400 X-IronPort-AV: E=Sophos;i="5.42,356,1500912000"; d="scan'208";a="25361994" Subject: Re: [PATCH v8 01/13] x86/apic: Construct a selector for the interrupt delivery mode To: Baoquan He References: <1503890438-27840-1-git-send-email-douly.fnst@cn.fujitsu.com> <1503890438-27840-2-git-send-email-douly.fnst@cn.fujitsu.com> <20170906101728.GM30906@x1> CC: , , , , , , , , From: Dou Liyang Message-ID: <9f9f7477-01e9-cd16-47a2-e7ce13789e50@cn.fujitsu.com> Date: Thu, 7 Sep 2017 12:19:31 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170906101728.GM30906@x1> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.167.226.106] X-yoursite-MailScanner-ID: 2A5664725566.A831A X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: douly.fnst@cn.fujitsu.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4631 Lines: 178 Hi Baoquan I am wordy one ah: our target is checking if BIOS supports APIC, no matter what type(separated/integrated) it is. if not, go to PIC mode. Let‘s discuss the original logic and the smp_found_config, then take about your code. The existing logic is: if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) ...(1) return -1; if (!boot_cpu_has(X86_FEATURE_APIC) && APIC_INTEGRATED(boot_cpu_apic_version)) { ...(2) pr_err(....); why smp_found_config has to be checked in (1)? Because, In case of discrete (pretty old) apics we may not set X86_FEATURE_APIC bit in cpuid, with 82489DX we can't rely on apic feature bit retrieved via cpuid(boot_cpu_has(X86_FEATURE_APIC)).[1] So we assume that if SMP configuration is found from MP table (smp_found_config = 1) in above case, there maybe a separated chip in our pc. After passing the check of (1), we in (2), check whether local APIC is detected or not, If we have a BIOS bug. [1] Commit 8312136fa8b0("x86, apic: Fix missed handling of discrete apics") At 09/06/2017 06:17 PM, Baoquan He wrote: > Hi Dou, > > On 08/28/17 at 11:20am, Dou Liyang wrote: >> +static int __init apic_intr_mode_select(void) >> +{ >> + /* Check kernel option */ >> + if (disable_apic) { >> + pr_info("APIC disabled via kernel command line\n"); >> + return APIC_PIC; >> + } >> + > > I am not very familiar with cpu registers, not sure if we can adjust > below code flow as: > > /* If APIC is integrated, check local APIC only */ > if (lapic_is_integrated() && !boot_cpu_has(X86_FEATURE_APIC)) { > disable_apic = 1; > pr_info("APIC disabled by BIOS\n"); > return APIC_PIC; > } > > /* If APIC is on a separate chip, check if smp_found_config is found*/ > if (!lapic_is_integrated() && !smp_found_config) { > disable_apic = 1; > return APIC_PIC; > } Yes, Awesome, we first consider it from APIC register space, then the BOIS and software configration. let me do more investigation. I rewrite it based on you, any comments will welcome. /* If APIC is not integrated, check if SMP configuration is * found from MP table. If not too, no 82489DX. switch to * PIC mode * * Else APIC is integrated, check if the BIOS allows local APIC * */ if (!lapic_is_integrated()) { if (!smp_found_config) { disable_apic = 1; return APIC_PIC; } } else if(!boot_cpu_has(X86_FEATURE_APIC)) { disable_apic = 1; pr_info("APIC disabled by BIOS\n"); return APIC_PIC; } } BTW, As the macro APIC_INTEGRATED(x) has already wrapped by CONFIG_X86_32, I will cleanup the lapic_is_integrated() for readablity like that: diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c index 7834f73..63b3ae9 100644 --- a/arch/x86/kernel/apic/apic.c +++ b/arch/x86/kernel/apic/apic.c @@ -211,11 +211,7 @@ static inline int lapic_get_version(void) */ static inline int lapic_is_integrated(void) { -#ifdef CONFIG_X86_64 - return 1; -#else return APIC_INTEGRATED(lapic_get_version()); -#endif } Do you think so. ;-) Thanks, dou. > ~~~~ Now, I haven't think of why smp_found_config has to be > checked here. > > In this way, we don't need the CONFIG_X86_64 checking since it's > contained in lapic_is_integrated() already. And the checking is obvious > for understanding. Just not very sure if the checking is adequate. > > Just my personal opinion. > >> + /* Check BIOS */ >> +#ifdef CONFIG_X86_64 >> + /* On 64-bit, the APIC must be integrated, Check local APIC only */ >> + if (!boot_cpu_has(X86_FEATURE_APIC)) { >> + disable_apic = 1; >> + pr_info("APIC disabled by BIOS\n"); >> + return APIC_PIC; >> + } >> +#else >> + /* >> + * On 32-bit, check whether there is a separate chip or integrated >> + * APIC >> + */ >> + >> + /* Has a separate chip ? */ >> + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) { >> + disable_apic = 1; >> + >> + return APIC_PIC; >> + } >> + >> + /* Has a local APIC ? */ >> + if (!boot_cpu_has(X86_FEATURE_APIC) && >> + APIC_INTEGRATED(boot_cpu_apic_version)) { >> + disable_apic = 1; >> + pr_err(FW_BUG "Local APIC %d not detected, force emulation\n", >> + boot_cpu_physical_apicid); >> + >> + return APIC_PIC; >> + } >> +#endif >> + >> + /* Check MP table or ACPI MADT configuration */ >> + if (!smp_found_config) { >> + disable_ioapic_support(); >> + >> + if (!acpi_lapic) >> + pr_info("APIC: ACPI MADT or MP tables are not detected\n"); >> + >> + return APIC_VIRTUAL_WIRE; >> + } >> + >> + return APIC_SYMMETRIC_IO; >> +} >> + >> /* >> * An initial setup of the virtual wire mode. >> */ >> -- >> 2.5.5 >> >> >> > > >