Re: interrupt cleanup #2

[Cherry G.Mathew <cherry%zyx.in@localhost>]

Cherry G.Mathew <cherry%zyx.in@localhost> writes:


[...]

>
> The idea is to introduce the xen_pic structure, and use it minimally
> with no functional changes.

In this next patch, we use the xen_pic abstraction functionally.

Less obviously, we hide the irq, vector, port mappings behind a set of
functions, and use a 'token' as an opaque key/handle to access these
mappings.

This allows us to more fully bring out the semantic mess that is
irq/vector/xenpirq mappings on top of pin/pic relationships that
goes on behind the scenes.

Manuel, this one I'd greatly appreciate your thoughts and comments.

The idea of a 'token' is essentially an opaque handle, which the user
makes as few assumptions as possible as to the meaning of its
contents. See xen_pic.c for usage.

I'm not fully comfortable with how this is abstracted out yet, but this
is the best version that I could come up with so far. Taylor had asked
some questions previously about this patch.

Also, testing on hardware would be very useful.

-- 
~cherry

# HG changeset patch
# User Cherry G. Mathew <cherry%NetBSD.org@localhost>
# Date 1535450591 0
#      Tue Aug 28 10:03:11 2018 +0000
# Branch cherry-xen
# Node ID 6feb6e3f2668222a455b1450031ba5c68aca9ab8
# Parent  7dd2093f7c345f2ad6721ee5addd4e5653eb8d00
[mq]: 0004-Move-to-the-full-xen-route-add-breakout

diff -r 7dd2093f7c34 -r 6feb6e3f2668 sys/arch/x86/x86/intr.c
--- a/sys/arch/x86/x86/intr.c	Tue Aug 28 09:40:38 2018 +0000
+++ b/sys/arch/x86/x86/intr.c	Tue Aug 28 10:03:11 2018 +0000
@@ -1264,7 +1264,7 @@
 #if NPCI > 0 || NISA > 0
 	struct pintrhand *pih;
 	intr_handle_t irq;
-	int evtchn;
+	int evtchn, token;
 
 	KASSERTMSG(legacy_irq == -1 || (0 <= legacy_irq && legacy_irq < 256),
 	    "bad legacy IRQ value: %d", legacy_irq);
@@ -1290,7 +1290,6 @@
 	intrstr = intr_create_intrid(irq, pic, pin, intrstr_buf,
 	    sizeof(intrstr_buf));
 
-	evtchn = xen_pirq_alloc(&irq, type);
 	pih = kmem_zalloc(sizeof(struct pintrhand),
 	    cold ? KM_NOSLEEP : KM_SLEEP);
 	if (pih == NULL) {
@@ -1298,8 +1297,19 @@
 		return NULL;
 	}
 
+	extern struct cpu_info phycpu_info_primary; /* XXX */
+	struct cpu_info *ci = &phycpu_info_primary;
+	xen_pic.pic_addroute(pic, &phycpu_info_primary, irq, -1, type);
+
+	token = xen_pirq_get_token_from_pirq(irq);
+	evtchn = xen_pirq_get_port_from_token(xen_pic.pic_xen, token);
+	KASSERT(evtchn < NR_EVENT_CHANNELS);
+	KASSERT(evtchn >= 0);
+
+	pih->pic = pic;
 	pih->pic_type = pic->pic_type;
-	pih->pirq = irq & 0xff; /* This is just a handle that xen gives us */	
+	pih->pirq = token; /* This is just a handle that xen gives us */
+	pih->ci = ci;
 	pih->evtch = evtchn;
 	pih->func = handler;
 	pih->arg = arg;
diff -r 7dd2093f7c34 -r 6feb6e3f2668 sys/arch/x86/x86/ioapic.c
--- a/sys/arch/x86/x86/ioapic.c	Tue Aug 28 09:40:38 2018 +0000
+++ b/sys/arch/x86/x86/ioapic.c	Tue Aug 28 10:03:11 2018 +0000
@@ -552,8 +552,6 @@
 	ioapic_unlock(sc, flags);
 }
 
-extern struct xen_pic pic_xen; /* XXX: */
-
 static void
 ioapic_addroute(struct pic *pic, struct cpu_info *ci, int pin,
 		int idtvec, int type)
@@ -566,28 +564,6 @@
 	pp->ip_vector = idtvec;
 	pp->ip_cpu = ci;
 	apic_set_redir(sc, pin, idtvec, ci);
-
-#if defined(XEN)
-	/*
-	 * This is kludgy, and not the right place, but we can't bind
-	 * before the routing has been set to the appropriate 'vector'.
-	 * in x86/intr.c, this is done after idt_vec_set(), where this
-	 * would have been more appropriate to put this.
-	 */
-
-	int port, irq;
-	irq = pic_xen.vect2irq[idtvec];
-	KASSERT(irq != 0);
-	port = bind_pirq_to_evtch(irq);
-	KASSERT(port < NR_EVENT_CHANNELS);
-	KASSERT(port >= 0);
-
-	KASSERT(pic_xen.irq2port[irq] == 0);
-	pic_xen.irq2port[irq] = port + 1;
-
-	xen_atomic_set_bit(&ci->ci_evtmask[0], port);
-#endif
-
 }
 
 static void
@@ -596,15 +572,6 @@
 {
 
 	ioapic_hwmask(pic, pin);
-
-#if defined(XEN)
-	int port, irq;
-	irq = pic_xen.vect2irq[idtvec];
-	port = unbind_pirq_from_evtch(irq);
-
-	KASSERT(port < NR_EVENT_CHANNELS);
-#endif
-
 }
 
 #ifdef DDB
diff -r 7dd2093f7c34 -r 6feb6e3f2668 sys/arch/xen/include/intr.h
--- a/sys/arch/xen/include/intr.h	Tue Aug 28 09:40:38 2018 +0000
+++ b/sys/arch/xen/include/intr.h	Tue Aug 28 10:03:11 2018 +0000
@@ -85,7 +85,7 @@
 void xen_pirq_save_port_token(struct xen_pic *, int, int);
 int xen_pirq_get_port_from_token(struct xen_pic *, int);
 int xen_pirq_get_token_from_pirq(intr_handle_t);
-int xen_pirq_alloc(intr_handle_t *, int);
+int xen_pirq_alloc(intr_handle_t);
 #endif /* defined(DOM0OPS) || NPCI > 0 */
 
 #ifdef MULTIPROCESSOR
diff -r 7dd2093f7c34 -r 6feb6e3f2668 sys/arch/xen/x86/pintr.c
--- a/sys/arch/xen/x86/pintr.c	Tue Aug 28 09:40:38 2018 +0000
+++ b/sys/arch/xen/x86/pintr.c	Tue Aug 28 10:03:11 2018 +0000
@@ -156,14 +156,19 @@
 #endif
 
 #if defined(DOM0OPS) || NPCI > 0
-extern struct xen_pic pic_xen; /* XXX: */
-int
-xen_pirq_alloc(intr_handle_t *pirq, int type)
+/*
+ * Ask Xen for a physical CPU vector.
+ * Give xen a hint about our idea of what we think the irq should be
+ * if we were native.
+ *
+ */
+#if defined(NIOAPIC) 
+static int
+xen_assign_vector(int irqhint)
 {
+
 	physdev_op_t op;
-	int irq = *pirq;
-#if NIOAPIC > 0
-	extern struct cpu_info phycpu_info_primary; /* XXX */
+
 	/*
 	 * The hypervisor has already allocated vectors and IRQs for the
 	 * devices. Reusing the same IRQ doesn't work because as we bind
@@ -176,54 +181,180 @@
 	 * or none is available.
 	 */
 	static int xen_next_irq = 200;
-	struct ioapic_softc *ioapic = ioapic_find(APIC_IRQ_APIC(*pirq));
-	struct pic *pic = &ioapic->sc_pic;
-	int pin = APIC_IRQ_PIN(*pirq);
 
-	if (*pirq & APIC_INT_VIA_APIC) {
-		irq = pic_xen.vect2irq[ioapic->sc_pins[pin].ip_vector];
-		if (ioapic->sc_pins[pin].ip_vector == 0 || irq == 0) {
-			/* allocate IRQ */
-			irq = APIC_IRQ_LEGACY_IRQ(*pirq);
-			if (irq <= 0 || irq > 15)
-				irq = xen_next_irq--;
+	if (irqhint <= 0 || irqhint > 15) {
+		irqhint = xen_next_irq--;
+	}
+
 retry:
-			/* allocate vector and route interrupt */
-			op.cmd = PHYSDEVOP_ASSIGN_VECTOR;
-			op.u.irq_op.irq = irq;
-			if (HYPERVISOR_physdev_op(&op) < 0) {
-				irq = xen_next_irq--;
-				if (xen_next_irq == 15)
-					panic("PHYSDEVOP_ASSIGN_VECTOR irq %d", irq);
-				goto retry;
-			}
-			KASSERT(pic_xen.irq2vect[irq] == 0);
-			pic_xen.irq2vect[irq] = op.u.irq_op.vector;
-			KASSERT(pic_xen.vect2irq[op.u.irq_op.vector] == 0);
-			pic_xen.vect2irq[op.u.irq_op.vector] = irq;
-			pic->pic_addroute(pic, &phycpu_info_primary, pin,
-			    op.u.irq_op.vector, type);
-		}
-		*pirq &= ~0xff;
-		*pirq |= irq;
+	op.cmd = PHYSDEVOP_ASSIGN_VECTOR;
+	op.u.irq_op.irq = irqhint;
+	if (HYPERVISOR_physdev_op(&op) < 0) {
+		irqhint = xen_next_irq--;
+		if (xen_next_irq == 15)
+			panic("PHYSDEVOP_ASSIGN_VECTOR irq %d", irqhint);
+		goto retry;
+	}
+
+	return op.u.irq_op.vector;
+}
+#endif /* NIOAPIC */
+
+static int
+xen_assign_vector_legacy(int irqhint)
+{
+	physdev_op_t op;
+
+	KASSERT(APIC_IRQ_ISLEGACY(irqhint));
+
+	/* allocate vector and route interrupt */
+	op.cmd = PHYSDEVOP_ASSIGN_VECTOR;
+	op.u.irq_op.irq = irqhint;
+	if (HYPERVISOR_physdev_op(&op) < 0) {
+		panic("PHYSDEVOP_ASSIGN_VECTOR irq %d", irqhint);
+	}
+
+	return op.u.irq_op.vector;
+}
+
+static void
+xen_pirq_save_vector_token(struct xen_pic *xpic, int vector, int token)
+{
+	KASSERT(xpic->irq2vect[token] == 0);
+	xpic->irq2vect[token] = vector;
+
+	KASSERT(xpic->vect2irq[vector] == 0);
+	xpic->vect2irq[vector] = token;
+
+	KASSERT(xpic->irq2port[token] == 0);
+}
+
+int
+xen_pirq_get_vector_from_token(struct xen_pic *xpic, int token)
+ {
+	KASSERT(xpic->irq2vect[token] != 0);
+
+	return xpic->irq2vect[token];
+
+}
+
+int
+xen_pirq_get_token_from_vector(struct xen_pic *xpic, int vector)
+{
+	KASSERT(xpic->vect2irq[vector] != 0);
+	return xpic->vect2irq[vector];
+}
+
+void
+xen_pirq_save_port_token(struct xen_pic *xpic, int port, int token)
+{
+	KASSERT(xpic->irq2port[token] == 0);
+	xpic->irq2port[token] = port + 1;
+}
+
+int
+xen_pirq_get_port_from_token(struct xen_pic *xpic, int token)
+{
+	KASSERT(xpic->irq2port[token] > 0);
+	return xpic->irq2port[token] - 1;
+}
+
+int
+xen_pirq_get_token_from_pirq(intr_handle_t pirq)
+{
+	int token = pirq;
+#if NIOAPIC > 0
+	struct ioapic_softc *ioapic = ioapic_find(APIC_IRQ_APIC(pirq));
+	int pin = APIC_IRQ_PIN(pirq);
+
+	if (pirq & APIC_INT_VIA_APIC) {
+		token = xen_pirq_get_token_from_vector(xen_pic.pic_xen, ioapic->sc_pins[pin].ip_vector);
+		KASSERT(token != 0);
 	} else
 #endif /* NIOAPIC */
 	{
-		if (pic_xen.irq2port[irq] == 0) {
-			op.cmd = PHYSDEVOP_ASSIGN_VECTOR;
-			op.u.irq_op.irq = irq;
-			if (HYPERVISOR_physdev_op(&op) < 0) {
-				panic("PHYSDEVOP_ASSIGN_VECTOR irq %d", irq);
+		KASSERT(APIC_IRQ_ISLEGACY(pirq));
+		token = pirq; /* Redundant but semantically clear */
+	}
+
+	return token;
+}
+
+int
+xen_pirq_alloc(intr_handle_t pirq)
+{
+	int irq = pirq;
+#if NIOAPIC > 0
+	struct ioapic_softc *ioapic = ioapic_find(APIC_IRQ_APIC(pirq));
+	int pin = APIC_IRQ_PIN(pirq);
+
+	if (pirq & APIC_INT_VIA_APIC) {
+		irq = xen_pic.pic_xen->vect2irq[ioapic->sc_pins[pin].ip_vector];
+		if (ioapic->sc_pins[pin].ip_vector == 0 || irq == 0) {
+			int xen_vector; /*
+					 * The allocated vector xen
+					 * gives us
+					 */
+			int token; /*
+				    * Our handle to the vector that we
+				    * got from Xen. We use this to
+				    * tell Xen about this vector, for
+				    * binding, for eg:
+				    */
+
+			/* request vector */
+			xen_vector = xen_assign_vector(APIC_IRQ_LEGACY_IRQ(pirq));
+			token = xen_vector;
+
+			/*
+			 * On Native, the first 15 IRQs are identity
+			 * mapped to CPU IDT vector numbers. Our
+			 * current implementation seems to imply that
+			 * the dom0 needs to assume that the hardware
+			 * sources of these IRQs are fixed, and as
+			 * documented in the ISA spec, whereas,
+			 * the CPU vectors for the corresponding
+			 * callbacks can be re-routed to arbitrary
+			 * vectors by Xen (presumably via APIC
+			 * magic).
+			 *
+			 * XXX: cherry - it's unclear to me if current
+			 * hypervisor implementations follow any of
+			 * the above, but I'm maintaining the current
+			 * logic for clarity and any backwards compat
+			 * that I may have missed.
+			 *
+			 */
+
+			if (APIC_IRQ_LEGACY_IRQ(pirq) > 0 &&
+			    APIC_IRQ_LEGACY_IRQ(pirq) <= 15) {
+				token = APIC_IRQ_LEGACY_IRQ(pirq);
 			}
-			KASSERT(pic_xen.irq2vect[irq] == 0);
-			pic_xen.irq2vect[irq] = op.u.irq_op.vector;
-			KASSERT(pic_xen.vect2irq[op.u.irq_op.vector] == 0);
-			pic_xen.vect2irq[op.u.irq_op.vector] = irq;
-			KASSERT(pic_xen.irq2port[irq] == 0);
-			pic_xen.irq2port[irq] = bind_pirq_to_evtch(irq) + 1;
+
+			xen_pirq_save_vector_token(xen_pic.pic_xen,
+			    xen_vector, token);
+			/*XXX:*/ irq = token;
+		}
+	} else
+#endif /* NIOAPIC */
+	{
+		if (xen_pic.pic_xen->irq2port[irq] == 0) {
+			int xen_vector; /*
+					 * The allocated vector xen
+					 * gives us
+					 */
+			xen_vector = xen_assign_vector_legacy(irq);
+
+			xen_pirq_save_vector_token(xen_pic.pic_xen,
+			    xen_vector, irq);
 		}
 	}
-	KASSERT(pic_xen.irq2port[irq] > 0);
-	return (pic_xen.irq2port[irq] - 1);
+
+	/*
+	 * Although we call it 'irq', it really is just a
+	 * token
+	 */
+	return irq; 
 }
 #endif /* defined(DOM0OPS) || NPCI > 0 */
+
diff -r 7dd2093f7c34 -r 6feb6e3f2668 sys/arch/xen/x86/xen_pic.c
--- a/sys/arch/xen/x86/xen_pic.c	Tue Aug 28 09:40:38 2018 +0000
+++ b/sys/arch/xen/x86/xen_pic.c	Tue Aug 28 10:03:11 2018 +0000
@@ -52,7 +52,7 @@
 static void xen_pic_delroute(struct pic *, struct cpu_info *, int, int, int);
 static bool xen_pic_trymask(struct pic *, int);
 
-/* XXX: static */ struct xen_pic pic_xen = {
+static struct xen_pic pic_xen = {
 	.irq2port = {0},
 	.irq2vect = {0},
 	.vect2irq = {0}
@@ -158,10 +158,8 @@
 xen_pic_addroute(struct pic *pic, struct cpu_info *ci, int src, int dest, int type)
 {
 
-#if notyet
 	/* Events are simulated as level triggered interrupts */
 	evtchn_op_t op;
-#endif
 
 	KASSERT(ci != NULL);
 
@@ -169,7 +167,7 @@
 	case PIC_XEN:
 		/* Pass through! */
 		break;
-#if notyet
+
 #if defined(DOM0OPS) || NPCI > 0
 		int xen_vector, token; /* CPU vector, irq returned by xen */
 	case PIC_I8259:
@@ -232,14 +230,12 @@
 	case PIC_MSI:
 	case PIC_MSIX:
 #endif /*  defined(DOM0OPS) || NPCI > 0 */
-#endif /* notyet */
 	default:
 		panic("/* TODO */");
 		break;
 
 	}
 
-#if notyet
 	/* Bind dest port to the routed vcpu */
 	op.cmd = EVTCHNOP_bind_vcpu;
 	op.u.bind_vcpu.vcpu = ci->ci_cpuid;
@@ -249,7 +245,7 @@
 
 	if (HYPERVISOR_event_channel_op(&op) != 0)
 		panic("Failed to bind port %d to VCPU %"PRIuCPUID"\n", dest, ci->ci_cpuid);
-#endif /* Not yet */
+
 	/* Give the event handler a hint about our vcpu preference */
 	xen_atomic_set_bit(&ci->ci_evtmask[0], dest);
 
@@ -265,8 +261,6 @@
 		/* Pass through! */
 		break;
 
-#if notyet
-#if defined(DOM0OPS) || NPCI > 0 && notyet
 	case PIC_I8259:
 	case PIC_LAPIC:
 		/* NOP */
@@ -277,8 +271,6 @@
 		    pic->pic_ioapic->sc_pins[src].ip_vector, type);
 	case PIC_MSI:
 	case PIC_MSIX:
-#endif
-#endif
 	default:
 		panic("/* TODO */");
 		break;