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

[Dou Liyang]

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.

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

[Dou Liyang]

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.

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

[Baoquan He]

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

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

[Baoquan He]

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

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

[Baoquan He]

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

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

[Baoquan He]

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

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

[Dou Liyang]

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.

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

[Dou Liyang]

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.

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

[Baoquan He]

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.

> 
> 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 rewrite it based on you, any comments will welcome.
> 
>   /* 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
>*
>*/
>   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;
>   }
>   }

Yeah, it's fine to me. At least the logic looks more understandable.

> 
> BTW, As the macro APIC_INTEGRATED(x) has already wrapped by
> CONFIG_X86_32, I will cleanup the lapic_is_integrated() for readablity
> like that:

Yes, looks good. There's duplicate judgement of X86_64 in
lapic_is_integrated.

> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 7834f73..63b3ae9 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -211,11 +211,7 @@ static inline int lapic_get_version(void)
>   */
>  static inline int lapic_is_integrated(void)
>  {
> -#ifdef CONFIG_X86_64
> -   return 1;
> -#else
> return APIC_INTEGRATED(lapic_get_version());
> -#endif
>  }
> 
> 
> Do you think so. ;-)
> 
> 
> Thanks,
>   dou.
> 
> 
> >  Now, I haven't think of why smp_found_config has to be
> >  checked here.
> > 
> > In this way, we don't need the CONFIG_X86_64 checking since it's
> > contained in lapic_is_integrated() already. And the checking is obvious
> > for understanding. Just not very sure if the checking is adequate.
> > 
> > Just my personal opinion.
> > 
> > > + /* Check BIOS */
> > > +#ifdef CONFIG_X86_64
> > > + /* On 64-bit, the APIC must be integrated, Check local APIC only */
> > > + if (!boot_cpu_has(X86_FEATURE_APIC)) {
> > > + disable_apic = 1;
> > > + pr_info("APIC disabled by BIOS\n");
> > > + return APIC_PIC;
> > > + }
> > > +#else
> > > + /*
> > > +  * On 32-bit, check whether there is a separate chip or integrated
> > > +  * APIC
> > > +  */
> > > +
> > > + /* Has a separate chip ? */
> > > + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
> > > + disable_apic = 1;
> > > +
> > > + return APIC_PIC;
> > > + }
> > > +
> > > + /* Has a local APIC ? */
> > > + if (!boot_cpu_has(X86_FEATURE_APIC) &&
> > > + APIC_INTEGRATED(boot_cpu_apic_version)) {
> > > +  

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

[Baoquan He]

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.

> 
> 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 rewrite it based on you, any comments will welcome.
> 
>   /* 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
>*
>*/
>   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;
>   }
>   }

Yeah, it's fine to me. At least the logic looks more understandable.

> 
> BTW, As the macro APIC_INTEGRATED(x) has already wrapped by
> CONFIG_X86_32, I will cleanup the lapic_is_integrated() for readablity
> like that:

Yes, looks good. There's duplicate judgement of X86_64 in
lapic_is_integrated.

> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 7834f73..63b3ae9 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -211,11 +211,7 @@ static inline int lapic_get_version(void)
>   */
>  static inline int lapic_is_integrated(void)
>  {
> -#ifdef CONFIG_X86_64
> -   return 1;
> -#else
> return APIC_INTEGRATED(lapic_get_version());
> -#endif
>  }
> 
> 
> Do you think so. ;-)
> 
> 
> Thanks,
>   dou.
> 
> 
> >  Now, I haven't think of why smp_found_config has to be
> >  checked here.
> > 
> > In this way, we don't need the CONFIG_X86_64 checking since it's
> > contained in lapic_is_integrated() already. And the checking is obvious
> > for understanding. Just not very sure if the checking is adequate.
> > 
> > Just my personal opinion.
> > 
> > > + /* Check BIOS */
> > > +#ifdef CONFIG_X86_64
> > > + /* On 64-bit, the APIC must be integrated, Check local APIC only */
> > > + if (!boot_cpu_has(X86_FEATURE_APIC)) {
> > > + disable_apic = 1;
> > > + pr_info("APIC disabled by BIOS\n");
> > > + return APIC_PIC;
> > > + }
> > > +#else
> > > + /*
> > > +  * On 32-bit, check whether there is a separate chip or integrated
> > > +  * APIC
> > > +  */
> > > +
> > > + /* Has a separate chip ? */
> > > + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
> > > + disable_apic = 1;
> > > +
> > > + return APIC_PIC;
> > > + }
> > > +
> > > + /* Has a local APIC ? */
> > > + if (!boot_cpu_has(X86_FEATURE_APIC) &&
> > > + APIC_INTEGRATED(boot_cpu_apic_version)) {
> > > +  

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

[Dou Liyang]

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")

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 rewrite it based on you, any comments will welcome.

/* 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
 *
 */
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;
}
}

BTW, As the macro APIC_INTEGRATED(x) has already wrapped by
CONFIG_X86_32, I will cleanup the lapic_is_integrated() for readablity
like that:

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 7834f73..63b3ae9 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -211,11 +211,7 @@ static inline int lapic_get_version(void)
  */
 static inline int lapic_is_integrated(void)
 {
-#ifdef CONFIG_X86_64
-   return 1;
-#else
return APIC_INTEGRATED(lapic_get_version());
-#endif
 }


Do you think so. ;-)


Thanks,
dou.



 Now, I haven't think of why smp_found_config has to be
 checked here.

In this way, we don't need the CONFIG_X86_64 checking since it's
contained in lapic_is_integrated() already. And the checking is obvious
for understanding. Just not very sure if the checking is adequate.

Just my personal opinion.


+   /* Check BIOS */
+#ifdef CONFIG_X86_64
+   /* On 64-bit, the APIC must be integrated, Check local APIC only */
+   if (!boot_cpu_has(X86_FEATURE_APIC)) {
+   disable_apic = 1;
+   pr_info("APIC disabled by BIOS\n");
+   return APIC_PIC;
+   }
+#else
+   /*
+* On 32-bit, check whether there is a separate chip or integrated
+* APIC
+*/
+
+   /* Has a separate chip ? */
+   if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
+   disable_apic = 1;
+
+   return APIC_PIC;
+   }
+
+   /* Has a local APIC ? */
+   if (!boot_cpu_has(X86_FEATURE_APIC) &&
+   APIC_INTEGRATED(boot_cpu_apic_version)) {
+   disable_apic = 1;
+   pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
+  boot_cpu_physical_apicid);
+
+   return APIC_PIC;
+   }
+#endif
+
+   /* Check MP table or ACPI MADT configuration */
+   if (!smp_found_config) {
+   disable_ioapic_support();
+
+   if (!acpi_lapic)
+   pr_info("APIC: ACPI MADT or MP tables are not 
detected\n");
+
+   

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

[Dou Liyang]

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")

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 rewrite it based on you, any comments will welcome.

/* 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
 *
 */
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;
}
}

BTW, As the macro APIC_INTEGRATED(x) has already wrapped by
CONFIG_X86_32, I will cleanup the lapic_is_integrated() for readablity
like that:

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 7834f73..63b3ae9 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -211,11 +211,7 @@ static inline int lapic_get_version(void)
  */
 static inline int lapic_is_integrated(void)
 {
-#ifdef CONFIG_X86_64
-   return 1;
-#else
return APIC_INTEGRATED(lapic_get_version());
-#endif
 }


Do you think so. ;-)


Thanks,
dou.



 Now, I haven't think of why smp_found_config has to be
 checked here.

In this way, we don't need the CONFIG_X86_64 checking since it's
contained in lapic_is_integrated() already. And the checking is obvious
for understanding. Just not very sure if the checking is adequate.

Just my personal opinion.


+   /* Check BIOS */
+#ifdef CONFIG_X86_64
+   /* On 64-bit, the APIC must be integrated, Check local APIC only */
+   if (!boot_cpu_has(X86_FEATURE_APIC)) {
+   disable_apic = 1;
+   pr_info("APIC disabled by BIOS\n");
+   return APIC_PIC;
+   }
+#else
+   /*
+* On 32-bit, check whether there is a separate chip or integrated
+* APIC
+*/
+
+   /* Has a separate chip ? */
+   if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
+   disable_apic = 1;
+
+   return APIC_PIC;
+   }
+
+   /* Has a local APIC ? */
+   if (!boot_cpu_has(X86_FEATURE_APIC) &&
+   APIC_INTEGRATED(boot_cpu_apic_version)) {
+   disable_apic = 1;
+   pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
+  boot_cpu_physical_apicid);
+
+   return APIC_PIC;
+   }
+#endif
+
+   /* Check MP table or ACPI MADT configuration */
+   if (!smp_found_config) {
+   disable_ioapic_support();
+
+   if (!acpi_lapic)
+   pr_info("APIC: ACPI MADT or MP tables are not 
detected\n");
+
+   

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

[Baoquan He]

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;
}
 Now, I haven't think of why smp_found_config has to be
 checked here.

In this way, we don't need the CONFIG_X86_64 checking since it's
contained in lapic_is_integrated() already. And the checking is obvious
for understanding. Just not very sure if the checking is adequate.

Just my personal opinion.

> + /* Check BIOS */
> +#ifdef CONFIG_X86_64
> + /* On 64-bit, the APIC must be integrated, Check local APIC only */
> + if (!boot_cpu_has(X86_FEATURE_APIC)) {
> + disable_apic = 1;
> + pr_info("APIC disabled by BIOS\n");
> + return APIC_PIC;
> + }
> +#else
> + /*
> +  * On 32-bit, check whether there is a separate chip or integrated
> +  * APIC
> +  */
> +
> + /* Has a separate chip ? */
> + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
> + disable_apic = 1;
> +
> + return APIC_PIC;
> + }
> +
> + /* Has a local APIC ? */
> + if (!boot_cpu_has(X86_FEATURE_APIC) &&
> + APIC_INTEGRATED(boot_cpu_apic_version)) {
> + disable_apic = 1;
> + pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
> +boot_cpu_physical_apicid);
> +
> + return APIC_PIC;
> + }
> +#endif
> +
> + /* Check MP table or ACPI MADT configuration */
> + if (!smp_found_config) {
> + disable_ioapic_support();
> +
> + if (!acpi_lapic)
> + pr_info("APIC: ACPI MADT or MP tables are not 
> detected\n");
> +
> + return APIC_VIRTUAL_WIRE;
> + }
> +
> + return APIC_SYMMETRIC_IO;
> +}
> +
>  /*
>   * An initial setup of the virtual wire mode.
>   */
> -- 
> 2.5.5
> 
> 
> 

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

[Baoquan He]

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;
}
 Now, I haven't think of why smp_found_config has to be
 checked here.

In this way, we don't need the CONFIG_X86_64 checking since it's
contained in lapic_is_integrated() already. And the checking is obvious
for understanding. Just not very sure if the checking is adequate.

Just my personal opinion.

> + /* Check BIOS */
> +#ifdef CONFIG_X86_64
> + /* On 64-bit, the APIC must be integrated, Check local APIC only */
> + if (!boot_cpu_has(X86_FEATURE_APIC)) {
> + disable_apic = 1;
> + pr_info("APIC disabled by BIOS\n");
> + return APIC_PIC;
> + }
> +#else
> + /*
> +  * On 32-bit, check whether there is a separate chip or integrated
> +  * APIC
> +  */
> +
> + /* Has a separate chip ? */
> + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
> + disable_apic = 1;
> +
> + return APIC_PIC;
> + }
> +
> + /* Has a local APIC ? */
> + if (!boot_cpu_has(X86_FEATURE_APIC) &&
> + APIC_INTEGRATED(boot_cpu_apic_version)) {
> + disable_apic = 1;
> + pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
> +boot_cpu_physical_apicid);
> +
> + return APIC_PIC;
> + }
> +#endif
> +
> + /* Check MP table or ACPI MADT configuration */
> + if (!smp_found_config) {
> + disable_ioapic_support();
> +
> + if (!acpi_lapic)
> + pr_info("APIC: ACPI MADT or MP tables are not 
> detected\n");
> +
> + return APIC_VIRTUAL_WIRE;
> + }
> +
> + return APIC_SYMMETRIC_IO;
> +}
> +
>  /*
>   * An initial setup of the virtual wire mode.
>   */
> -- 
> 2.5.5
> 
> 
> 

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

[Baoquan He]

On 09/06/17 at 12:18pm, 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;
> > > + }
> > > +
> > > + /* Check BIOS */
> > > +#ifdef CONFIG_X86_64
> > > + /* On 64-bit, the APIC must be integrated, Check local APIC only */
> > > + if (!boot_cpu_has(X86_FEATURE_APIC)) {
> > > + disable_apic = 1;
> > > + pr_info("APIC disabled by BIOS\n");
> > > + return APIC_PIC;
> > > + }
> > > +#else
> > > + /*
> > > +  * On 32-bit, check whether there is a separate chip or integrated
> 
> Change the comment to:
> 
> On 32-bit, the APIC may be a separated chip(82489DX) or integrated chip.
> if BSP doesn't has APIC feature, we can sure there is no integrated
> chip, but can not be sure there is no independent chip. So check two
> situation when BSP doesn't has APIC feature.
> 
> > > +  * APIC
> > > +  */
> > > +
> > > + /* Has a separate chip ? */
> 
> If there is also no SMP configuration, we can be sure there is no
> separated chip. Switch the interrupt delivery node to APIC_PIC directly.
> 
> > > + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {

Here, the most confusing thing to me is the '!smp_found_config'. Why
does 'smp_found_config' has anything with APIC being separate or
integrated?

>From code, 'smp_found_config = 1' when process ACPI MADT, or in
smp_scan_config(). Do you have any finding about the thing that if no
smp config apic must not exist?

Just for curiosity, I know this is copied from APIC_init_uniprocessor().
But I don't understand the logic clearly.

> > > + disable_apic = 1;
> > > +
> > > + return APIC_PIC;
> > > + }
> > > +
> > Here you said several times you are checking if APIC is integrated, but
> > you always check boot_cpu_has(X86_FEATURE_APIC), and you also check
> > smp_found_config in above case. Can you make the comment match the code?
> > 
> 
> Yes.
> 
> > E.g if (!boot_cpu_has(X86_FEATURE_APIC)), cpu doesn't support lapic,
> > just return, you can remove the CONFIG_X86_64 check to make it a common
> > check. And we have lapic_is_integrated() to check if lapic is integrated.
> > 
> I am sorry my comment may confuse you. our target is checking if BIOS
> supports APIC, no matter what type(separated/integrated) it is.
> 
> The new logic 1) as you said may like :
> 
>   if (!boot_cpu_has(X86_FEATURE_APIC))
>   return ...
>   if (lapic_is_integrated())
>   return ...
> here we miss (!boot_cpu_has(X86_FEATURE_APIC) && smp_found_config) for
> a separated chip.
> 
> > Besides, we are saying lapic is integrated with ioapic in a single chip,
> > right? I found MP-Spec mention it. If yes, could you add more words to
> 
> Yes, 82489DX – it was a discrete chip that functioned both as local and
> I/O APIC
> 
> > make it more specific and precise? Then people can get the exact
> 
> Indeed, I will. Please see the modification of comments
> 
> > information from the comment and code.
> > 
> > Thanks
> > Baoquan
> > 
> > > + /* Has a local APIC ? */
> 
> Sanity check if the BIOS pretends there is one local APIC.
> 
> 
> Thanks,
>   dou.
> 
> > > + if (!boot_cpu_has(X86_FEATURE_APIC) &&
> > > + APIC_INTEGRATED(boot_cpu_apic_version)) {
> > > + disable_apic = 1;
> > > + pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
> > > +boot_cpu_physical_apicid);
> > > +
> > > + return APIC_PIC;
> > > + }
> > > +#endif
> > > +
> > > + /* Check MP table or ACPI MADT configuration */
> > > + if (!smp_found_config) {
> > > + disable_ioapic_support();
> > > +
> > > + if (!acpi_lapic)
> > > + pr_info("APIC: ACPI MADT or MP tables are not 
> > > detected\n");
> > > +
> > > + return APIC_VIRTUAL_WIRE;
> > > + }
> > > +
> > > + return APIC_SYMMETRIC_IO;
> > > +}
> > > +
> > >  /*
> > >   * An initial setup of the virtual wire mode.
> > >   */
> > > --
> > > 2.5.5
> > > 
> > > 
> > > 
> > 
> > 
> > 
> 
> 

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

[Baoquan He]

On 09/06/17 at 12:18pm, 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;
> > > + }
> > > +
> > > + /* Check BIOS */
> > > +#ifdef CONFIG_X86_64
> > > + /* On 64-bit, the APIC must be integrated, Check local APIC only */
> > > + if (!boot_cpu_has(X86_FEATURE_APIC)) {
> > > + disable_apic = 1;
> > > + pr_info("APIC disabled by BIOS\n");
> > > + return APIC_PIC;
> > > + }
> > > +#else
> > > + /*
> > > +  * On 32-bit, check whether there is a separate chip or integrated
> 
> Change the comment to:
> 
> On 32-bit, the APIC may be a separated chip(82489DX) or integrated chip.
> if BSP doesn't has APIC feature, we can sure there is no integrated
> chip, but can not be sure there is no independent chip. So check two
> situation when BSP doesn't has APIC feature.
> 
> > > +  * APIC
> > > +  */
> > > +
> > > + /* Has a separate chip ? */
> 
> If there is also no SMP configuration, we can be sure there is no
> separated chip. Switch the interrupt delivery node to APIC_PIC directly.
> 
> > > + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {

Here, the most confusing thing to me is the '!smp_found_config'. Why
does 'smp_found_config' has anything with APIC being separate or
integrated?

>From code, 'smp_found_config = 1' when process ACPI MADT, or in
smp_scan_config(). Do you have any finding about the thing that if no
smp config apic must not exist?

Just for curiosity, I know this is copied from APIC_init_uniprocessor().
But I don't understand the logic clearly.

> > > + disable_apic = 1;
> > > +
> > > + return APIC_PIC;
> > > + }
> > > +
> > Here you said several times you are checking if APIC is integrated, but
> > you always check boot_cpu_has(X86_FEATURE_APIC), and you also check
> > smp_found_config in above case. Can you make the comment match the code?
> > 
> 
> Yes.
> 
> > E.g if (!boot_cpu_has(X86_FEATURE_APIC)), cpu doesn't support lapic,
> > just return, you can remove the CONFIG_X86_64 check to make it a common
> > check. And we have lapic_is_integrated() to check if lapic is integrated.
> > 
> I am sorry my comment may confuse you. our target is checking if BIOS
> supports APIC, no matter what type(separated/integrated) it is.
> 
> The new logic 1) as you said may like :
> 
>   if (!boot_cpu_has(X86_FEATURE_APIC))
>   return ...
>   if (lapic_is_integrated())
>   return ...
> here we miss (!boot_cpu_has(X86_FEATURE_APIC) && smp_found_config) for
> a separated chip.
> 
> > Besides, we are saying lapic is integrated with ioapic in a single chip,
> > right? I found MP-Spec mention it. If yes, could you add more words to
> 
> Yes, 82489DX – it was a discrete chip that functioned both as local and
> I/O APIC
> 
> > make it more specific and precise? Then people can get the exact
> 
> Indeed, I will. Please see the modification of comments
> 
> > information from the comment and code.
> > 
> > Thanks
> > Baoquan
> > 
> > > + /* Has a local APIC ? */
> 
> Sanity check if the BIOS pretends there is one local APIC.
> 
> 
> Thanks,
>   dou.
> 
> > > + if (!boot_cpu_has(X86_FEATURE_APIC) &&
> > > + APIC_INTEGRATED(boot_cpu_apic_version)) {
> > > + disable_apic = 1;
> > > + pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
> > > +boot_cpu_physical_apicid);
> > > +
> > > + return APIC_PIC;
> > > + }
> > > +#endif
> > > +
> > > + /* Check MP table or ACPI MADT configuration */
> > > + if (!smp_found_config) {
> > > + disable_ioapic_support();
> > > +
> > > + if (!acpi_lapic)
> > > + pr_info("APIC: ACPI MADT or MP tables are not 
> > > detected\n");
> > > +
> > > + return APIC_VIRTUAL_WIRE;
> > > + }
> > > +
> > > + return APIC_SYMMETRIC_IO;
> > > +}
> > > +
> > >  /*
> > >   * An initial setup of the virtual wire mode.
> > >   */
> > > --
> > > 2.5.5
> > > 
> > > 
> > > 
> > 
> > 
> > 
> 
> 

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

[Dou Liyang]

Hi Baoquan,

Thanks for your reply!

My answer is in below.

At 09/06/2017 08:55 AM, Baoquan He wrote:

Hi Liyang,

On 08/28/17 at 11:20am, Dou Liyang wrote:

Now, there are many switches in kernel which are used to determine
the final interrupt delivery mode, as shown below:

1) kconfig:
   CONFIG_X86_64; CONFIG_X86_LOCAL_APIC; CONFIG_x86_IO_APIC
2) kernel option: disable_apic; skip_ioapic_setup
3) CPU Capability: boot_cpu_has(X86_FEATURE_APIC)
4) MP table: smp_found_config
5) ACPI: acpi_lapic; acpi_ioapic; nr_ioapic

These switches are disordered and scattered and there are also some
dependencies with each other. These make the code difficult to
maintain and read.

Construct a selector to unify them into a single function, then,
Use this selector to get an interrupt delivery mode directly.

Signed-off-by: Dou Liyang 
---
 arch/x86/kernel/apic/apic.c | 59 +
 1 file changed, 59 insertions(+)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 98b3dd8..01bde03 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1235,6 +1235,65 @@ void __init sync_Arb_IDs(void)
APIC_INT_LEVELTRIG | APIC_DM_INIT);
 }

+enum apic_intr_mode {
+   APIC_PIC,
+   APIC_VIRTUAL_WIRE,
+   APIC_SYMMETRIC_IO,
+};
+
+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;
+   }
+
+   /* Check BIOS */
+#ifdef CONFIG_X86_64
+   /* On 64-bit, the APIC must be integrated, Check local APIC only */
+   if (!boot_cpu_has(X86_FEATURE_APIC)) {
+   disable_apic = 1;
+   pr_info("APIC disabled by BIOS\n");
+   return APIC_PIC;
+   }
+#else
+   /*
+* On 32-bit, check whether there is a separate chip or integrated


Change the comment to:

On 32-bit, the APIC may be a separated chip(82489DX) or integrated chip.
if BSP doesn't has APIC feature, we can sure there is no integrated
chip, but can not be sure there is no independent chip. So check two
situation when BSP doesn't has APIC feature.


+* APIC
+*/
+
+   /* Has a separate chip ? */


If there is also no SMP configuration, we can be sure there is no
separated chip. Switch the interrupt delivery node to APIC_PIC directly.


+   if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
+   disable_apic = 1;
+
+   return APIC_PIC;
+   }
+

Here you said several times you are checking if APIC is integrated, but
you always check boot_cpu_has(X86_FEATURE_APIC), and you also check
smp_found_config in above case. Can you make the comment match the code?



Yes.


E.g if (!boot_cpu_has(X86_FEATURE_APIC)), cpu doesn't support lapic,
just return, you can remove the CONFIG_X86_64 check to make it a common
check. And we have lapic_is_integrated() to check if lapic is integrated.


I am sorry my comment may confuse you. our target is checking if BIOS
supports APIC, no matter what type(separated/integrated) it is.

The new logic 1) as you said may like :

if (!boot_cpu_has(X86_FEATURE_APIC))
return ...
if (lapic_is_integrated())
return ...
here we miss (!boot_cpu_has(X86_FEATURE_APIC) && smp_found_config) for
a separated chip.


Besides, we are saying lapic is integrated with ioapic in a single chip,
right? I found MP-Spec mention it. If yes, could you add more words to


Yes, 82489DX – it was a discrete chip that functioned both as local and
I/O APIC


make it more specific and precise? Then people can get the exact


Indeed, I will. Please see the modification of comments


information from the comment and code.

Thanks
Baoquan


+   /* Has a local APIC ? */


Sanity check if the BIOS pretends there is one local APIC.


Thanks,
dou.


+   if (!boot_cpu_has(X86_FEATURE_APIC) &&
+   APIC_INTEGRATED(boot_cpu_apic_version)) {
+   disable_apic = 1;
+   pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
+  boot_cpu_physical_apicid);
+
+   return APIC_PIC;
+   }
+#endif
+
+   /* Check MP table or ACPI MADT configuration */
+   if (!smp_found_config) {
+   disable_ioapic_support();
+
+   if (!acpi_lapic)
+   pr_info("APIC: ACPI MADT or MP tables are not 
detected\n");
+
+   return APIC_VIRTUAL_WIRE;
+   }
+
+   return APIC_SYMMETRIC_IO;
+}
+
 /*
  * An initial setup of the virtual wire mode.
  */
--
2.5.5

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

[Dou Liyang]

Hi Baoquan,

Thanks for your reply!

My answer is in below.

At 09/06/2017 08:55 AM, Baoquan He wrote:

Hi Liyang,

On 08/28/17 at 11:20am, Dou Liyang wrote:

Now, there are many switches in kernel which are used to determine
the final interrupt delivery mode, as shown below:

1) kconfig:
   CONFIG_X86_64; CONFIG_X86_LOCAL_APIC; CONFIG_x86_IO_APIC
2) kernel option: disable_apic; skip_ioapic_setup
3) CPU Capability: boot_cpu_has(X86_FEATURE_APIC)
4) MP table: smp_found_config
5) ACPI: acpi_lapic; acpi_ioapic; nr_ioapic

These switches are disordered and scattered and there are also some
dependencies with each other. These make the code difficult to
maintain and read.

Construct a selector to unify them into a single function, then,
Use this selector to get an interrupt delivery mode directly.

Signed-off-by: Dou Liyang 
---
 arch/x86/kernel/apic/apic.c | 59 +
 1 file changed, 59 insertions(+)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 98b3dd8..01bde03 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1235,6 +1235,65 @@ void __init sync_Arb_IDs(void)
APIC_INT_LEVELTRIG | APIC_DM_INIT);
 }

+enum apic_intr_mode {
+   APIC_PIC,
+   APIC_VIRTUAL_WIRE,
+   APIC_SYMMETRIC_IO,
+};
+
+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;
+   }
+
+   /* Check BIOS */
+#ifdef CONFIG_X86_64
+   /* On 64-bit, the APIC must be integrated, Check local APIC only */
+   if (!boot_cpu_has(X86_FEATURE_APIC)) {
+   disable_apic = 1;
+   pr_info("APIC disabled by BIOS\n");
+   return APIC_PIC;
+   }
+#else
+   /*
+* On 32-bit, check whether there is a separate chip or integrated


Change the comment to:

On 32-bit, the APIC may be a separated chip(82489DX) or integrated chip.
if BSP doesn't has APIC feature, we can sure there is no integrated
chip, but can not be sure there is no independent chip. So check two
situation when BSP doesn't has APIC feature.


+* APIC
+*/
+
+   /* Has a separate chip ? */


If there is also no SMP configuration, we can be sure there is no
separated chip. Switch the interrupt delivery node to APIC_PIC directly.


+   if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
+   disable_apic = 1;
+
+   return APIC_PIC;
+   }
+

Here you said several times you are checking if APIC is integrated, but
you always check boot_cpu_has(X86_FEATURE_APIC), and you also check
smp_found_config in above case. Can you make the comment match the code?



Yes.


E.g if (!boot_cpu_has(X86_FEATURE_APIC)), cpu doesn't support lapic,
just return, you can remove the CONFIG_X86_64 check to make it a common
check. And we have lapic_is_integrated() to check if lapic is integrated.


I am sorry my comment may confuse you. our target is checking if BIOS
supports APIC, no matter what type(separated/integrated) it is.

The new logic 1) as you said may like :

if (!boot_cpu_has(X86_FEATURE_APIC))
return ...
if (lapic_is_integrated())
return ...
here we miss (!boot_cpu_has(X86_FEATURE_APIC) && smp_found_config) for
a separated chip.


Besides, we are saying lapic is integrated with ioapic in a single chip,
right? I found MP-Spec mention it. If yes, could you add more words to


Yes, 82489DX – it was a discrete chip that functioned both as local and
I/O APIC


make it more specific and precise? Then people can get the exact


Indeed, I will. Please see the modification of comments


information from the comment and code.

Thanks
Baoquan


+   /* Has a local APIC ? */


Sanity check if the BIOS pretends there is one local APIC.


Thanks,
dou.


+   if (!boot_cpu_has(X86_FEATURE_APIC) &&
+   APIC_INTEGRATED(boot_cpu_apic_version)) {
+   disable_apic = 1;
+   pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
+  boot_cpu_physical_apicid);
+
+   return APIC_PIC;
+   }
+#endif
+
+   /* Check MP table or ACPI MADT configuration */
+   if (!smp_found_config) {
+   disable_ioapic_support();
+
+   if (!acpi_lapic)
+   pr_info("APIC: ACPI MADT or MP tables are not 
detected\n");
+
+   return APIC_VIRTUAL_WIRE;
+   }
+
+   return APIC_SYMMETRIC_IO;
+}
+
 /*
  * An initial setup of the virtual wire mode.
  */
--
2.5.5

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

[Baoquan He]

Hi Liyang,

On 08/28/17 at 11:20am, Dou Liyang wrote:
> Now, there are many switches in kernel which are used to determine
> the final interrupt delivery mode, as shown below:
> 
> 1) kconfig:
>CONFIG_X86_64; CONFIG_X86_LOCAL_APIC; CONFIG_x86_IO_APIC
> 2) kernel option: disable_apic; skip_ioapic_setup
> 3) CPU Capability: boot_cpu_has(X86_FEATURE_APIC)
> 4) MP table: smp_found_config
> 5) ACPI: acpi_lapic; acpi_ioapic; nr_ioapic
> 
> These switches are disordered and scattered and there are also some
> dependencies with each other. These make the code difficult to
> maintain and read.
> 
> Construct a selector to unify them into a single function, then,
> Use this selector to get an interrupt delivery mode directly.
> 
> Signed-off-by: Dou Liyang 
> ---
>  arch/x86/kernel/apic/apic.c | 59 
> +
>  1 file changed, 59 insertions(+)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 98b3dd8..01bde03 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1235,6 +1235,65 @@ void __init sync_Arb_IDs(void)
>   APIC_INT_LEVELTRIG | APIC_DM_INIT);
>  }
>  
> +enum apic_intr_mode {
> + APIC_PIC,
> + APIC_VIRTUAL_WIRE,
> + APIC_SYMMETRIC_IO,
> +};
> +
> +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;
> + }
> +
> + /* Check BIOS */
> +#ifdef CONFIG_X86_64
> + /* On 64-bit, the APIC must be integrated, Check local APIC only */
> + if (!boot_cpu_has(X86_FEATURE_APIC)) {
> + disable_apic = 1;
> + pr_info("APIC disabled by BIOS\n");
> + return APIC_PIC;
> + }
> +#else
> + /*
> +  * On 32-bit, check whether there is a separate chip or integrated
> +  * APIC
> +  */
> +
> + /* Has a separate chip ? */
> + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
> + disable_apic = 1;
> +
> + return APIC_PIC;
> + }
> +
Here you said several times you are checking if APIC is integrated, but
you always check boot_cpu_has(X86_FEATURE_APIC), and you also check
smp_found_config in above case. Can you make the comment match the code?

E.g if (!boot_cpu_has(X86_FEATURE_APIC)), cpu doesn't support lapic,
just return, you can remove the CONFIG_X86_64 check to make it a common
check. And we have lapic_is_integrated() to check if lapic is integrated.

Besides, we are saying lapic is integrated with ioapic in a single chip,
right? I found MP-Spec mention it. If yes, could you add more words to
make it more specific and precise? Then people can get the exact
information from the comment and code.

Thanks
Baoquan

> + /* Has a local APIC ? */
> + if (!boot_cpu_has(X86_FEATURE_APIC) &&
> + APIC_INTEGRATED(boot_cpu_apic_version)) {
> + disable_apic = 1;
> + pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
> +boot_cpu_physical_apicid);
> +
> + return APIC_PIC;
> + }
> +#endif
> +
> + /* Check MP table or ACPI MADT configuration */
> + if (!smp_found_config) {
> + disable_ioapic_support();
> +
> + if (!acpi_lapic)
> + pr_info("APIC: ACPI MADT or MP tables are not 
> detected\n");
> +
> + return APIC_VIRTUAL_WIRE;
> + }
> +
> + return APIC_SYMMETRIC_IO;
> +}
> +
>  /*
>   * An initial setup of the virtual wire mode.
>   */
> -- 
> 2.5.5
> 
> 
> 

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

[Baoquan He]

Hi Liyang,

On 08/28/17 at 11:20am, Dou Liyang wrote:
> Now, there are many switches in kernel which are used to determine
> the final interrupt delivery mode, as shown below:
> 
> 1) kconfig:
>CONFIG_X86_64; CONFIG_X86_LOCAL_APIC; CONFIG_x86_IO_APIC
> 2) kernel option: disable_apic; skip_ioapic_setup
> 3) CPU Capability: boot_cpu_has(X86_FEATURE_APIC)
> 4) MP table: smp_found_config
> 5) ACPI: acpi_lapic; acpi_ioapic; nr_ioapic
> 
> These switches are disordered and scattered and there are also some
> dependencies with each other. These make the code difficult to
> maintain and read.
> 
> Construct a selector to unify them into a single function, then,
> Use this selector to get an interrupt delivery mode directly.
> 
> Signed-off-by: Dou Liyang 
> ---
>  arch/x86/kernel/apic/apic.c | 59 
> +
>  1 file changed, 59 insertions(+)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 98b3dd8..01bde03 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1235,6 +1235,65 @@ void __init sync_Arb_IDs(void)
>   APIC_INT_LEVELTRIG | APIC_DM_INIT);
>  }
>  
> +enum apic_intr_mode {
> + APIC_PIC,
> + APIC_VIRTUAL_WIRE,
> + APIC_SYMMETRIC_IO,
> +};
> +
> +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;
> + }
> +
> + /* Check BIOS */
> +#ifdef CONFIG_X86_64
> + /* On 64-bit, the APIC must be integrated, Check local APIC only */
> + if (!boot_cpu_has(X86_FEATURE_APIC)) {
> + disable_apic = 1;
> + pr_info("APIC disabled by BIOS\n");
> + return APIC_PIC;
> + }
> +#else
> + /*
> +  * On 32-bit, check whether there is a separate chip or integrated
> +  * APIC
> +  */
> +
> + /* Has a separate chip ? */
> + if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
> + disable_apic = 1;
> +
> + return APIC_PIC;
> + }
> +
Here you said several times you are checking if APIC is integrated, but
you always check boot_cpu_has(X86_FEATURE_APIC), and you also check
smp_found_config in above case. Can you make the comment match the code?

E.g if (!boot_cpu_has(X86_FEATURE_APIC)), cpu doesn't support lapic,
just return, you can remove the CONFIG_X86_64 check to make it a common
check. And we have lapic_is_integrated() to check if lapic is integrated.

Besides, we are saying lapic is integrated with ioapic in a single chip,
right? I found MP-Spec mention it. If yes, could you add more words to
make it more specific and precise? Then people can get the exact
information from the comment and code.

Thanks
Baoquan

> + /* Has a local APIC ? */
> + if (!boot_cpu_has(X86_FEATURE_APIC) &&
> + APIC_INTEGRATED(boot_cpu_apic_version)) {
> + disable_apic = 1;
> + pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
> +boot_cpu_physical_apicid);
> +
> + return APIC_PIC;
> + }
> +#endif
> +
> + /* Check MP table or ACPI MADT configuration */
> + if (!smp_found_config) {
> + disable_ioapic_support();
> +
> + if (!acpi_lapic)
> + pr_info("APIC: ACPI MADT or MP tables are not 
> detected\n");
> +
> + return APIC_VIRTUAL_WIRE;
> + }
> +
> + return APIC_SYMMETRIC_IO;
> +}
> +
>  /*
>   * An initial setup of the virtual wire mode.
>   */
> -- 
> 2.5.5
> 
> 
>