Hi Keir, without the old IO-APIC part addressed, I wonder whether we should really have the backport of 23805:7048810180de ("IRQ: manually EOI migrating line interrupts") in both trees. I''d also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf() should always zero-terminate its output buffer") to 4.0 (I see it''s already in 4.1), as this not being fixed confuses ''e'' debug key output. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 08.09.11 at 11:19, "Jan Beulich" <JBeulich@suse.com> wrote: > Hi Keir, > > without the old IO-APIC part addressed, I wonder whether we should > really have the backport of 23805:7048810180de ("IRQ: manually EOI > migrating line interrupts") in both trees. > > I''d also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf() > should always zero-terminate its output buffer") to 4.0 (I see it''s > already in 4.1), as this not being fixed confuses ''e'' debug key output.Also shouldn''t we have a backport of 23112:84e3706df07a ("Passthrough: disable bus-mastering on any card that causes an IOMMU fault.") in the 4.0 tree (even if it doesn''t fully address the problem, yet - that''s the case in the other trees too)? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 08/09/2011 10:19, "Jan Beulich" <JBeulich@suse.com> wrote:> Hi Keir, > > without the old IO-APIC part addressed, I wonder whether we should > really have the backport of 23805:7048810180de ("IRQ: manually EOI > migrating line interrupts") in both trees.It does fix a real bug. Perhaps Andrew could hack up a patch to make it dependent on IO-APIC version?> I''d also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf() > should always zero-terminate its output buffer") to 4.0 (I see it''s > already in 4.1), as this not being fixed confuses ''e'' debug key output.Ok. -- Keir> Jan >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 08/09/11 11:17, Keir Fraser wrote:> On 08/09/2011 10:19, "Jan Beulich" <JBeulich@suse.com> wrote: > >> Hi Keir, >> >> without the old IO-APIC part addressed, I wonder whether we should >> really have the backport of 23805:7048810180de ("IRQ: manually EOI >> migrating line interrupts") in both trees. > It does fix a real bug. Perhaps Andrew could hack up a patch to make it > dependent on IO-APIC version?Now that I am not racing against a release deadline, this is a possibility. It probably means falling through to the "fake an EOI for line level interrupt code", as the Status bit is RO in the IO-APIC (The IRR bit is RW but both need updating) Does anyone know which revision of the IO-APIC was the first with an EOI register? If not, I think I have some document trawling to do.>> I''d also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf() >> should always zero-terminate its output buffer") to 4.0 (I see it''s >> already in 4.1), as this not being fixed confuses ''e'' debug key output. > Ok. > > -- Keir > >> Jan >> >-- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 08/09/2011 11:48, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:>>> Hi Keir, >>> >>> without the old IO-APIC part addressed, I wonder whether we should >>> really have the backport of 23805:7048810180de ("IRQ: manually EOI >>> migrating line interrupts") in both trees. >> It does fix a real bug. Perhaps Andrew could hack up a patch to make it >> dependent on IO-APIC version? > Now that I am not racing against a release deadline, this is a > possibility. It probably means falling through to the "fake an EOI for > line level interrupt code", as the Status bit is RO in the IO-APIC (The > IRR bit is RW but both need updating)Falling back to the old behaviour would be acceptable. I think we''re talking only very old systems here.> Does anyone know which revision of the IO-APIC was the first with an EOI > register?Jan''s patches which use the IOAPIC EOI register have a version check. You can copy that. -- Keir> If not, I think I have some document trawling to do._______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 08/09/2011 11:12, "Jan Beulich" <JBeulich@suse.com> wrote:>>>> On 08.09.11 at 11:19, "Jan Beulich" <JBeulich@suse.com> wrote: >> Hi Keir, >> >> without the old IO-APIC part addressed, I wonder whether we should >> really have the backport of 23805:7048810180de ("IRQ: manually EOI >> migrating line interrupts") in both trees. >> >> I''d also like you to add 23820:ba75234a6f56 ("bitmap_scnlistprintf() >> should always zero-terminate its output buffer") to 4.0 (I see it''s >> already in 4.1), as this not being fixed confuses ''e'' debug key output. > > Also shouldn''t we have a backport of 23112:84e3706df07a ("Passthrough: > disable bus-mastering on any card that causes an IOMMU fault.") in the > 4.0 tree (even if it doesn''t fully address the problem, yet - that''s the > case in the other trees too)?Now done. -- Keir> Jan >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 08.09.11 at 12:48, Andrew Cooper <andrew.cooper3@citrix.com> wrote:> > On 08/09/11 11:17, Keir Fraser wrote: >> On 08/09/2011 10:19, "Jan Beulich" <JBeulich@suse.com> wrote: >> >>> Hi Keir, >>> >>> without the old IO-APIC part addressed, I wonder whether we should >>> really have the backport of 23805:7048810180de ("IRQ: manually EOI >>> migrating line interrupts") in both trees. >> It does fix a real bug. Perhaps Andrew could hack up a patch to make it >> dependent on IO-APIC version? > Now that I am not racing against a release deadline, this is a > possibility. It probably means falling through to the "fake an EOI for > line level interrupt code", as the Status bit is RO in the IO-APIC (The > IRR bit is RW but both need updating) > > Does anyone know which revision of the IO-APIC was the first with an EOI > register?As Keir said, code already exists which does this version check. But to answer explicitly: It''s version 0x20. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
--- SNIP --- Attached is my first attempt at expanding the EOI to work on older IO-APICs. It has not undergone any significant testing yet. Does this look suitable? -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> On 08.09.11 at 15:18, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > --- SNIP --- > > Attached is my first attempt at expanding the EOI to work on older > IO-APICs. It has not undergone any significant testing yet. > > Does this look suitable?It looks correct, but I''d prefer not having two io_apic_eoi_*() functions: Namely in the "normal" __eoi_IO_APIC_irq() case you have pin *and* vector readily available, and hence there is no need to look up anything. So perhaps a better option would be to pass both pin and vector to io_apic_eoi() (using e.g. -1 to identify the "unknown-needs-lookup" case). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 08/09/11 14:40, Jan Beulich wrote:>>>> On 08.09.11 at 15:18, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> --- SNIP --- >> >> Attached is my first attempt at expanding the EOI to work on older >> IO-APICs. It has not undergone any significant testing yet. >> >> Does this look suitable? > It looks correct, but I''d prefer not having two io_apic_eoi_*() functions: > Namely in the "normal" __eoi_IO_APIC_irq() case you have pin *and* > vector readily available, and hence there is no need to look up anything. > So perhaps a better option would be to pass both pin and vector to > io_apic_eoi() (using e.g. -1 to identify the "unknown-needs-lookup" > case). > > JanOk - I will refactor with that suggestion. Also, the code in end_level_ioapic_irq() suggests masking the entry while playing with it, so I will include that as well. -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Sep-09 15:06 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v2 [RFC]
---SNIP--- v2 of the EOI patch. This provides one function to perform EOI''ing, taking a vector or a pin (or both). -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-09 15:39 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v2 [RFC]
>>> 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.> } > > /* >... >@@ -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.>+ /* 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.>+ } >+ >+ /* 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.>+ >+ 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?>+ >+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_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Sep-09 15:55 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v2 [RFC]
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-09 16:03 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v2 [RFC]
>>> On 09.09.11 at 17:55, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > 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...It indeed is not being taken, and as you say this is deliberate (we may be cleaning up in a crashed environment here).>>> + { >>> + /* 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.Sorry, I must have missed that. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Sep-09 16:22 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v3 [RFC]
---SNIP--- Version 3. Applied Jan''s suggestions, and extended some of the comments. -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Sep-09 16:47 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v4 [RFC]
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? -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Sep-09 16:50 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v4 [RFC]
On 09/09/11 17: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. >> > 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? >Remember to qrefresh this time. -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 225 900, http://www.citrix.com _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-12 06:50 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v4 [RFC]
>>> 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()).>+ >+#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@lists.xensource.com http://lists.xensource.com/xen-devel
Andrew Cooper
2011-Sep-12 10:15 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v4 [RFC]
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? > > JanIf 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-12 10:23 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v4 [RFC]
>>> On 12.09.11 at 12:15, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 12/09/11 07:50, Jan Beulich wrote: >>>>> On 09.09.11 at 18:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> + * 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, theWhy would it loop forever? You get in here only with pin == -1, and for the recursive call you''d pass the pin number you determined.> 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.Sure. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Sep-12 11:30 UTC
Re: [Xen-devel] Re: 4.0/4.1 requests - IO-APIC EOI v4 [RFC]
On 12/09/2011 11:23, "Jan Beulich" <JBeulich@suse.com> wrote:>>>> On 12.09.11 at 12:15, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 12/09/11 07:50, Jan Beulich wrote: >>>>>> On 09.09.11 at 18:47, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>>> + * 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 > > Why would it loop forever? You get in here only with pin == -1, and > for the recursive call you''d pass the pin number you determined.Exactly. Please re-send the patch with the recursive call. It makes the function quite a bit shorter. -- Keir>> 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. > > Sure. > > Jan >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel