[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 01/12] x86/apic: Construct a selector for the interrupt delivery mode
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |