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

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

On 09/09/11 16:39, Jan Beulich wrote:
>>>> On 09.09.11 at 17:06, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> @@ -397,18 +397,7 @@ static void clear_IO_APIC_pin(unsigned i
>>             entry.trigger = 1;
>>             __ioapic_write_entry(apic, pin, TRUE, entry);
>>         }
>> -        if (mp_ioapics[apic].mpc_apicver >= 0x20)
>> -            io_apic_eoi(apic, entry.vector);
>> -        else {
>> -            /*
>> -             * Mechanism by which we clear remoteIRR in this case is by
>> -             * changing the trigger mode to edge and back to level.
>> -             */
>> -            entry.trigger = 0;
>> -            __ioapic_write_entry(apic, pin, TRUE, entry);
>> -            entry.trigger = 1;
>> -            __ioapic_write_entry(apic, pin, TRUE, entry);
>> -        }
>> +        io_apic_eoi(apic, entry.vector, pin);
> This should be __io_apic_eoi() - all other functions called here are the
> non-locking ones, too.

Questionable - I traced the calls and at this point and cant see the
ioapic lock being taken.  I guess it might be safer overall to use
non-locking and leave the problem to whoever cleans up the irq code...

>>     }
>>
>>     /*
>> ...
>> @@ -2622,3 +2611,86 @@ 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 */
>> +void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
> static?
>
>> +{
>> +    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 */
>> +void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin)
> static!
>
>> +    {
>> +        /* Else fake an EOI by switching to edge triggered mode
>> +         * and back */
>> +        struct IO_APIC_route_entry entry;
>> +        bool_t need_to_unmask = 0;
>> +
> You may want to assert that at least one of vector and pin is not -1.

There is a BUG_ON at the top of the function if both vector and pin are -1.

>> +        /* If pin is unknown, search for it */
>> +        if ( pin == -1 )
>> +        {
>> +            unsigned int p;
>> +            for ( p = 0; p < nr_ioapic_registers[apic]; ++p )
>> +                if ( __ioapic_read_entry(apic, p, TRUE).vector == vector )
>> +                {
>> +                    pin = p;
>> +                    break;
> While we seem to agree that sharing of vectors within an IO-APIC must
> be prevented, I don't think this is currently being enforced, and hence
> you can't just "break" here - you need to handle all matching pins.

Good point - I will leave a comment to remove it when fixed.

>> +                }
>> +            
>> +            /* If search fails, nothing to do */
>> +            if ( pin == -1 )
>> +                return;
>> +        }
>> +
>> +        /* If vector is unknown, read it from the IO-APIC */
>> +        if ( vector == -1 )
>> +            vector = __ioapic_read_entry(apic, pin, TRUE).vector;
> You don't seem to use vector further down.

So I dont.

>> +
>> +        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);
>> +        }
>> +    }
>> +}
>> diff -r 0268e7380953 xen/include/asm-x86/io_apic.h
>> --- a/xen/include/asm-x86/io_apic.h	Mon Sep 05 15:10:28 2011 +0100
>> +++ b/xen/include/asm-x86/io_apic.h	Fri Sep 09 15:58:59 2011 +0100
>> @@ -157,10 +157,13 @@ static inline void io_apic_write(unsigne
>> 	__io_apic_write(apic, reg, value);
>> }
>>
>> -static inline void io_apic_eoi(unsigned int apic, unsigned int vector)
>> -{
>> -	*(IO_APIC_BASE(apic)+16) = vector;
>> -}
>> +#define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >= 0x20)
> Is this used outside of io_apic.c?

Not according to cscope - I will adjust them appropriately.

>> +
>> +void __io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin);
>> +void io_apic_eoi(unsigned int apic, unsigned int vector, unsigned int pin);
>> +
>> +#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))
> None of these should be either (see also above).
>
> Jan
>
>> /*
>>  * Re-write a value: to be used for read-modify-write
-- 
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com