[PATCH, resend] replacement for noirqdebug hack

[PATCH, resend] replacement for noirqdebug hack

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 249 bytes --]

Instead of re-establishing the noirqdebug hack earlier present in the i386 Linux code, communicate the information about
whether a particular IRQ is shared across domains from hypervisor to kernel.

Signed-off-by: Jan Beulich <jbeulich@novell.com>


[-- Attachment #2: xen-pirq_shared.patch --]
[-- Type: text/plain, Size: 4026 bytes --]

--- 2006-04-21/linux-2.6-xen-sparse/kernel/irq/handle.c	2006-03-20 06:53:29.000000000 +0100
+++ 2006-04-21/linux-2.6-xen-sparse/kernel/irq/handle.c	2006-04-27 10:16:35.000000000 +0200
@@ -12,6 +12,10 @@
 #include <linux/interrupt.h>
 #include <linux/kernel_stat.h>
 
+#ifdef CONFIG_XEN
+#include <asm/hypervisor.h>
+#endif
+
 #include "internals.h"
 
 /*
@@ -76,10 +80,19 @@ irqreturn_t no_action(int cpl, void *dev
 /*
  * Have got an event to handle:
  */
-fastcall int handle_IRQ_event(unsigned int irq, struct pt_regs *regs,
+fastcall /*irqreturn_t*/int handle_IRQ_event(unsigned int irq, struct pt_regs *regs,
 				struct irqaction *action)
 {
-	int ret, retval = 0, status = 0;
+	irqreturn_t ret, retval = IRQ_NONE;
+	unsigned int status = 0;
+
+#ifdef CONFIG_XEN
+	unsigned int pirq = irq_to_pirq(irq);
+
+	BUILD_BUG_ON(BITS_TO_LONGS(NR_PIRQS) > sizeof(((shared_info_t *)0)->pirq_shared));
+	if (pirq < NR_PIRQS && test_bit(pirq, HYPERVISOR_shared_info->pirq_shared))
+		retval = IRQ_HANDLED;
+#endif
 
 	if (!(action->flags & SA_INTERRUPT))
 		local_irq_enable();
--- 2006-04-21/xen/arch/x86/irq.c.0	2006-04-24 10:56:27.000000000 +0200
+++ 2006-04-21/xen/arch/x86/irq.c	2006-04-27 09:54:43.000000000 +0200
@@ -200,7 +200,7 @@ static void __do_IRQ_guest(int vector)
         if ( (action->ack_type != ACKTYPE_NONE) &&
              !test_and_set_bit(irq, d->pirq_mask) )
             action->in_flight++;
-        send_guest_pirq(d, irq);
+        send_guest_pirq(d, irq, action->nr_guests > 1);
     }
 }
 
--- 2006-04-21/xen/arch/ia64/xen/irq.c.0	2006-04-06 17:50:25.000000000 +0200
+++ 2006-04-21/xen/arch/ia64/xen/irq.c	2006-04-27 09:54:32.000000000 +0200
@@ -411,7 +411,7 @@ static void __do_IRQ_guest(int irq)
         if ( (action->ack_type != ACKTYPE_NONE) &&
              !test_and_set_bit(irq, &d->pirq_mask) )
             action->in_flight++;
-        send_guest_pirq(d, irq);
+        send_guest_pirq(d, irq, action->nr_guests > 1);
     }
 }
 
--- 2006-04-21/xen/common/event_channel.c.0	2006-04-06 17:50:26.000000000 +0200
+++ 2006-04-21/xen/common/event_channel.c	2006-04-27 09:57:36.000000000 +0200
@@ -539,13 +539,20 @@ void send_guest_virq(struct vcpu *v, int
 }
 
 
-void send_guest_pirq(struct domain *d, int pirq)
+void send_guest_pirq(struct domain *d, int pirq, int shared)
 {
     int port = d->pirq_to_evtchn[pirq];
     struct evtchn *chn;
+    shared_info_t *s = d->shared_info;
 
     ASSERT(port != 0);
 
+    BUILD_BUG_ON(BITS_TO_LONGS(NR_PIRQS) > sizeof(((shared_info_t *)0)->pirq_shared));
+    if (shared)
+        set_bit(pirq, s->pirq_shared);
+    else
+        clear_bit(pirq, s->pirq_shared);
+
     chn = evtchn_from_port(d, port);
     evtchn_set_pending(d->vcpu[chn->notify_vcpu_id], port);
 }
--- 2006-04-21/xen/include/public/xen.h.0	2006-04-07 10:05:06.000000000 +0200
+++ 2006-04-21/xen/include/public/xen.h	2006-04-27 11:36:53.000000000 +0200
@@ -408,6 +408,14 @@ typedef struct shared_info {
 
     struct arch_shared_info arch;
 
+    /*
+     * Indicator which PIRQs are shared with other domains, allowing the guest
+     * to not consider respective interrupts spurious even if no handler claims
+     * them, while still being able to detect spurious occurrences of un-shared
+     * interrupts.
+     */
+    unsigned long pirq_shared[256 / (8 * sizeof(unsigned long))];
+
 };
 typedef struct shared_info shared_info_t;
 
--- 2006-04-21/xen/include/xen/event.h.0	2006-04-06 17:50:27.000000000 +0200
+++ 2006-04-21/xen/include/xen/event.h	2006-04-27 09:54:21.000000000 +0200
@@ -35,8 +35,9 @@ extern void send_guest_virq(struct vcpu 
  * send_guest_pirq:
  *  @d:        Domain to which physical IRQ should be sent
  *  @pirq:     Physical IRQ number
+ *  @shared:   Indicator whether IRQ is shared with other domain(s)
  */
-extern void send_guest_pirq(struct domain *d, int pirq);
+extern void send_guest_pirq(struct domain *d, int pirq, int shared);
 
 #define evtchn_pending(d, p)                    \
     (test_bit((p), &(d)->shared_info->evtchn_pending[0]))

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH, resend] replacement for noirqdebug hack

[Keir Fraser]

On 2 Jun 2006, at 11:02, Jan Beulich wrote:

> Instead of re-establishing the noirqdebug hack earlier present in the 
> i386 Linux code, communicate the information about
> whether a particular IRQ is shared across domains from hypervisor to 
> kernel.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>

This is gross for a few reasons. Firstly it adds Xen cruft to a file we 
don't currently modify, and the changes would never be mergable 
upstream. Plus I'd prefer a lighter weight solution for handling this 
x86 crufty corner case -- adding extra bitmaps to shared_info and 
updating bits on every IRQ delivery is overkill imo.

What I would suggest is change note_interrupt() to only increment 
irqs_unhandled if some new function spurious_irqs_ok(irq) returns 
false. We can then define that function as Xen-specific and extend 
physdev_irq_status_query to be able to determine if an irq is shared 
across guests. We can avoid frequent hypercalls by caching the shared 
status the first time it evaluates true (so sharedness is sticky; I'd 
assume we would rarely take the path that executes the hypercall if the 
irq is really not shared).

  -- Keir

Re: [PATCH, resend] replacement for noirqdebug hack

[Jan Beulich]

>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 02.06.06 12:32 >>>
>
>On 2 Jun 2006, at 11:02, Jan Beulich wrote:
>
>> Instead of re-establishing the noirqdebug hack earlier present in the 
>> i386 Linux code, communicate the information about
>> whether a particular IRQ is shared across domains from hypervisor to 
>> kernel.
>>
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>
>This is gross for a few reasons. Firstly it adds Xen cruft to a file we 
>don't currently modify, and the changes would never be mergable 
>upstream. Plus I'd prefer a lighter weight solution for handling this 
>x86 crufty corner case -- adding extra bitmaps to shared_info and 
>updating bits on every IRQ delivery is overkill imo.

I don't think this has less chances of merging than other changes to pre-existing files. Besides that, I don't think
sharing of IRQs across domains is an x86-only issue. And where do you see overkill in updating a single bit?

>What I would suggest is change note_interrupt() to only increment 
>irqs_unhandled if some new function spurious_irqs_ok(irq) returns 
>false. We can then define that function as Xen-specific and extend 
>physdev_irq_status_query to be able to determine if an irq is shared 
>across guests. We can avoid frequent hypercalls by caching the shared 
>status the first time it evaluates true (so sharedness is sticky; I'd 
>assume we would rarely take the path that executes the hypercall if the 
>irq is really not shared).

That stickiness is clearly undesirable to me - if you bring up a domU for testing or other temporary purposes that
makes use of an interrupt already in use elsewhere, all other users of the interrupt will from there on not do proper
accounting anymore. Further, how would you want to prevent doing the hypercall if by default interrupts are non-shared
(I hope you didn't have other intentions) and hence the (sticky) flag wasn't set, yet.

Jan

Re: [PATCH, resend] replacement for noirqdebug hack

[Keir Fraser]

On 2 Jun 2006, at 12:01, Jan Beulich wrote:

> That stickiness is clearly undesirable to me - if you bring up a domU 
> for testing or other temporary purposes that
> makes use of an interrupt already in use elsewhere, all other users of 
> the interrupt will from there on not do proper
> accounting anymore.

It's hardly a core part of interrupt management. It'd just be disabling 
a code path that only matters for broken hardware. Now that the IOAPIC 
ACking is fixed we could arguably just disable that path completely 
again (noirqdebug). So I think all the interface changes and additions 
to shared_info are overkill because this is just enabling a 
broken-hardware workaround.

>  Further, how would you want to prevent doing the hypercall if by 
> default interrupts are non-shared
> (I hope you didn't have other intentions) and hence the (sticky) flag 
> wasn't set, yet.

We'd only execute the hypercall if the return code was IRQ_NONE.

  -- Keir

Re: [PATCH, resend] replacement for noirqdebug hack

[Keir Fraser]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]


On 2 Jun 2006, at 11:02, Jan Beulich wrote:

> Instead of re-establishing the noirqdebug hack earlier present in the 
> i386 Linux code, communicate the information about
> whether a particular IRQ is shared across domains from hypervisor to 
> kernel.
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>

How about the following patch, which is a cleaner modification to Linux 
and does not affect shared_info layout? Note that it depends on a Xen 
patch that is currently in the staging tree (extends the irq info 
hypercall to inform about sharing status), and that the patch is 
against xen-unstable. The status check is much slower than reading from 
shared memory, but ought to be on a rare path (unless you have a 
high-rate interrupt line shared across guests, which is going to be 
rather suckful anyway).

  -- Keir


[-- Attachment #2: ignore_shared_irqs.patch --]
[-- Type: application/octet-stream, Size: 1758 bytes --]

--- olinux-2.6.16.13/drivers/xen/Kconfig	Wed Jun  7 14:51:56 2006
+++ nlinux-2.6.16.13/drivers/xen/Kconfig	Wed Jun  7 16:11:05 2006
@@ -224,6 +224,10 @@
 	bool
 	default y
 
+config HAVE_IRQ_IGNORE_UNHANDLED
+	bool
+	default y
+
 config NO_IDLE_HZ
 	bool
 	default y
--- olinux-2.6.16.13/drivers/xen/core/evtchn.c	Wed Jun  7 14:51:56 2006
+++ nlinux-2.6,16.13/drivers/xen/core/evtchn.c	Wed Jun  7 16:11:05 2006
@@ -678,6 +678,13 @@
 	set_affinity_irq
 };
 
+int irq_ignore_unhandled(unsigned int irq)
+{
+	struct physdev_irq_status_query irq_status = { .irq = irq };
+	(void)HYPERVISOR_physdev_op(PHYSDEVOP_irq_status_query, &irq_status);
+	return !!(irq_status.flags & XENIRQSTAT_shared);
+}
+
 void resend_irq_on_evtchn(struct hw_interrupt_type *h, unsigned int i)
 {
 	int evtchn = evtchn_from_irq(i);
--- olinux-2.6.16.13/include/linux/interrupt.h	2006-05-02 22:38:44.000000000 +0100
+++ nlinux-2.6.16.13/include/linux/interrupt.h	2006-06-07 16:05:04.000000000 +0100
@@ -58,6 +58,12 @@
 extern void enable_irq(unsigned int irq);
 #endif
 
+#ifdef CONFIG_HAVE_IRQ_IGNORE_UNHANDLED
+int irq_ignore_unhandled(unsigned int irq);
+#else
+#define irq_ignore_unhandled(irq) 0
+#endif
+
 #ifndef __ARCH_SET_SOFTIRQ_PENDING
 #define set_softirq_pending(x) (local_softirq_pending() = (x))
 #define or_softirq_pending(x)  (local_softirq_pending() |= (x))
--- olinux-2.6.16.13/kernel/irq/spurious.c	2006-05-02 22:38:44.000000000 +0100
+++ nlinux-2.6.16.13/kernel/irq/spurious.c	2006-06-07 16:01:39.000000000 +0100
@@ -137,7 +137,8 @@
 			struct pt_regs *regs)
 {
 	if (action_ret != IRQ_HANDLED) {
-		desc->irqs_unhandled++;
+		if (!irq_ignore_unhandled(irq))
+			desc->irqs_unhandled++;
 		if (action_ret != IRQ_NONE)
 			report_bad_irq(irq, desc, action_ret);
 	}

[-- Attachment #3: Type: text/plain, Size: 1 bytes --]



[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH, resend] replacement for noirqdebug hack

[Jan Beulich]

>>> Keir Fraser <Keir.Fraser@cl.cam.ac.uk> 07.06.06 17:57 >>>
>
>How about the following patch, which is a cleaner modification to Linux 
>and does not affect shared_info layout? Note that it depends on a Xen 
>patch that is currently in the staging tree (extends the irq info 
>hypercall to inform about sharing status), and that the patch is 
>against xen-unstable. The status check is much slower than reading from 
>shared memory, but ought to be on a rare path (unless you have a 
>high-rate interrupt line shared across guests, which is going to be 
>rather suckful anyway).

Yes, that looks fine to me (assuming that the other patch you mention is the one introducing
PHYSDEVOP_irq_status_query, and that this is a non-intrusive addition).

Jan