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

[Julien Grall <julien@xxxxxxx>]

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@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)
> >   {
> > -    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