[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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.