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