>>> On 09.09.11 at 18:47, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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()).
>+
>+#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 ( ! 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);
>+ }
>+ }
>+}
>...
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home