Re: Re: 4.0/4.1 requests - IO-APIC EOI v4 [RFC]

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/09/11 07:50, Jan Beulich wrote:
>>>> On 09.09.11 at 18:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 09/09/11 17:22, Andrew Cooper wrote:
>>> ---SNIP---
>>>
>>> Version 3.  Applied Jan's suggestions, and extended some of the comments.
>>>
>> V4 - get the BUG_ON logic correct (oops).
>>
>> I have now done a few reboot tests and a kexec test with this patch.
>>
>> Are there any other tests you would suggest, or shall I formally submit
>> the patch for unstable and backporting?
>
> Looks technically good now, but there are a few cosmetic comments still:
>
>> --- a/xen/arch/x86/io_apic.c	Mon Sep 05 15:10:28 2011 +0100
>> +++ b/xen/arch/x86/io_apic.c	Fri Sep 09 17:20:49 2011 +0100
>> @@ -68,6 +68,16 @@ int __read_mostly nr_ioapics;
>> #define MAX_PLUS_SHARED_IRQS nr_irqs_gsi
>> #define PIN_MAP_SIZE (MAX_PLUS_SHARED_IRQS + nr_irqs_gsi)
>>
>> +
>> +#define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >= 0x20)
>> +
>> +static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin);
>> +static void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin);
> No need to declare these here if you move the definitions up before
> the first use (would fit well after ioapic_write_entry()).

Ok

>> +
>> +#define io_apic_eoi_vector(apic, vector) io_apic_eoi((apic), (vector), -1)
>> +#define io_apic_eoi_pin(apic, pin) io_apic_eoi((apic), -1, (pin))
>> +
>> +
>> /*
>>  * This is performance-critical, we want to do it O(1)
>>  *
>> ...
>> @@ -2622,3 +2621,124 @@ void __init init_ioapic_mappings(void)
>>     printk(XENLOG_INFO "IRQ limits: %u GSI, %u MSI/MSI-X\n",
>>            nr_irqs_gsi, nr_irqs - nr_irqs_gsi);
>> }
>> +
>> +/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
>> + * it should be worked out using the other.  This function disables interrupts
>> + * and takes the ioapic_lock */
>> +static void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
>> +{
>> +    unsigned int flags;
>> +    spin_lock_irqsave(&ioapic_lock, flags);
>> +    __io_apic_eoi(apic, vector, pin);
>> +    spin_unlock_irqrestore(&ioapic_lock, flags);
>> +}
>> +
>> +/* EOI an IO-APIC entry.  One of vector or pin may be -1, indicating that
>> + * it should be worked out using the other.  This function expect that the
>> + * ioapic_lock is taken, and interrupts are disabled (or there is a good reason
>> + * not to), and that if both pin and vector are passed, that they refer to the
>> + * same redirection entry in the IO-APIC. */
>> +static void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
>> +{
>> +    /* Ensure some useful information is passed in */
>> +    BUG_ON( !(vector == -1 && pin != -1) );
>> +    
>> +    /* Prefer the use of the EOI register if available */
>> +    if ( ioapic_has_eoi_reg(apic) )
>> +    {
>> +        /* If vector is unknown, read it from the IO-APIC */
>> +        if ( vector == -1 )
>> +            vector = __ioapic_read_entry(apic, pin, TRUE).vector;
>> +
>> +        *(IO_APIC_BASE(apic)+16) = vector;
>> +    }
>> +    else
>> +    {
>> +        /* Else fake an EOI by switching to edge triggered mode
>> +         * and back */
>> +        struct IO_APIC_route_entry entry;
>> +        bool_t need_to_unmask = 0;
>> +
>> +        /* If pin is unknown, search for it */
>> +        if ( pin == -1 )
>> +        {
>> +            unsigned int p;
>> +            for ( p = 0; p < nr_ioapic_registers[apic]; ++p )
>> +            {
>> +                entry = __ioapic_read_entry(apic, p, TRUE);
>> +                if ( entry.vector == vector )
>> +                {
>> +                    pin = p;
>> +                    /* break; */
>> +
>> +                    /* Here should be a break out of the loop, but at the 
> ... moment ...
>
>> +                     * Xen code doesn't actually prevent multiple IO-APIC
>> +                     * entries being assigned the same vector, so EOI all
>> +                     * pins which have the correct vector.
>> +                     *
>> +                     * Remove the following code when the above assertion
>> +                     * is fulfilled. */
>> +
> Why don't you just call __io_apic_eoi() recursively here?
>
> Jan

If I call the function recursively, it will loop forever.  Anyway, the
need to clear multiple pins is only temorary until George finishes his
per-device AMD interrupt remap patch which will enforce vector
uniqueness in each IO-APIC.  My expectation is that this issue will be
fixed in the next few weeks.

>> +                    if ( ! entry.mask )
>> +                    {
>> +                        /* If entry is not currently masked, mask it and make
>> +                         * a note to unmask it later */
>> +                        entry.mask = 1;
>> +                        __ioapic_write_entry(apic, pin, TRUE, entry);
>> +                        need_to_unmask = 1;
>> +                    }
>> +
>> +                    /* Flip the trigger mode to edge and back */
>> +                    entry.trigger = 0;
>> +                    __ioapic_write_entry(apic, pin, TRUE, entry);
>> +                    entry.trigger = 1;
>> +                    __ioapic_write_entry(apic, pin, TRUE, entry);
>> +
>> +                    if ( need_to_unmask )
>> +                    {
>> +                        /* Unmask if neccesary */
>> +                        entry.mask = 0;
>> +                        __ioapic_write_entry(apic, pin, TRUE, entry);
>> +                    }
>> +
>> +                }
>> +            }
>> +            
>> +            /* If search fails, nothing to do */
>> +
>> +            /* if ( pin == -1 ) */
>> +
>> +            /* Because the loop wasn't broken out of (see comment above),
>> +             * all relevant pins have been EOI, so we can always return.
>> +             * 
>> +             * Re-instate the if statement above when the Xen logic has been
>> +             * fixed.*/
>> +
>> +            return;
>> +        }
>> +
>> +        entry = __ioapic_read_entry(apic, pin, TRUE);
>> +
>> +        if ( ! entry.mask )
>> +        {
>> +            /* If entry is not currently masked, mask it and make
>> +             * a note to unmask it later */
>> +            entry.mask = 1;
>> +            __ioapic_write_entry(apic, pin, TRUE, entry);
>> +            need_to_unmask = 1;
>> +        }
>> +
>> +        /* Flip the trigger mode to edge and back */
>> +        entry.trigger = 0;
>> +        __ioapic_write_entry(apic, pin, TRUE, entry);
>> +        entry.trigger = 1;
>> +        __ioapic_write_entry(apic, pin, TRUE, entry);
>> +
>> +        if ( need_to_unmask )
>> +        {
>> +            /* Unmask if neccesary */
>> +            entry.mask = 0;
>> +            __ioapic_write_entry(apic, pin, TRUE, entry);
>> +        }
>> +    }
>> +}
>> ...

I will formally submit the patch for inclusion now.

-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com