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

[Thomas Gleixner <tglx@xxxxxxxxxxxxx>]

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, but it's completely non obvious and does
not really make the code more understandable. Quite the contrary.

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

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

> +     return APIC_SYMMETRIC_IO;
> +}

Thanks,

        tglx

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