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

Groups > linux.kernel > #1730604 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

#1730604 — 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.

[toc] | [next] | [standalone]

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

Hi dou,

I tested your patchset, the result is positive. Kdump kernel functions
well.

About the sanity check in patch 1/13, I still have concerns.
On 09/12/17 at 09:20am, Dou Liyang wrote:
> 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")

> 
> I thought again and again, I would not change this check logic.

I remeber you said you have been working on this issue for more than
half year, and you must have read the code flow agin and again, and
again. I read it too again and again recently.

You have read the related code so many times, while when we talk about
it, you still need think about it again and again, so for other code
reviewers or people who just read code for learning knowledge in this
area in short time, do you think they will get what the
apic_intr_mode_select() is doing immediately? or need read and think
again and again, and again ....?

I think it makes sense to make the logic clearer. In fact the below
logic looks better to me.

        /* 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
         *
         */
apic is not integrated into cpu, it includes two cases: apic is on
82489DX or no apic at all. whatever it is, it's not on cpu. In this two
cases, PIC mode is right choice.
	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;
                }
        }

From this logic the code can explain what it is doing, even without the
need of your code comment. But with the old logic you stick to, I am not
optimistic it can be made clear by code comments.

Surely, this is decided by maintainer.
> 

> 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.
> 
> 

[toc] | [prev] | [next] | [standalone]

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

Hi dou,

On 09/12/17 at 09:20am, Dou Liyang wrote:
> 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)

I won't insist that the logic need be changed. From the test result, the
patchset works very well with notsc specified. And the whole patchset
looks not risky. Maybe the patch putting acpi_early_init() earlier can
be posted independently and involve other ARCHes maintainer to review.

About the code logic, I think the confusion comes from the unclear
condition check. E.g the above case, you said it's used to check
discrete apic, in fact !boot_cpu_has(X86_FEATURE_APIC) could means 3
cases:
1) discrete apic
2) no apic
3) integrated apic but disabled by bios.

See, that's why it's confusing, the condition of judgement is not
adequate. I don't know why the code contributer wanted to check discrete
apic case with it.

Anyway, after discussion, it's clear to me now. And the code works well.
So it's up to you to change it or not. Except of this place, the whole
patchset looks good.

Thanks
Baoquan

> 
> ...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.
> 
> 

[toc] | [prev] | [next] | [standalone]

#1731356

Hi Baoquan,

At 09/13/2017 10:30 AM, Baoquan He wrote:
> Hi dou,
>
> On 09/12/17 at 09:20am, Dou Liyang wrote:
>> 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)
>
> I won't insist that the logic need be changed. From the test result, the
> patchset works very well with notsc specified. And the whole patchset
> looks not risky. Maybe the patch putting acpi_early_init() earlier can
> be posted independently and involve other ARCHes maintainer to review.
>

Yes,  I will send it as an independent patch, and Cc other ARCH
maintainers

> About the code logic, I think the confusion comes from the unclear
> condition check. E.g the above case, you said it's used to check
> discrete apic, in fact !boot_cpu_has(X86_FEATURE_APIC) could means 3
> cases:
> 1) discrete apic
> 2) no apic
> 3) integrated apic but disabled by bios.

Indeed

>
> See, that's why it's confusing, the condition of judgement is not
> adequate. I don't know why the code contributer wanted to check discrete
> apic case with it.
>
> Anyway, after discussion, it's clear to me now. And the code works well.
> So it's up to you to change it or not. Except of this place, the whole
> patchset looks good.

Thank you very much for your review and test.


Thanks,
	dou.
>
> Thanks
> Baoquan
>
>>
>> ...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.
>>
>>
>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web