[Xen-devel] [PATCH 0/3] xen/x86: Simplify ioapic_init()

[Xen-devel] [PATCH 0/3] xen/x86: Simplify ioapic_init()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

The main goal of this small series is to simplify ioapic_init().

Cheers,

Julien Grall (3):
  xen/x86: ioapic: Use true/false in bad_ioapic_register()
  xen/x86: ioapic: Rename init_ioapic_mappings() to init_ioapic()
  xen/x86: ioapic: Simplify ioapic_init()

 xen/arch/x86/apic.c           |  2 +-
 xen/arch/x86/io_apic.c        | 67 +++++++++++++++++------------------
 xen/include/asm-x86/io_apic.h |  2 +-
 3 files changed, 34 insertions(+), 37 deletions(-)

-- 
2.17.1

[Xen-devel] [PATCH 1/3] xen/x86: ioapic: Use true/false in bad_ioapic_register()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

bad_ioapic_register() is return a bool, so we should switch to
true/false.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/x86/io_apic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index e98e08e9c8..9868933287 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2531,10 +2531,10 @@ static __init bool bad_ioapic_register(unsigned int idx)
     {
         printk(KERN_WARNING "I/O APIC %#x registers return all ones, skipping!\n",
                mp_ioapics[idx].mpc_apicaddr);
-        return 1;
+        return true;
     }
 
-    return 0;
+    return false;
 }
 
 void __init init_ioapic_mappings(void)
-- 
2.17.1

[Xen-devel] [PATCH 2/3] xen/x86: ioapic: Rename init_ioapic_mappings() to init_ioapic()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The function init_ioapic_mappings() is doing more than initialization
mappings. It is also initialization the number of IRQs/GSIs supported.

So rename the function to init_ioapic(). This will allow us to re-use
the name in a follow-up patch.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/x86/apic.c           | 2 +-
 xen/arch/x86/io_apic.c        | 2 +-
 xen/include/asm-x86/io_apic.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index cde67cd87e..c7a54cafc3 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -978,7 +978,7 @@ __next:
         boot_cpu_physical_apicid = get_apic_id();
     x86_cpu_to_apicid[0] = get_apic_id();
 
-    init_ioapic_mappings();
+    init_ioapic();
 }
 
 /*****************************************************************************
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 9868933287..9a11ee8342 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2537,7 +2537,7 @@ static __init bool bad_ioapic_register(unsigned int idx)
     return false;
 }
 
-void __init init_ioapic_mappings(void)
+void __init init_ioapic(void)
 {
     unsigned long ioapic_phys;
     unsigned int i, idx = FIX_IO_APIC_BASE_0;
diff --git a/xen/include/asm-x86/io_apic.h b/xen/include/asm-x86/io_apic.h
index 998905186b..8c0af4bdd3 100644
--- a/xen/include/asm-x86/io_apic.h
+++ b/xen/include/asm-x86/io_apic.h
@@ -180,7 +180,7 @@ extern int io_apic_get_version (int ioapic);
 extern int io_apic_get_redir_entries (int ioapic);
 extern int io_apic_set_pci_routing (int ioapic, int pin, int irq, int edge_level, int active_high_low);
 
-extern void init_ioapic_mappings(void);
+extern void init_ioapic(void);
 
 extern void ioapic_suspend(void);
 extern void ioapic_resume(void);
-- 
2.17.1

[Xen-devel] [PATCH 3/3] xen/x86: ioapic: Simplify ioapic_init()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

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 <jgrall@amazon.com>
---
 xen/arch/x86/io_apic.c | 63 ++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index 9a11ee8342..3d52e4daf1 100644
--- 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;
+        init_ioapic_mappings();
     }
 
     nr_irqs_gsi = max(nr_irqs_gsi, highest_gsi() + 1);
-- 
2.17.1

Re: [Xen-devel] [PATCH 1/3] xen/x86: ioapic: Use true/false in bad_ioapic_register()

[Wei Liu]

On Fri, Mar 27, 2020 at 07:05:44PM +0000, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> bad_ioapic_register() is return a bool, so we should switch to
> true/false.

is return -> returns / is returning

> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Wei Liu <wl@xen.org>

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

[Wei Liu]

On Fri, Mar 27, 2020 at 07:05:46PM +0000, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> 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 <jgrall@amazon.com>
> ---
>  xen/arch/x86/io_apic.c | 63 ++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index 9a11ee8342..3d52e4daf1 100644
> --- 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;

ioapic_phys is set a second time here. See the line before.

Wei.

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

[Julien Grall]

Hi Wei,

On 29/03/2020 15:35, Wei Liu wrote:
> On Fri, Mar 27, 2020 at 07:05:46PM +0000, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> 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 <jgrall@amazon.com>
>> ---
>>   xen/arch/x86/io_apic.c | 63 ++++++++++++++++++++----------------------
>>   1 file changed, 30 insertions(+), 33 deletions(-)
>>
>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
>> index 9a11ee8342..3d52e4daf1 100644
>> --- 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;
> 
> ioapic_phys is set a second time here. See the line before.

Good spot! I will drop the line.

Cheers,

-- 
Julien Grall

Re: [Xen-devel] [PATCH 2/3] xen/x86: ioapic: Rename init_ioapic_mappings() to init_ioapic()

[Roger Pau Monné]

On Fri, Mar 27, 2020 at 07:05:45PM +0000, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The function init_ioapic_mappings() is doing more than initialization
> mappings. It is also initialization the number of IRQs/GSIs supported.
> 
> So rename the function to init_ioapic(). This will allow us to re-use
> the name in a follow-up patch.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I've got one comment/request however.

> ---
>  xen/arch/x86/apic.c           | 2 +-
>  xen/arch/x86/io_apic.c        | 2 +-
>  xen/include/asm-x86/io_apic.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
> index cde67cd87e..c7a54cafc3 100644
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -978,7 +978,7 @@ __next:
>          boot_cpu_physical_apicid = get_apic_id();
>      x86_cpu_to_apicid[0] = get_apic_id();
>  
> -    init_ioapic_mappings();
> +    init_ioapic();

I would rename this to ioapic_init instead since you are already
changing it. I usually prefer the subsystem name to be prefixed to the
action the function performs instead of the other way around.

Thanks, Roger.

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

[Roger Pau Monné]

On Fri, Mar 27, 2020 at 07:05:46PM +0000, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> 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 <jgrall@amazon.com>
> ---
>  xen/arch/x86/io_apic.c | 63 ++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
> index 9a11ee8342..3d52e4daf1 100644
> --- 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)

Likewise my comment in 2/3 I would name this ioapic_init_mappings.

>  {
> -    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;

Nit: paddr_t might be better here.

Thanks, Roger.

Re: [Xen-devel] [PATCH 2/3] xen/x86: ioapic: Rename init_ioapic_mappings() to init_ioapic()

[Julien Grall]

On 30/03/2020 11:37, Roger Pau Monné wrote:
> On Fri, Mar 27, 2020 at 07:05:45PM +0000, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The function init_ioapic_mappings() is doing more than initialization
>> mappings. It is also initialization the number of IRQs/GSIs supported.
>>
>> So rename the function to init_ioapic(). This will allow us to re-use
>> the name in a follow-up patch.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I've got one comment/request however.
> 
>> ---
>>   xen/arch/x86/apic.c           | 2 +-
>>   xen/arch/x86/io_apic.c        | 2 +-
>>   xen/include/asm-x86/io_apic.h | 2 +-
>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
>> index cde67cd87e..c7a54cafc3 100644
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -978,7 +978,7 @@ __next:
>>           boot_cpu_physical_apicid = get_apic_id();
>>       x86_cpu_to_apicid[0] = get_apic_id();
>>   
>> -    init_ioapic_mappings();
>> +    init_ioapic();
> 
> I would rename this to ioapic_init instead since you are already
> changing it. I usually prefer the subsystem name to be prefixed to the
> action the function performs instead of the other way around.

I will do it.

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi Roger,

On 30/03/2020 11:43, Roger Pau Monné wrote:
> On Fri, Mar 27, 2020 at 07:05:46PM +0000, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> 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 <jgrall@amazon.com>
>> ---
>>   xen/arch/x86/io_apic.c | 63 ++++++++++++++++++++----------------------
>>   1 file changed, 30 insertions(+), 33 deletions(-)
>>
>> diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
>> index 9a11ee8342..3d52e4daf1 100644
>> --- 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)
> 
> Likewise my comment in 2/3 I would name this ioapic_init_mappings.

Will do.

> 
>>   {
>> -    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;
> 
> Nit: paddr_t might be better here.

Sure.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/3] xen/x86: ioapic: Use true/false in bad_ioapic_register()

[Jan Beulich]

On 28.03.2020 12:01, Wei Liu wrote:
> On Fri, Mar 27, 2020 at 07:05:44PM +0000, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> bad_ioapic_register() is return a bool, so we should switch to
>> true/false.
> 
> is return -> returns / is returning
> 
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Wei Liu <wl@xen.org>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 2/3] xen/x86: ioapic: Rename init_ioapic_mappings() to init_ioapic()

[Jan Beulich]

On 30.03.2020 12:37, Roger Pau Monné wrote:
> On Fri, Mar 27, 2020 at 07:05:45PM +0000, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The function init_ioapic_mappings() is doing more than initialization
>> mappings. It is also initialization the number of IRQs/GSIs supported.
>>
>> So rename the function to init_ioapic(). This will allow us to re-use
>> the name in a follow-up patch.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I've got one comment/request however.
> 
>> ---
>>  xen/arch/x86/apic.c           | 2 +-
>>  xen/arch/x86/io_apic.c        | 2 +-
>>  xen/include/asm-x86/io_apic.h | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
>> index cde67cd87e..c7a54cafc3 100644
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -978,7 +978,7 @@ __next:
>>          boot_cpu_physical_apicid = get_apic_id();
>>      x86_cpu_to_apicid[0] = get_apic_id();
>>  
>> -    init_ioapic_mappings();
>> +    init_ioapic();
> 
> I would rename this to ioapic_init instead since you are already
> changing it. I usually prefer the subsystem name to be prefixed to the
> action the function performs instead of the other way around.

With this adjustment
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Jan Beulich]

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

Jan

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

[Julien Grall]

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

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

[Jan Beulich]

On 31.03.2020 13:51, Julien Grall wrote:
> 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.

I expected reservations like this, hence the "e.g."; i.e. ...

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

... I'm fine with this as well.

Jan