WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Jan Beulich <JBeulich@xxxxxxxx>
Subject: Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v2 [RFC]
From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Date: Fri, 9 Sep 2011 16:55:23 +0100
Cc: Keir Fraser <keir.xen@xxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Fri, 09 Sep 2011 08:56:16 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4E6A4F66020000780005589B@xxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <CA8E54AE.20876%keir.xen@xxxxxxxxx> <4E689D9A.1020405@xxxxxxxxxx> <4E68C6C10200007800055485@xxxxxxxxxxxxxxxxxxxx> <4E68C09B.8050409@xxxxxxxxxx> <4E68E1D30200007800055501@xxxxxxxxxxxxxxxxxxxx> <4E68C9A9.8090707@xxxxxxxxxx> <4E6A2B7F.6060006@xxxxxxxxxx> <4E6A4F66020000780005589B@xxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Lightning/1.0b2 Thunderbird/3.1.13
On 09/09/11 16:39, Jan Beulich wrote:
>>>> On 09.09.11 at 17:06, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel