lkml.org 
[lkml]   [2017]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v8 01/13] x86/apic: Construct a selector for the interrupt delivery mode
From
Date
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.


\
 
 \ /
  Last update: 2017-09-12 03:21    [W:0.088 / U:6.088 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site