linux-kernel - Re: [PATCH v8 01/13] x86/apic: Construct a selector for the interrupt delivery mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <172fc486-f894-0daa-7fb8-29a384cb6ae0@cn.fujitsu.com>
Date:   Tue, 12 Sep 2017 09:20:28 +0800
From:   Dou Liyang <douly.fnst@...fujitsu.com>
To:     Baoquan He <bhe@...hat.com>
CC:     <x86@...nel.org>, <linux-kernel@...r.kernel.org>,
        <tglx@...utronix.de>, <mingo@...nel.org>, <hpa@...or.com>,
        <rjw@...ysocki.net>, <bp@...en8.de>, <indou.takao@...fujitsu.com>,
        <izumi.taku@...fujitsu.com>
Subject: Re: [PATCH v8 01/13] x86/apic: Construct a selector for the interrupt
 delivery mode

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.