| Message ID | 20200327190546.21580-4-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/x86: Simplify ioapic_init() | expand |
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.
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,
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.
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,
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
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,
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
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);