Re: [Xen-devel] [PATCH v5 01/12] x86/apic: Construct a selector for the interrupt delivery mode

[Dou Liyang <douly.fnst@xxxxxxxxxxxxxx>]

Hi Thomas,

At 07/03/2017 01:37 AM, Thomas Gleixner wrote:
> On Fri, 30 Jun 2017, 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
> > +        * APIC
> > +        */
> > +
> > +       /* Has a local APIC ? */
> > +       if (!boot_cpu_has(X86_FEATURE_APIC) &&
> > +               APIC_INTEGRATED(boot_cpu_apic_version)) {
> This looks wrong. The existing logic is:
>
>         if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config)
>                 return -1;
>
>         if (!boot_cpu_has(X86_FEATURE_APIC) &&
>                         APIC_INTEGRATED(boot_cpu_apic_version)) {
>                 pr_err(....);
>
> I know that this is magically the same because boot_cpu_apic_version is 0
> in the !boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config case, so you
> don't fall into that conditional,
I see, it an unnecessary and surplus thing I did.

 but it's completely non obvious and does
> not really make the code more understandable. Quite the contrary.
You are right.

> > +               disable_apic = 1;
> > +               pr_err(FW_BUG "Local APIC %d not detected, force emulation\n",
> > +                                      boot_cpu_physical_apicid);
> > +               return APIC_PIC;
> > +       }
> > +
> > +       /* Has a separate chip ? */
> > +       if (!boot_cpu_has(X86_FEATURE_APIC) && !smp_found_config) {
> > +               disable_apic = 1;
> > +
> > +               return APIC_PIC;
> > +       }
> So if you move exactly that check above the other then it's clear what's
> going on.
Will keep the order like the existing logic you gave above.

> > +#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;
> > +       }
> > +
> > +       /* Other checks of APIC options will be done in each setup function */
> > +
> Please remove the extra new line. It's not helping readability.
Yes, remove right now.

Thanks,
        dou.

> > +       return APIC_SYMMETRIC_IO;
> > +}
> Thanks,
>
>         tglx


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel