From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: Re: 4.0/4.1 requests - IO-APIC EOI v4 [RFC] Date: Mon, 12 Sep 2011 07:50:32 +0100 Message-ID: <4E6DC7D80200007800055AB1@nat28.tlf.novell.com> References: <4E689D9A.1020405@citrix.com> <4E68C6C10200007800055485@nat28.tlf.novell.com> <4E68C09B.8050409@citrix.com> <4E68E1D30200007800055501@nat28.tlf.novell.com> <4E68C9A9.8090707@citrix.com> <4E6A2B7F.6060006@citrix.com> <4E6A4F66020000780005589B@nat28.tlf.novell.com> <4E6A36EB.6000802@citrix.com> <4E6A54DB02000078000558B6@nat28.tlf.novell.com> <4E6A3D5B.5010104@citrix.com> <4E6A430B.30308@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <4E6A430B.30308@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Andrew Cooper Cc: Keir Fraser , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org >>> On 09.09.11 at 18:47, Andrew Cooper wrote: > On 09/09/11 17:22, Andrew Cooper wrote: >> ---SNIP--- >> >> Version 3. Applied Jan's suggestions, and extended some of the = comments. >> >=20 > V4 - get the BUG_ON logic correct (oops). >=20 > I have now done a few reboot tests and a kexec test with this patch. >=20 > 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) >=20 >+ >+#define ioapic_has_eoi_reg(apic) (mp_ioapics[(apic)].mpc_apicver >=3D = 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 =3D=3D -1 && pin !=3D -1) ); >+ =20 >+ /* 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 =3D=3D -1 ) >+ vector =3D __ioapic_read_entry(apic, pin, TRUE).vector; >+ >+ *(IO_APIC_BASE(apic)+16) =3D 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 =3D 0; >+ >+ /* If pin is unknown, search for it */ >+ if ( pin =3D=3D -1 ) >+ { >+ unsigned int p; >+ for ( p =3D 0; p < nr_ioapic_registers[apic]; ++p ) >+ { >+ entry =3D __ioapic_read_entry(apic, p, TRUE); >+ if ( entry.vector =3D=3D vector ) >+ { >+ pin =3D p; >+ /* break; */ >+ >+ /* Here should be a break out of the loop, but at = the=20 ... 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 =3D 1; >+ __ioapic_write_entry(apic, pin, TRUE, entry); >+ need_to_unmask =3D 1; >+ } >+ >+ /* Flip the trigger mode to edge and back */ >+ entry.trigger =3D 0; >+ __ioapic_write_entry(apic, pin, TRUE, entry); >+ entry.trigger =3D 1; >+ __ioapic_write_entry(apic, pin, TRUE, entry); >+ >+ if ( need_to_unmask ) >+ { >+ /* Unmask if neccesary */ >+ entry.mask =3D 0; >+ __ioapic_write_entry(apic, pin, TRUE, entry); >+ } >+ >+ } >+ } >+ =20 >+ /* If search fails, nothing to do */ >+ >+ /* if ( pin =3D=3D -1 ) */ >+ >+ /* Because the loop wasn't broken out of (see comment = above), >+ * all relevant pins have been EOI, so we can always return. >+ *=20 >+ * Re-instate the if statement above when the Xen logic has = been >+ * fixed.*/ >+ >+ return; >+ } >+ >+ entry =3D __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 =3D 1; >+ __ioapic_write_entry(apic, pin, TRUE, entry); >+ need_to_unmask =3D 1; >+ } >+ >+ /* Flip the trigger mode to edge and back */ >+ entry.trigger =3D 0; >+ __ioapic_write_entry(apic, pin, TRUE, entry); >+ entry.trigger =3D 1; >+ __ioapic_write_entry(apic, pin, TRUE, entry); >+ >+ if ( need_to_unmask ) >+ { >+ /* Unmask if neccesary */ >+ entry.mask =3D 0; >+ __ioapic_write_entry(apic, pin, TRUE, entry); >+ } >+ } >+} >...