[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/3] xen/x86: ioapic: Simplify ioapic_init()



Hi Jan,

On 31/03/2020 12:22, Jan Beulich wrote:
On 27.03.2020 20:05, Julien Grall wrote:
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2537,34 +2537,25 @@ static __init bool bad_ioapic_register(unsigned int idx)
      return false;
  }
-void __init init_ioapic(void)
+static void __init init_ioapic_mappings(void)
  {
-    unsigned long ioapic_phys;
      unsigned int i, idx = FIX_IO_APIC_BASE_0;
-    union IO_APIC_reg_01 reg_01;
- if ( smp_found_config )
-        nr_irqs_gsi = 0;
      for ( i = 0; i < nr_ioapics; i++ )
      {
-        if ( smp_found_config )
-        {
-            ioapic_phys = mp_ioapics[i].mpc_apicaddr;
-            if ( !ioapic_phys )
-            {
-                printk(KERN_ERR "WARNING: bogus zero IO-APIC address "
-                       "found in MPTABLE, disabling IO/APIC support!\n");
-                smp_found_config = false;
-                skip_ioapic_setup = true;
-                goto fake_ioapic_page;
-            }
-        }
-        else
+        union IO_APIC_reg_01 reg_01;
+        unsigned long ioapic_phys = mp_ioapics[i].mpc_apicaddr;
+
+        ioapic_phys = mp_ioapics[i].mpc_apicaddr;
+        if ( !ioapic_phys )
          {
- fake_ioapic_page:
-            ioapic_phys = __pa(alloc_xenheap_page());
-            clear_page(__va(ioapic_phys));
+            printk(KERN_ERR
+                   "WARNING: bogus zero IO-APIC address found in MPTABLE, disabling 
IO/APIC support!\n");
+            smp_found_config = false;
+            skip_ioapic_setup = true;
+            break;
          }
+
          set_fixmap_nocache(idx, ioapic_phys);
          apic_printk(APIC_VERBOSE, "mapped IOAPIC to %08Lx (%08lx)\n",
                      __fix_to_virt(idx), ioapic_phys);
@@ -2576,18 +2567,24 @@ void __init init_ioapic(void)
              continue;
          }
- if ( smp_found_config )
-        {
-            /* The number of IO-APIC IRQ registers (== #pins): */
-            reg_01.raw = io_apic_read(i, 1);
-            nr_ioapic_entries[i] = reg_01.bits.entries + 1;
-            nr_irqs_gsi += nr_ioapic_entries[i];
-
-            if ( rangeset_add_singleton(mmio_ro_ranges,
-                                        ioapic_phys >> PAGE_SHIFT) )
-                printk(KERN_ERR "Failed to mark IO-APIC page %lx read-only\n",
-                       ioapic_phys);
-        }
+        /* The number of IO-APIC IRQ registers (== #pins): */
+        reg_01.raw = io_apic_read(i, 1);
+        nr_ioapic_entries[i] = reg_01.bits.entries + 1;
+        nr_irqs_gsi += nr_ioapic_entries[i];
+
+        if ( rangeset_add_singleton(mmio_ro_ranges,
+                                    ioapic_phys >> PAGE_SHIFT) )
+            printk(KERN_ERR "Failed to mark IO-APIC page %lx read-only\n",
+                   ioapic_phys);
+    }
+}
+
+void __init init_ioapic(void)
+{
+    if ( smp_found_config )
+    {
+        nr_irqs_gsi = 0;

This would seem to also belong into the function, e.g. as part of
the loop header:

     for ( nr_irqs_gsi = i = 0; i < nr_ioapics; i++ )

While the initial value of the 'i' and 'nr_irqs_gsi' is the same, the variables have very different meaning and may confuse the reader.

So I would rather not follow your suggestion here. Although, I can move the zeroing right before the for loop.

Cheers,

--
Julien Grall



 


Rackspace

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