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

[Jan Beulich <jbeulich@xxxxxxxx>]

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