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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Mar 27, 2020 at 07:05:46PM +0000, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
>  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.