Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1730604

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

Show all headers | View raw

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.

Back to linux.kernel | Previous | Next — Next in thread | Find similar | Unroll thread

Thread

Re: [PATCH v8 01/13] x86/apic: Construct a selector for the interrupt  delivery mode Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-09-12 03:30 +0200
  Re: [PATCH v8 01/13] x86/apic: Construct a selector for the  interrupt delivery mode Baoquan He <bhe@redhat.com> - 2017-09-12 10:10 +0200
  Re: [PATCH v8 01/13] x86/apic: Construct a selector for the  interrupt delivery mode Baoquan He <bhe@redhat.com> - 2017-09-13 04:40 +0200
    Re: [PATCH v8 01/13] x86/apic: Construct a selector for the interrupt  delivery mode Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-09-13 05:50 +0200

csiph-web