Re: [tip:irq/numa] x86, apic: Fix dummy apic read operation together with broken MP handling

[Yinghai Lu <yhlu.kernel@gmail.com>]

On Sun, Jun 7, 2009 at 1:39 PM, Cyrill Gorcunov<gorcunov@gmail.com> wrote:
> [Yinghai Lu - Sun, Jun 07, 2009 at 12:23:47PM -0700]
> ...
> | > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> | > index d2e8de9..7c80007 100644
> | > --- a/arch/x86/kernel/smpboot.c
> | > +++ b/arch/x86/kernel/smpboot.c
> | > @@ -992,10 +992,12 @@ static int __init smp_sanity_check(unsigned max_cpus)
> | >         */
> | >        if (APIC_INTEGRATED(apic_version[boot_cpu_physical_apicid]) &&
> | >            !cpu_has_apic) {
> | > -               printk(KERN_ERR "BIOS bug, local APIC #%d not detected!...\n",
> | > -                       boot_cpu_physical_apicid);
> | > -               printk(KERN_ERR "... forcing use of dummy APIC emulation."
> | > +               if (!disable_apic) {
> | > +                       pr_err("BIOS bug, local APIC #%d not detected!...\n",
> | > +                               boot_cpu_physical_apicid);
> | > +                       pr_err("... forcing use of dummy APIC emulation."
> | >                                "(tell your hw vendor)\n");
> | > +               }
> |
> | It seems we don't need this check here.
> | when we have disable_apic, cpu_has_apic is cleared forcely.
> |
> | YH
> |
>
> No Yinghai, it's needed. The check is for !disable_apic
> and if we really has a BIOS bug we should report about
> it _only_ in case if it's a bios bug not apic being
> disabled via boot line. I could be missing something
> of course. Rechecking...
>
> Ah, I remember the scenario I've kept in mind while
> was cooking the patch.
>
> 1) MP apic entry is broken.
> 2) apic was disabled via boot option.
> 3) kernel compiled with smp support.
>
> So we have the calls
>
> init_apic_mappings
>                (due to boot option)
>                apic_disable()
>        (due to broken MP)
>        apic_version[new_apicid] = 0
> smp_sanity_check
>        if (APIC_INTEGRATED(apic_version[boot_cpu_physical_apicid]) &&
>            !cpu_has_apic) {
> Stop! So APIC_INTEGRATED returns false now regargless of what
> really we have on HW level. Darn! Actually I was in idea
> this if() should be true so SMP support will be turned _off_.

in Ingo' case:
before that check
        /*
         * If we couldn't find an SMP configuration at boot time,
         * get out of here now!
         */
        if (!smp_found_config && !acpi_lapic) {
                preempt_enable();
                printk(KERN_NOTICE "SMP motherboard not detected.\n");
                disable_smp();
                if (APIC_init_uniprocessor())
                        printk(KERN_NOTICE "Local APIC not detected."
                                           " Using dummy APIC emulation.\n");
                return -1;
        }

will get out earlier.



>
> Yinghai, I think we need to set apic_version[boot_cpu_physical_apicid]
> to 0xf0 in case if apic is disabled via cmdline option together with
> broken MP. Thoughts?

should be other case:
when MADT is right, but disablelapic is used.
will get cpu_has_apic == 0, and we are not using dummy apic read/write.

so don't need to check
        /*
         * If we couldn't find a local APIC, then get out of here now!
         */
        if (APIC_INTEGRATED(apic_version[boot_cpu_physical_apicid]) &&
            !cpu_has_apic) {
                if (!disable_apic) {
                        pr_err("BIOS bug, local APIC #%d not detected!...\n",
                                boot_cpu_physical_apicid);
                        pr_err("... forcing use of dummy APIC emulation."
                                "(tell your hw vendor)\n");
                }
                smpboot_clear_io_apic();
                arch_disable_smp_support();
                return -1;
        }

>
> To Ingo: this !disable_apic will be needed if Yinghai confirm
> that my idea is right. Meanwhile -- it's just an always-true
> cpu consuming operation. So should not be harming. But sorry
> anyway -- was thinking about one way but reached another.

YH