Subject: Re: [PATCH v8 01/13] x86/apic: Construct a selector for the interrupt
 delivery mode
To: Baoquan He <bhe@redhat.com>
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>
 <9f9f7477-01e9-cd16-47a2-e7ce13789e50@cn.fujitsu.com>
 <20170907052259.GP30906@x1>
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>
From: Dou Liyang <douly.fnst@cn.fujitsu.com>
Message-ID: <172fc486-f894-0daa-7fb8-29a384cb6ae0@cn.fujitsu.com>
Date: Tue, 12 Sep 2017 09:20:28 +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: <20170907052259.GP30906@x1>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3049
Lines: 103

Hi Baoquan,

At 09/07/2017 01:22 PM, Baoquan He wrote:
> On 09/07/17 at 12:19pm, Dou Liyang wrote:
>> 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")
>
> Hmm, sounds reasonable. Just a sentence to describe it could be better.
>

OK, I will

>>
>> 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 thought again and again, I would not change this check logic.

Because actually, we have three possibilities:

   1. ACPI on chip
   2. 82489DX
   3. no APIC

lapic_is_integrated() is used to check the APIC's type which is
APIC on chip or 82489DX. It has a prerequisite, we should avoid
the third possibility(no APIC) first, which is decided by
boot_cpu_has(X86_FEATURE_APIC) and smp_found_config. So, the original
logic:

if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config)

...is not just for 82489DX, but also for no APIC.

It looks more correct and understandable than us.

I am sorry my comments were wrong, and misled us. I will modify it
in my next version.

BTW, How about your test result, is this series OK?

Thanks,
	dou.