[v2,3/3] xen/x86: ioapic: Simplify ioapic_init()

Commit Message

Julien Grall April 4, 2020, 10:26 a.m. UTC

From: Julien Grall <[e-mail]>

Since commit 9facd54a45 "x86/ioapic: Add register level checks to detect
bogus io-apic entries", Xen is able to cope with IO APICs not mapped in
the fixmap.

Therefore the whole logic to allocate a fake page for some IO APICs is
unnecessary.

With the logic removed, the code can be simplified a lot as we don't
need to go through all the IO APIC if SMP has not been detected or a
bogus zero IO-APIC address has been detected.

To avoid another level of tabulation, the simplification is now moved in
its own function.

Signed-off-by: Julien Grall <[e-mail]>

---
    Changes in v2:
        - Don't set ioapic_phys twice in a row
        - Rename init_ioapic_mappings() to ioapic_init_mappings()
        - Use paddr_t rather than unsigned long
        - Move nr_irq_gsis = 0 in ioapic_init_mappings()
---
 xen/arch/x86/io_apic.c | 61 +++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 33 deletions(-)

Comments

Jan Beulich April 6, 2020, 1:24 p.m. UTC | #1

On 04.04.2020 12:26, Julien Grall wrote:
> From: Julien Grall <[e-mail]>
> 
> Since commit 9facd54a45 "x86/ioapic: Add register level checks to detect
> bogus io-apic entries", Xen is able to cope with IO APICs not mapped in
> the fixmap.
> 
> Therefore the whole logic to allocate a fake page for some IO APICs is
> unnecessary.
> 
> With the logic removed, the code can be simplified a lot as we don't
> need to go through all the IO APIC if SMP has not been detected or a
> bogus zero IO-APIC address has been detected.
> 
> To avoid another level of tabulation, the simplification is now moved in
> its own function.
> 
> Signed-off-by: Julien Grall <[e-mail]>

Reviewed-by: Jan Beulich <[e-mail]>

Julien Grall April 10, 2020, 11:26 a.m. UTC | #2

Hi Jan,

On 06/04/2020 14:24, Jan Beulich wrote:
> On 04.04.2020 12:26, Julien Grall wrote:
>> From: Julien Grall <[e-mail]>
>>
>> Since commit 9facd54a45 "x86/ioapic: Add register level checks to detect
>> bogus io-apic entries", Xen is able to cope with IO APICs not mapped in
>> the fixmap.
>>
>> Therefore the whole logic to allocate a fake page for some IO APICs is
>> unnecessary.
>>
>> With the logic removed, the code can be simplified a lot as we don't
>> need to go through all the IO APIC if SMP has not been detected or a
>> bogus zero IO-APIC address has been detected.
>>
>> To avoid another level of tabulation, the simplification is now moved in
>> its own function.
>>
>> Signed-off-by: Julien Grall <[e-mail]>
> 
> Reviewed-by: Jan Beulich <[e-mail]>

Thank you! I took the liberty to push them.

Cheers,

Patch

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 8233eb44e1..878ee5192d 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2537,34 +2537,26 @@  static __init bool bad_ioapic_register(unsigned int idx)
     return false;
 }
 
-void __init ioapic_init(void)
+static void __init ioapic_init_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;
+    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;
+        paddr_t 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,19 +2568,22 @@  void __init ioapic_init(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 ioapic_init(void)
+{
+    if ( smp_found_config )
+        ioapic_init_mappings();
 
     nr_irqs_gsi = max(nr_irqs_gsi, highest_gsi() + 1);