diff --git a/cmd/pcitest/main.c b/cmd/pcitest/main.c index b08adbc..c4dd868 100644 --- a/cmd/pcitest/main.c +++ b/cmd/pcitest/main.c @@ -13,6 +13,10 @@ // // pcitest list the bus // pcitest claim that device, map BAR0, read it, allocate DMA +// pcitest x the same, then assert an interrupt and exit WITHOUT +// acking it -- which is what a driver that crashed looks +// like from the kernel's side, and the case that used to +// leave a level-triggered line asserted with no handler #include "types.h" #include "user.h" @@ -89,7 +93,7 @@ main(int argc, char *argv[]) } if (argc == 1) { - puts(" # bdf vendor:device class driver\n"); + puts(" # bdf vendor:device irq class driver\n"); for (int i = 0; i < count; i++) { struct pciuser_info in; in.index = (unsigned int)i; @@ -98,7 +102,8 @@ main(int argc, char *argv[]) puts(" "); putn(i); puts(" "); puthex(in.bus, 2); puts(":"); puthex(in.slot, 2); puts("."); puthex(in.func, 1); puts(" "); - puthex(in.vendor, 4); puts(":"); puthex(in.device, 4); puts(" "); + puthex(in.vendor, 4); puts(":"); puthex(in.device, 4); puts(" "); + if (in.irq == 0xFF) puts(" - "); else { putn(in.irq); puts(" "); } puts(class_name(in.class)); puts(" "); puts(in.driver[0] ? in.driver : "-"); @@ -191,6 +196,52 @@ main(int argc, char *argv[]) else puts(" DMA buffer did not read back correctly\n"); - puts("\nmechanisms 2 and 4 work: a driver could live out here.\n"); + // Mechanism 3. Ask for the device's interrupts, then make it interrupt. + // + // The e1000 family has an Interrupt Cause Set register at 0x00C8: writing + // a cause bit asserts the line exactly as the hardware would, which is the + // only way to test interrupt delivery without traffic arriving from + // somewhere. Reading ICR at 0x00C0 clears it, which is the ack the kernel + // cannot do for us. + if (in.vendor != 0x8086 || in.class != 0x02) { + puts("\nmechanisms 2 and 4 work: a driver could live out here.\n"); + puts("(interrupts untested -- that part knows only Intel NICs)\n"); + exit(); + } + + if (ioctl(fd, PCIIOC_IRQ, &idx) < 0) { + puts(" interrupts refused -- the line is shared with a kernel driver\n"); + puts("\nmechanisms 2 and 4 work: a driver could live out here.\n"); + exit(); + } + puts(" interrupts routed here (line "); putn(in.irq); puts(")\n"); + + volatile unsigned int *IMS = ®s[0x00D0 / 4]; // interrupt mask set + volatile unsigned int *ICS = ®s[0x00C8 / 4]; // interrupt cause set + volatile unsigned int *ICR = ®s[0x00C0 / 4]; // cause read, clears + + *IMS = 0x00000004; // LSC: link status change, a cause with no traffic + (void)*ICR; // clear anything already pending + *ICS = 0x00000004; // assert it + + // Crash simulation: assert the cause and leave, without reading or + // acking. The line is asserted and this process is about to stop existing. + if (argc > 2 && argv[2][0] == 'x') { + puts(" interrupt asserted; exiting WITHOUT acking (simulated crash)\n"); + exit(); + } + + unsigned long long n = 0; + if (read(fd, &n, sizeof n) != (int)sizeof n) { + puts(" read() did not return an interrupt\n"); + exit(); + } + puts(" interrupt delivered, count = "); putn((int)n); puts("\n"); + + unsigned int cause = *ICR; // ack at the device + puts(" ICR = 0x"); puthex(cause, 8); puts("\n"); + write(fd, &n, sizeof n); // re-arm + + puts("\nall three mechanisms work: a driver could live out here.\n"); exit(); } diff --git a/docs/plan/hybrid.md b/docs/plan/hybrid.md index 15cad48..5829d16 100644 --- a/docs/plan/hybrid.md +++ b/docs/plan/hybrid.md @@ -68,7 +68,41 @@ link-up set -- which is how you tell a working mapping from one that reads refused, and everything a process held is released when it exits, because a driver that crashed must not keep its hardware away from its replacement. -**Interrupts (mechanism 3) and the control channel (mechanism 1) are next.** +**Mechanism 3 exists too.** A process asks for a device's interrupts and then +reads them, uio-style: + +``` + interrupts routed here (line 10) + interrupt delivered, count = 1 + ICR = 0x80000004 +``` + +`0x80000004` is the e1000e's cause register with the bit the test asserted and +INT_ASSERTED set, read back by userspace as its acknowledgement. + +The line is **masked by the kernel on arrival and unmasked by userspace's +write()**, and that is the entire design rather than a detail. A PCI interrupt +line is level-triggered: it stays asserted until the cause is cleared *at the +device*, which is a different register on every chip and therefore something +only the driver can do. Signalling EOI on a line that is still asserted +re-enters the handler immediately and for ever -- 72,000 times a second, when +this kernel last did it by accident (history/kernel.md ยง13). + +So a driver that dies mid-interrupt leaves the line **masked, not unmasked**. +The instinct is the other way round -- do not leave the device dead -- and it +is wrong: unmasking an asserted line with no handler attached is a crashed +driver taking the machine down with it. Verified by killing one on purpose: + +``` + interrupt asserted; exiting WITHOUT acking (simulated crash) + timer: 3431 ticks taken, 3436 expected +``` + +Five ticks lost in seven seconds, which is the ordinary background. The next +driver to claim the device re-arms the line, so masked is safe without being +permanent -- also verified. + +**The control channel (mechanism 1) is what remains.** ### One thing the interface is deliberately not diff --git a/include/irq.h b/include/irq.h index c4285b2..47fd1be 100644 --- a/include/irq.h +++ b/include/irq.h @@ -35,6 +35,7 @@ typedef void (*irq_handler_t)(void *arg); int irq_register(int irq, irq_handler_t fn, void *arg, const char *name, int level_low); void irq_unregister(int irq, irq_handler_t fn, void *arg); +int irq_handler_count(int irq); // Called from trap() with the IOAPIC pin number, not the vector. void irq_dispatch(int irq); diff --git a/include/pciuser.h b/include/pciuser.h index bf25004..4a18f42 100644 --- a/include/pciuser.h +++ b/include/pciuser.h @@ -19,8 +19,7 @@ // - to be given it, exclusively CLAIM // - to reach its registers mmap(BAR) // - memory it can hand to the device DMA -// - to be told when the device wants attention (interrupts, -// not yet) +// - to be told when the device wants attention IRQ + read() // // There is no IOMMU. A process holding a DMA buffer can point the device at // any physical address, so this isolates a driver against its own *bugs* and @@ -81,6 +80,37 @@ struct pciuser_dma { #define PCIIOC_CLAIM 0x9002 // in: int index -- take it from the kernel #define PCIIOC_RELEASE 0x9003 // in: int index #define PCIIOC_DMA 0x9004 // struct pciuser_dma +#define PCIIOC_IRQ 0x9005 // in: int index -- deliver this device's + // interrupts to this process + +// Interrupts arrive by *reading* this device, which is how Linux's uio does it +// and for the same reason: a descriptor composes with poll, and a driver that +// stops reading falls behind rather than losing events. +// +// ioctl(fd, PCIIOC_IRQ, &index); +// for (;;) { +// unsigned long long n; +// read(fd, &n, sizeof n); // blocks until the device interrupts +// ... service the device, and ACK IT AT THE DEVICE ... +// write(fd, &n, sizeof n); // re-arm +// } +// +// **The write is not optional.** A PCI interrupt line is level-triggered: it +// stays asserted until the driver clears the cause *at the device*, and the +// kernel cannot do that because clearing it is device-specific -- it is a +// different register on every chip. So the kernel masks the line when the +// interrupt arrives and leaves it masked until the write says the device has +// been dealt with. +// +// Without that, EOI is signalled on a line that is still asserted and the +// interrupt re-fires immediately, for ever. That is not hypothetical: it +// happened in this kernel, at 72,000 interrupts a second, and what it looked +// like from userspace was a clock running seven times slow. +// See docs/history/kernel.md section 13. +// +// One device per process. A driver drives one thing; if you need two, run two. + + // Registers are reached with mmap() on this device, where the *offset is the // physical address of the BAR*: diff --git a/kernel/dev/irq.c b/kernel/dev/irq.c index 78aa994..44e9dfc 100644 --- a/kernel/dev/irq.c +++ b/kernel/dev/irq.c @@ -300,3 +300,17 @@ irq_info(char *buf, int cap) } return off; } + +// How many handlers are attached to a line. +// +// Asked by kernel/dev/pciuser.c before it gives a line to a userspace driver: +// that driver masks the line while it works, and a masked line is masked for +// everyone on it. Handing out a shared line would silence whichever kernel +// driver is also there, with no message and no obvious cause. +int +irq_handler_count(int irq) +{ + if (irq < 0 || irq >= NIRQ) + return 0; + return line[irq].n; +} diff --git a/kernel/dev/pciuser.c b/kernel/dev/pciuser.c index 5f8ebcd..c0a620c 100644 --- a/kernel/dev/pciuser.c +++ b/kernel/dev/pciuser.c @@ -27,6 +27,7 @@ #include "pci.h" #include "errno.h" #include "uaccess.h" +#include "irq.h" #include "pciuser.h" int mappages(pml4e_t *pgdir, void *va, uint64 size, uint64 pa, int perm); @@ -37,6 +38,57 @@ static struct proc *owner[NPCI]; static struct spinlock pciuser_lock; +// Interrupt delivery, one device per process. +// +// The count is what read() returns and what the reader compares against: a +// driver that was slow sees the count jump rather than missing an edge, which +// is the property that makes a count better than a flag. +struct irqdev { + struct proc *proc; // who reads, 0 when unused + int index; // pci_get() index + int irq; // the line + uint64 count; // interrupts taken + uint64 seen; // what the reader has consumed + int masked; +}; +static struct irqdev irqdev; + +// DMA buffers handed out, so they can be taken back. +// +// The arena is small and contiguous -- it has to be, a device cannot follow a +// free list -- so a driver that exits without returning its buffers exhausts +// it in a handful of restarts. Which is precisely the workload this interface +// invites: the whole promise is that a driver can crash and be restarted, and +// a promise that leaks a few kilobytes every time it is kept is not one. +#define PCIUSER_MAXDMA 16 +struct dmablk { + struct proc *proc; + void *kva; + uint64 size; +}; +static struct dmablk dmablk[PCIUSER_MAXDMA]; + +// The handler. Runs in interrupt context, so it does the two things that +// cannot wait and nothing else. +// +// Masking is the important one. The line is level-triggered and still asserted +// -- only the device's own driver can clear that, and its driver is a process +// that is not running yet. Signalling EOI on an asserted line re-enters here +// immediately and for ever; see docs/history/kernel.md section 13 for what +// that looks like from outside. So the line goes quiet until userspace says +// it has dealt with the device. +static void +pciuser_irq_handler(void *arg) +{ + (void)arg; + irqdev.count++; + if (!irqdev.masked) { + ioapicmask(irqdev.irq); + irqdev.masked = 1; + } + wakeup(&irqdev); +} + void pciuser_init(void) { @@ -89,6 +141,37 @@ pciuser_release_all(struct proc *p) for (int i = 0; i < NPCI; i++) if (owner[i] == p) owner[i] = 0; + + // And its interrupt line -- unregistered, and left MASKED. + // + // The instinct is to unmask on the way out, so the device is not left dead. + // That is exactly wrong, and for the reason running through this whole file: + // the driver died, so nobody cleared the cause at the device, so the line is + // very likely still asserted. Unmasking it with no handler attached means + // every EOI re-enters an empty dispatch, for ever -- a crashed driver taking + // the machine down with it, which is the precise opposite of what running + // drivers in processes is for. + // + // Masked is safe and it is not permanent: PCIIOC_IRQ arms the line when the + // next driver claims the device. + // Its DMA buffers. The process's page tables are torn down by exit() either + // way; what needs saying here is that the *arena* gets its pages back. + for (int i = 0; i < PCIUSER_MAXDMA; i++) { + if (dmablk[i].proc != p) + continue; + if (dmablk[i].kva) + dma_free(dmablk[i].kva, dmablk[i].size); + dmablk[i].proc = 0; + dmablk[i].kva = 0; + dmablk[i].size = 0; + } + + if (irqdev.proc == p) { + irq_unregister(irqdev.irq, pciuser_irq_handler, 0); + ioapicmask(irqdev.irq); + irqdev.masked = 1; + irqdev.proc = 0; + } release(&pciuser_lock); } @@ -221,10 +304,23 @@ do_dma(uint64 argp) if (size == 0 || size > 4 * 1024 * 1024) return -EINVAL; - void *kva = dma_alloc(size, PGSIZE); - if (!kva) + // A slot to remember it by, found before allocating: handing out memory that + // cannot be recorded is how the leak this fixes got written in the first + // place. + struct dmablk *blk = 0; + acquire(&pciuser_lock); + for (int i = 0; i < PCIUSER_MAXDMA; i++) + if (!dmablk[i].proc) { blk = &dmablk[i]; blk->proc = myproc(); break; } + release(&pciuser_lock); + if (!blk) return -ENOMEM; + void *kva = dma_alloc(size, PGSIZE); + if (!kva) { + blk->proc = 0; + return -ENOMEM; + } + // Its own mapping in the caller, taken from the same bump allocator mmap // uses so the two cannot collide. struct proc *p = myproc(); @@ -236,14 +332,65 @@ do_dma(uint64 argp) uint64 phys = dma_phys(kva); if (mappages(p->pgdir, (void *)va, size, phys, PTE_W | PTE_U) < 0) { dma_free(kva, size); + blk->proc = 0; return -ENOMEM; } + blk->kva = kva; + blk->size = size; req->uaddr = va; req->paddr = phys; return 0; } +// Hand a device's interrupts to the calling process. +static int +do_irq(uint64 argp) +{ + int idx; + if (copy_from_user(&idx, argp, sizeof(idx)) < 0) + return -EFAULT; + if (idx < 0 || idx >= pci_count() || idx >= NPCI) + return -ENODEV; + if (owner[idx] != myproc()) + return -EACCES; // claim it first + + const struct pci_dev *d = pci_get(idx); + if (!d || d->irq == 0xFF) + return -ENODEV; // the firmware routed it nowhere + + // Refuse a line the kernel is already listening on. + // + // This handler masks the line, and a masked line is masked for *everyone* on + // it. Doing that to a shared line silences whichever kernel driver is also + // there, and the symptom -- a device that simply stops, with nothing logged + // -- is about as hard to find as bugs get. MSI is the real answer, since it + // is per-device and cannot be shared; pci_msi_enable() is already in the + // tree waiting for a caller. + if (irq_handler_count(d->irq) > 0) + return -EBUSY; + + if (irqdev.proc && irqdev.proc != myproc()) + return -EBUSY; // one device per process, and one process + + irqdev.proc = myproc(); + irqdev.index = idx; + irqdev.irq = d->irq; + irqdev.count = 0; + irqdev.seen = 0; + irqdev.masked = 0; + + // Level-triggered and active low, like every PCI INTx line. + irq_register(d->irq, pciuser_irq_handler, 0, "userspace", 1); + + // Arm it. The line may have been left masked by whatever held this device + // before -- see pciuser_release_all -- and a driver that has to write() + // before its first read() to get going is an interface nobody will guess. + ioapicroute(d->irq, 0, 1); + irqdev.masked = 0; + return 0; +} + int pciuser_ioctl(uint64 request, uint64 argp) { @@ -257,23 +404,65 @@ pciuser_ioctl(uint64 request, uint64 argp) case PCIIOC_CLAIM: return do_claim(argp); case PCIIOC_RELEASE: return do_release(argp); case PCIIOC_DMA: return do_dma(argp); + case PCIIOC_IRQ: return do_irq(argp); default: return -ENOTTY; } } -// Reads and writes are not how this device is used -- registers come through -// mmap and everything else through ioctl -- but a device in devsw needs both, -// and returning 0 is a better answer than a null pointer dereference. +// Wait for the device to interrupt. +// +// Returns the total count rather than a flag, so a driver that was busy can +// see it missed three and act once -- which is what you want, because the +// alternative is either losing them or queueing them, and a device does not +// care how many times you noticed it. int pciuser_read(struct inode *ip, uint64 off, char *dst, int n) { - (void)ip; (void)off; (void)dst; (void)n; - return 0; + (void)off; + + if (irqdev.proc != myproc()) + return -1; + if (n < (int)sizeof(uint64)) + return -1; + + iunlock(ip); + acquire(&pciuser_lock); + while (irqdev.count == irqdev.seen) { + if (myproc()->killed) { + release(&pciuser_lock); + ilock(ip); + return -1; + } + sleep(&irqdev, &pciuser_lock); + } + uint64 c = irqdev.count; + irqdev.seen = c; + release(&pciuser_lock); + ilock(ip); + + memmove(dst, &c, sizeof(c)); + return (int)sizeof(c); } +// "I have dealt with the device" -- unmask the line. +// +// The driver must have cleared the interrupt *at the device* before this, or +// the line is still asserted and unmasking simply re-enters the handler. That +// is the contract, it cannot be checked from here, and getting it wrong is the +// 72,000-interrupts-a-second failure in docs/history/kernel.md section 13. int pciuser_write(struct inode *ip, uint64 off, char *src, int n) { (void)ip; (void)off; (void)src; + + if (irqdev.proc != myproc()) + return -1; + + acquire(&pciuser_lock); + if (irqdev.masked) { + ioapicroute(irqdev.irq, 0, 1); // level, active low: a PCI INTx line + irqdev.masked = 0; + } + release(&pciuser_lock); return n; }