From ea68ef4b54fc68ddddd85b48436e3facab6a09ea Mon Sep 17 00:00:00 2001 From: Nexoniarz Date: Sun, 13 Sep 2026 18:27:41 +0200 Subject: [PATCH] hybrid: the GPU driver now runs in userspace docs/plan/hybrid.md's first eviction. cmd/vgpud is the virtio-gpu driver as a process: it finds the device by vendor and device id, asks the kernel for it, and does the whole virtio 1.0 bring-up from outside -- reset, feature negotiation, a virtqueue in DMA memory, GET_DISPLAY_INFO, RESOURCE_CREATE_2D, ATTACH_BACKING, SET_SCANOUT, TRANSFER and FLUSH. vgpud: virtio-gpu at 00:03.0, currently held by virtio-gpu virtio-gpu: released to userspace; screen is the VBE framebuffer vgpud: claimed vgpud: display is 1280x800 vgpud: scanout is ours, framebuffer at physical 0x3eceb000 vgpud: drew a test pattern from userspace Verified by screendumping the virtio-gpu head: the pattern is there, with a checkerboard that would shear if the stride were wrong. The console carries on in VBE on the other head at the same time, which is the fallback rule from the plan working rather than being described. No syscall knows this is a GPU. The kernel handed over a PCI device and the process decided what it was -- which is the property that makes this a general mechanism, and the reason the evictions come before anything foreign. Four things the first real driver needed that the test program had not: - PCI config space (PCIIOC_CFG). A virtio device says where its registers are in vendor capabilities, which live in config space, so a driver that cannot read it cannot find its own registers. - a way for the incumbent to let go: struct pci_driver gained a release hook. A driver without one keeps its device, which is always safe and for anything on the path to a shell is also correct. - the outgoing driver freeing its DMA. Releasing the device but keeping its 4 MB framebuffer is half a handover -- the incoming driver asks for one and is told there is no room. - a 16 MB DMA arena. 4 MB was exactly one framebuffer at this resolution, and either side of a handover there are two. And one bug it reproduced on its first working run: vgpud polls the used ring, so it registered no interrupt handler, and a virtio device interrupts on every completion. The storm detector said so immediately -- irq 11: 77780 interrupts/second -- a device on this line is not acknowledging it. handler 'e1000' is attached -- naming the line, the rate and the neighbour it was drowning. One line fixes it (VRING_AVAIL_F_NO_INTERRUPT before the queue is enabled), and that detector was worth every minute it cost. include/virtio.h moved out of the driver directory. Its own comment said it should when a second reader appeared; one has. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VQ7Qx8E1q1M1bbCq8QLHS2 --- Makefile | 2 +- cmd/vgpud/main.c | 551 ++++++++++++++++++ docs/plan/hybrid.md | 55 ++ include/defs.h | 1 + include/dma.h | 13 +- include/pci.h | 16 + include/pciuser.h | 11 + .../gpu/virtio_gpu => include}/virtio.h | 23 +- kernel/dev/pci.c | 1 + kernel/dev/pciuser.c | 48 +- kernel/drivers/gpu/vga/vga_fb.c | 28 + kernel/drivers/gpu/virtio_gpu/virtio_gpu.c | 59 ++ 12 files changed, 799 insertions(+), 9 deletions(-) create mode 100644 cmd/vgpud/main.c rename {kernel/drivers/gpu/virtio_gpu => include}/virtio.h (83%) diff --git a/Makefile b/Makefile index ebb4c75..e7be12c 100644 --- a/Makefile +++ b/Makefile @@ -107,7 +107,7 @@ KERN_NOENTRY = $(filter-out $(BUILD)/kernel/arch/x86_64/entry.o, $(KERNOBJS)) # Third-party software does NOT go here — see ports/. # ----------------------------------------------------------------------- ULIB = $(addprefix $(BUILD)/lib/, start.o glue.o usys.o) -CMDS = init schinit schinitd fbinfo shmtest socktest inputtest nettest reflink schattr ping chvt pcitest +CMDS = init schinit schinitd fbinfo shmtest socktest inputtest nettest reflink schattr ping chvt pcitest vgpud CMDBINS = $(addprefix $(BUILD)/cmd/, $(CMDS)) # ----------------------------------------------------------------------- diff --git a/cmd/vgpud/main.c b/cmd/vgpud/main.c new file mode 100644 index 0000000..f6a43a4 --- /dev/null +++ b/cmd/vgpud/main.c @@ -0,0 +1,551 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// vgpud -- the virtio-gpu driver, outside the kernel. +// +// docs/plan/hybrid.md's first eviction. The same device the kernel drives in +// kernel/drivers/gpu/virtio_gpu/, driven instead by a process that asks the +// kernel for four things and nothing else: +// +// PCIIOC_CLAIM take the device (the kernel driver is asked to let go) +// PCIIOC_CFG read config space, to find the vendor capabilities +// mmap(BAR) reach the registers +// PCIIOC_DMA memory the device can be pointed at +// +// The point is not that this is better than the in-kernel driver -- it is the +// same code in a worse language for the job. The point is that it is a real +// driver rather than a test program, so the interface has something to be +// judged by, and what it asks for is what a foreign driver would ask for too. +// +// Note what is *not* here: no syscall knows this is a GPU. The kernel handed +// over a PCI device and this decided it was a GPU, which is the property that +// makes the interface general rather than a hole shaped like one thing. + +#include "types.h" +#include "user.h" +#include "fcntl.h" +#include "posix.h" +#include "pciuser.h" +#include "virtio.h" + +// --- small output, since lib/ has no printf --------------------------------- + +static void +puts(const char *s) +{ + int n = 0; + while (s[n]) n++; + write(1, (void *)s, n); +} + +static void +putn(long long v) +{ + char b[24]; + int i = 0; + if (v < 0) { write(1, "-", 1); v = -v; } + if (v == 0) { write(1, "0", 1); return; } + while (v > 0) { b[i++] = '0' + (int)(v % 10); v /= 10; } + while (i-- > 0) write(1, &b[i], 1); +} + +static void +puthex(unsigned long long v, int digits) +{ + static const char h[] = "0123456789abcdef"; + char b[17]; + for (int i = digits - 1; i >= 0; i--) { b[i] = h[v & 0xF]; v >>= 4; } + write(1, b, digits); +} + +static void +die(const char *why) +{ + puts("vgpud: "); puts(why); puts("\n"); + exit(); +} + +// --- register access -------------------------------------------------------- + +static inline unsigned char r8 (volatile unsigned char *p, int o) { return p[o]; } +static inline void w8 (volatile unsigned char *p, int o, unsigned char v) { p[o] = v; } +static inline unsigned short r16(volatile unsigned char *p, int o) { return *(volatile unsigned short *)(p + o); } +static inline void w16(volatile unsigned char *p, int o, unsigned short v) { *(volatile unsigned short *)(p + o) = v; } +static inline unsigned int r32(volatile unsigned char *p, int o) { return *(volatile unsigned int *)(p + o); } +static inline void w32(volatile unsigned char *p, int o, unsigned int v) { *(volatile unsigned int *)(p + o) = v; } +static inline void w64(volatile unsigned char *p, int o, unsigned long long v) +{ + // Two 32-bit writes, low half first. The virtio spec requires it and the + // device latches on the high write; a single 64-bit store is a different + // bus transaction and some implementations simply drop it. + w32(p, o, (unsigned int)v); + w32(p, o + 4, (unsigned int)(v >> 32)); +} + +// --- the device ------------------------------------------------------------- + +static int pcifd; +static int index_; + +static volatile unsigned char *common; +static volatile unsigned char *notify; +static volatile unsigned char *isr; +static unsigned int notify_mult; + +static struct pciuser_info info; + +static unsigned int +cfg32(unsigned int off) +{ + struct pciuser_cfg c; + c.index = (unsigned int)index_; + c.offset = off & ~3u; + c.value = 0; + if (ioctl(pcifd, PCIIOC_CFG, &c) < 0) + die("config read failed"); + return c.value; +} + +static unsigned char +cfg8(unsigned int off) +{ + return (unsigned char)(cfg32(off) >> ((off & 3) * 8)); +} + +// Find a virtio configuration structure. +// +// A virtio 1.0 device does not put its registers at a fixed offset. It walks +// you there: a vendor capability in config space names which BAR, and how far +// into it, each structure lives. So a driver that cannot read config space +// cannot find its own registers -- which is why PCIIOC_CFG exists, and why the +// test program that came before this never needed it. +static volatile unsigned char * +find_cfg(int want, unsigned int *out_mult) +{ + unsigned char off = cfg8(0x34) & 0xFC; + + while (off) { + unsigned char id = cfg8(off); + unsigned char next = cfg8(off + 1); + + if (id == PCI_CAP_ID_VNDR) { + unsigned char type = cfg8(off + 3); + unsigned char bar = cfg8(off + 4); + unsigned int o = cfg32(off + 8); + unsigned int len = cfg32(off + 12); + + if (type == want && bar < PCIUSER_MAXBAR && info.bar[bar].base) { + if (out_mult) + *out_mult = cfg32(off + 16); + + // Map from the start of the BAR, not from the capability's + // offset: two capabilities usually live in one BAR, and + // mapping each separately would hand out two mappings of + // overlapping pages. + unsigned long long want_len = (unsigned long long)o + len; + if (want_len < 4096) want_len = 4096; + if (want_len > info.bar[bar].size) want_len = info.bar[bar].size; + + volatile unsigned char *p = + (volatile unsigned char *)mmap(0, want_len, + PROT_READ | PROT_WRITE, + MAP_SHARED, pcifd, + info.bar[bar].base); + if (p == 0 || (long)p == -1) + return 0; + return p + o; + } + } + off = next & 0xFC; + } + return 0; +} + +// --- the queue -------------------------------------------------------------- + +#define CTRL_QUEUE 0 + +static struct vring_desc *desc; +static struct vring_avail *avail; +static struct vring_used *used; +static unsigned long long ring_phys; +static unsigned short qsize, notify_off, last_used, next_desc; + +static void *cmdbuf, *respbuf; +static unsigned long long cmdphys, respphys; + +static void +queue_setup(void) +{ + w16(common, VCOMMON_Q_SELECT, CTRL_QUEUE); + qsize = r16(common, VCOMMON_Q_SIZE); + if (qsize == 0) + die("control queue has no slots"); + if (qsize > 64) + qsize = 64; // more than one command in flight is not + // something this driver has any use for + w16(common, VCOMMON_Q_SIZE, qsize); + notify_off = r16(common, VCOMMON_Q_NOTIFY_OFF); + + // One contiguous block for the three rings, laid out the way the spec + // requires: descriptors, then available, then used, each aligned. The + // device reads all three by physical address, so they have to come from + // the kernel's DMA arena rather than from anything this process could + // allocate for itself. + unsigned int desc_sz = 16 * qsize; + unsigned int avail_sz = 6 + 2 * qsize; + + struct pciuser_dma d; + d.size = 16384; + if (ioctl(pcifd, PCIIOC_DMA, &d) < 0) + die("no DMA memory for the queue"); + + unsigned long long base = d.uaddr; + unsigned long long basep = d.paddr; + + unsigned int off_avail = (desc_sz + 1) & ~1u; + unsigned int off_used = (off_avail + avail_sz + 3) & ~3u; + + desc = (struct vring_desc *)(base); + avail = (struct vring_avail *)(base + off_avail); + used = (struct vring_used *)(base + off_used); + ring_phys = basep; + + for (unsigned int i = 0; i < 16384; i++) + ((volatile unsigned char *)base)[i] = 0; + + // Do not interrupt us. docmd() polls the used ring, so an interrupt is + // not merely unwanted, it is dangerous: this process holds the device but + // has no handler registered, and the line is shared with the e1000. A + // level-triggered line nobody acknowledges re-fires for ever -- 77,000 + // times a second, which is what the first working run of this driver did + // before this line existed. The storm detector in kernel/dev/irq.c named + // the line and the neighbour it was drowning. + avail->flags = VRING_AVAIL_F_NO_INTERRUPT; + __sync_synchronize(); + + w64(common, VCOMMON_Q_DESC, basep); + w64(common, VCOMMON_Q_DRIVER, basep + off_avail); + w64(common, VCOMMON_Q_DEVICE, basep + off_used); + w16(common, VCOMMON_Q_ENABLE, 1); + + // Command and response buffers, also from the arena for the same reason. + struct pciuser_dma b; + b.size = 8192; + if (ioctl(pcifd, PCIIOC_DMA, &b) < 0) + die("no DMA memory for command buffers"); + cmdbuf = (void *)b.uaddr; + cmdphys = b.paddr; + respbuf = (void *)(b.uaddr + 4096); + respphys = b.paddr + 4096; +} + +// Submit one command and wait for the answer. +// +// Polled, not interrupt-driven. This driver issues a handful of commands at +// startup and one flush per frame, and a poll on a used-ring index that the +// device updates by DMA is both simpler and lower-latency than waking a +// process. The interrupt path exists (PCIIOC_IRQ) and belongs in whatever +// wants to sleep between frames. +static int +docmd(unsigned int cmdlen, unsigned int resplen) +{ + unsigned short head = next_desc; + + desc[head].addr = cmdphys; + desc[head].len = cmdlen; + desc[head].flags = VRING_DESC_F_NEXT; + desc[head].next = (unsigned short)(head + 1); + + desc[head + 1].addr = respphys; + desc[head + 1].len = resplen; + desc[head + 1].flags = VRING_DESC_F_WRITE; + desc[head + 1].next = 0; + + avail->ring[avail->idx % qsize] = head; + __sync_synchronize(); + avail->idx++; + __sync_synchronize(); + + *(volatile unsigned short *)(notify + notify_off * notify_mult) = CTRL_QUEUE; + + // Spin with a bound. A device that never answers is a bug somewhere, and + // hanging for ever is a worse way to report it than saying so. + for (int spin = 0; spin < 100000000; spin++) { + __sync_synchronize(); + if (used->idx != last_used) { + last_used = used->idx; + next_desc = (unsigned short)((next_desc + 2) % qsize); + return 0; + } + } + return -1; +} + +static void +hdr_init(void *p, unsigned int type) +{ + struct virtio_gpu_ctrl_hdr *h = (struct virtio_gpu_ctrl_hdr *)p; + for (unsigned int i = 0; i < sizeof(*h); i++) ((unsigned char *)h)[i] = 0; + h->type = type; +} + +static int +resp_ok(void) +{ + struct virtio_gpu_ctrl_hdr *h = (struct virtio_gpu_ctrl_hdr *)respbuf; + return h->type == VIRTIO_GPU_RESP_OK_NODATA || + h->type == VIRTIO_GPU_RESP_OK_DISPLAY_INFO; +} + +// --- bringing the device up ------------------------------------------------- + +static int width, height; +static unsigned long long fb_uaddr, fb_paddr; +static unsigned int fb_bytes; + +#define RESOURCE_ID 1 + +static void +bring_up(void) +{ + // Reset, then the status handshake in the order the spec sets out. A + // device that finds the sequence wrong fails the feature step rather than + // telling you which line was at fault, so it is written out plainly. + w8(common, VCOMMON_STATUS, 0); + while (r8(common, VCOMMON_STATUS) != 0) + ; + w8(common, VCOMMON_STATUS, VSTATUS_ACK); + w8(common, VCOMMON_STATUS, VSTATUS_ACK | VSTATUS_DRIVER); + + w32(common, VCOMMON_DFEAT_SEL, 1); + unsigned int hi = r32(common, VCOMMON_DFEAT); + if (!(hi & (1u << (VIRTIO_F_VERSION_1 - 32)))) { + w8(common, VCOMMON_STATUS, VSTATUS_FAILED); + die("device is not virtio 1.0"); + } + + w32(common, VCOMMON_GFEAT_SEL, 0); + w32(common, VCOMMON_GFEAT, 0); + w32(common, VCOMMON_GFEAT_SEL, 1); + w32(common, VCOMMON_GFEAT, 1u << (VIRTIO_F_VERSION_1 - 32)); + + w8(common, VCOMMON_STATUS, VSTATUS_ACK | VSTATUS_DRIVER | VSTATUS_FEATURES_OK); + if (!(r8(common, VCOMMON_STATUS) & VSTATUS_FEATURES_OK)) { + w8(common, VCOMMON_STATUS, VSTATUS_FAILED); + die("device refused the feature set"); + } + + queue_setup(); + + w8(common, VCOMMON_STATUS, + VSTATUS_ACK | VSTATUS_DRIVER | VSTATUS_FEATURES_OK | VSTATUS_DRIVER_OK); + + // What size is the screen? + hdr_init(cmdbuf, VIRTIO_GPU_CMD_GET_DISPLAY_INFO); + if (docmd(sizeof(struct virtio_gpu_ctrl_hdr), + sizeof(struct virtio_gpu_resp_display_info)) < 0) + die("the device never answered GET_DISPLAY_INFO"); + if (!resp_ok()) + die("GET_DISPLAY_INFO refused"); + + struct virtio_gpu_resp_display_info *di = + (struct virtio_gpu_resp_display_info *)respbuf; + width = (int)di->pmodes[0].r.width; + height = (int)di->pmodes[0].r.height; + if (width <= 0 || height <= 0) + die("the device reports no display"); + + puts("vgpud: display is "); putn(width); puts("x"); putn(height); puts("\n"); + + // A 2D resource the size of the screen. + struct virtio_gpu_resource_create_2d *c = + (struct virtio_gpu_resource_create_2d *)cmdbuf; + hdr_init(c, VIRTIO_GPU_CMD_RESOURCE_CREATE_2D); + c->resource_id = RESOURCE_ID; + c->format = VIRTIO_GPU_FORMAT_B8G8R8X8_UNORM; + c->width = (unsigned int)width; + c->height = (unsigned int)height; + if (docmd(sizeof(*c), sizeof(struct virtio_gpu_ctrl_hdr)) < 0 || !resp_ok()) + die("RESOURCE_CREATE_2D refused"); + + // Memory behind it. This is the framebuffer: the process writes here and + // the device reads it, which is the entire reason PCIIOC_DMA reports a + // physical address. + fb_bytes = (unsigned int)(width * height * 4); + + struct pciuser_dma d; + d.size = fb_bytes; + if (ioctl(pcifd, PCIIOC_DMA, &d) < 0) { + puts("vgpud: wanted "); putn(fb_bytes / 1024); + puts(" KB of DMA memory for the framebuffer\n"); + die("not enough DMA memory -- see DMA_ARENA_BYTES"); + } + fb_uaddr = d.uaddr; + fb_paddr = d.paddr; + + struct virtio_gpu_resource_attach_backing *a = + (struct virtio_gpu_resource_attach_backing *)cmdbuf; + hdr_init(a, VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING); + a->resource_id = RESOURCE_ID; + a->nr_entries = 1; + // The entries follow the struct rather than being in it: the count is + // variable, so the command is a header with an array glued on the end. + struct virtio_gpu_mem_entry *e = (struct virtio_gpu_mem_entry *)(a + 1); + e->addr = fb_paddr; + e->length = fb_bytes; + e->padding = 0; + if (docmd(sizeof(*a) + sizeof(*e), sizeof(struct virtio_gpu_ctrl_hdr)) < 0 || + !resp_ok()) + die("RESOURCE_ATTACH_BACKING refused"); + + // Put it on the screen. + struct virtio_gpu_set_scanout *so = (struct virtio_gpu_set_scanout *)cmdbuf; + hdr_init(so, VIRTIO_GPU_CMD_SET_SCANOUT); + so->r.x = so->r.y = 0; + so->r.width = (unsigned int)width; + so->r.height = (unsigned int)height; + so->scanout_id = 0; + so->resource_id = RESOURCE_ID; + if (docmd(sizeof(*so), sizeof(struct virtio_gpu_ctrl_hdr)) < 0 || !resp_ok()) + die("SET_SCANOUT refused"); + + puts("vgpud: scanout is ours, framebuffer at physical 0x"); + puthex(fb_paddr, 16); puts("\n"); +} + +// Push what has been drawn to the host. Two commands, and both are needed: +// the transfer copies from the backing into the device's copy of the resource, +// the flush tells it to put that on screen. +static void +present(void) +{ + struct virtio_gpu_transfer_to_host_2d *t = + (struct virtio_gpu_transfer_to_host_2d *)cmdbuf; + hdr_init(t, VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D); + t->r.x = t->r.y = 0; + t->r.width = (unsigned int)width; + t->r.height = (unsigned int)height; + t->offset = 0; + t->resource_id = RESOURCE_ID; + t->padding = 0; + if (docmd(sizeof(*t), sizeof(struct virtio_gpu_ctrl_hdr)) < 0) + die("TRANSFER_TO_HOST_2D never answered"); + + struct virtio_gpu_resource_flush *f = + (struct virtio_gpu_resource_flush *)cmdbuf; + hdr_init(f, VIRTIO_GPU_CMD_RESOURCE_FLUSH); + f->r.x = f->r.y = 0; + f->r.width = (unsigned int)width; + f->r.height = (unsigned int)height; + f->resource_id = RESOURCE_ID; + f->padding = 0; + if (docmd(sizeof(*f), sizeof(struct virtio_gpu_ctrl_hdr)) < 0) + die("RESOURCE_FLUSH never answered"); +} + +// --- something to look at --------------------------------------------------- + +// Not a compositor. A pattern that is obviously deliberate, so a screenshot +// answers "did a userspace process put this here" without ambiguity -- a black +// screen and a working screen look identical if all you draw is black. +static void +draw_test_pattern(void) +{ + volatile unsigned int *fb = (volatile unsigned int *)fb_uaddr; + + for (int y = 0; y < height; y++) { + for (int x = 0; x < width; x++) { + unsigned int c; + int band = (y * 8) / height; + switch (band) { + case 0: c = 0x0C0E13; break; /* SCH_BG */ + case 1: c = 0x7C5CFF; break; /* SCH_ACCENT */ + case 2: c = 0x4B1D9A; break; /* PSOGM purple */ + case 3: c = (unsigned int)((x * 255 / width) * 0x010101); break; + case 4: c = (unsigned int)(x * 255 / width) << 16; break; + case 5: c = (unsigned int)(x * 255 / width) << 8; break; + case 6: c = (unsigned int)(x * 255 / width); break; + default: + // A checkerboard, so a stride bug is visible as a shear rather + // than as a plausible-looking gradient. + c = (((x / 16) + (y / 16)) & 1) ? 0xE6E8EC : 0x0C0E13; + break; + } + fb[y * width + x] = c; + } + } +} + +// --- main ------------------------------------------------------------------- + +int +main(int argc, char *argv[]) +{ + pcifd = open("/dev/pci", O_RDWR); + if (pcifd < 0) + die("cannot open /dev/pci"); + + int count = 0; + if (ioctl(pcifd, PCIIOC_COUNT, &count) < 0) + die("PCIIOC_COUNT failed"); + + // Find it by what it is, not by where it is. A driver that takes a bus + // address on the command line is one nobody can start automatically. + index_ = -1; + for (int i = 0; i < count; i++) { + info.index = (unsigned int)i; + if (ioctl(pcifd, PCIIOC_INFO, &info) < 0) + continue; + if (info.vendor == VIRTIO_VENDOR && info.device == VIRTIO_DEV_GPU) { + index_ = i; + break; + } + } + if (index_ < 0) + die("no virtio-gpu on this machine"); + + puts("vgpud: virtio-gpu at "); puthex(info.bus, 2); puts(":"); + puthex(info.slot, 2); puts("."); puthex(info.func, 1); + if (info.driver[0]) { puts(", currently held by "); puts(info.driver); } + puts("\n"); + + // The kernel driver is asked to let go here. It puts the VBE framebuffer + // back on screen before returning, so there is never a moment with no + // display -- see vgpu_release() and vga_fb_restore(). + if (ioctl(pcifd, PCIIOC_CLAIM, &index_) < 0) + die("claim refused -- the kernel driver would not release it"); + puts("vgpud: claimed\n"); + + // The info is re-read because claiming is what enables memory space and + // bus mastering, and a BAR read before that can legitimately be zero. + info.index = (unsigned int)index_; + if (ioctl(pcifd, PCIIOC_INFO, &info) < 0) + die("PCIIOC_INFO failed after claim"); + + common = find_cfg(VIRTIO_PCI_CAP_COMMON_CFG, 0); + notify = find_cfg(VIRTIO_PCI_CAP_NOTIFY_CFG, ¬ify_mult); + isr = find_cfg(VIRTIO_PCI_CAP_ISR_CFG, 0); + if (!common || !notify) + die("no modern virtio capabilities -- not a 1.0 device"); + + bring_up(); + draw_test_pattern(); + present(); + + puts("vgpud: drew a test pattern from userspace\n"); + + if (argc > 1 && argv[1][0] == '-' && argv[1][1] == 'x') { + // Exit without releasing, to show what a crashed driver leaves behind: + // the kernel takes the device back and the screen is whatever the + // device was last told to scan out. + puts("vgpud: exiting without cleaning up (simulated crash)\n"); + exit(); + } + + // Nothing to serve yet -- the compositor is what will connect here, over + // mechanism 1, which does not exist. Holding the device is the honest + // thing to do until then: releasing it would put the console back and make + // it look like this never worked. + for (;;) + sleep(100); +} diff --git a/docs/plan/hybrid.md b/docs/plan/hybrid.md index 5829d16..5504853 100644 --- a/docs/plan/hybrid.md +++ b/docs/plan/hybrid.md @@ -102,6 +102,61 @@ 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 first eviction happened + +`cmd/vgpud` is the virtio-gpu driver, outside the kernel. It finds the device +by vendor and device id, asks for it, and the kernel driver hands it over: + +``` +vgpud: virtio-gpu at 00:03.0, currently held by virtio-gpu +virtio-gpu: released to userspace; screen is the VBE framebuffer +vgpud: claimed +vgpud: display is 1280x800 +vgpud: scanout is ours, framebuffer at physical 0x3eceb000 +vgpud: drew a test pattern from userspace +``` + +Full virtio 1.0 bring-up from a process: reset, feature negotiation, virtqueue +in DMA memory, GET_DISPLAY_INFO, RESOURCE_CREATE_2D, ATTACH_BACKING, +SET_SCANOUT, and TRANSFER/FLUSH to present. The pattern it draws is on the +virtio-gpu head while the console carries on in VBE on the other one -- which +is the fallback rule working rather than being described. + +**No syscall knows this is a GPU.** The kernel handed over a PCI device; the +process decided what it was. That is the property that makes the interface +general, and it is why the evictions come before anything foreign. + +### What the first real driver asked for that a test program had not + +- **PCI configuration space** (`PCIIOC_CFG`). A virtio device says where its + registers are in *vendor capabilities*, which live in config space -- so a + driver that cannot read it cannot find its own registers. Anything modern is + discovered the same way. +- **A way for the incumbent to let go.** `struct pci_driver` gained a + `release` hook. A driver without one keeps its device, which is always safe + and, for anything on the path to a shell, correct. +- **The outgoing driver freeing its DMA.** Releasing the device while keeping + its 4 MB framebuffer is half a handover: the incoming driver asks for one and + is told there is no room. Found by doing it. +- **A bigger arena.** 4 MB was exactly one framebuffer at this resolution, and + for the moment either side of a handover there are two. 16 MB now. + +### And one bug it reproduced immediately + +The first working run raised this: + +``` +irq 11: 77780 interrupts/second -- a device on this line is not acknowledging it. +irq 11: handler 'e1000' is attached +``` + +`vgpud` polls the used ring, so it registered no handler -- and a virtio device +interrupts on every completion. A level-triggered line nobody acknowledges +re-fires for ever, drowning the e1000 that shares it. The fix is one line, +`VRING_AVAIL_F_NO_INTERRUPT` before the queue is enabled, and the value of it +is that the storm detector built for history/kernel.md section 13 named the +line, the rate and the neighbour without anyone having to look. + **The control channel (mechanism 1) is what remains.** ### One thing the interface is deliberately not diff --git a/include/defs.h b/include/defs.h index 4557676..e2cd78b 100644 --- a/include/defs.h +++ b/include/defs.h @@ -608,6 +608,7 @@ void platform_register_devices(uint64 fb_addr); // from registry.c; everything else draws through include/gfx.h. void vga_fb_init(uint64 phys_addr); uint64 vga_fb_phys(void); +void vga_fb_restore(void); uint64 vga_fb_size(void); void e820_init(void); diff --git a/include/dma.h b/include/dma.h index 1a40de9..f15c3e0 100644 --- a/include/dma.h +++ b/include/dma.h @@ -21,7 +21,18 @@ // will happily reorder stores to a ring, so a driver still needs a barrier // between filling a descriptor and telling the device about it. -#define DMA_ARENA_BYTES (4*1024*1024) +// 16 MB, not 4. +// +// 4 MB was one framebuffer at 1280x800, and that was fine while exactly one +// driver wanted one. It stopped being fine the day a display driver could be +// handed to userspace (docs/plan/hybrid.md): for the moment either side of the +// handover both drivers exist, and the incoming one has to allocate its +// framebuffer before the outgoing one is gone. +// +// It is carved off the top of physical memory at boot and never grows, so the +// cost of being wrong in this direction is 12 MB and the cost of being wrong +// in the other is a driver that cannot start. +#define DMA_ARENA_BYTES (16*1024*1024) void dma_init(void *vstart, void *vend); diff --git a/include/pci.h b/include/pci.h index b34f67a..77e725f 100644 --- a/include/pci.h +++ b/include/pci.h @@ -71,6 +71,9 @@ struct pci_dev { uint8 header_type; struct pci_bar bar[6]; const char *driver; // name of whatever claimed it, or 0 + // The driver itself, so it can be asked to release the device. The name + // alone cannot be called. + const struct pci_driver *drv; }; // One row of a driver's match table. Terminate the table with a row whose @@ -116,4 +119,17 @@ struct pci_driver { // Without this the second card of the same model is either refused or // silently shares the first one's registers. void (*attached)(struct pci_dev *, struct device *); + // Give the device up, so a userspace driver can take it. + // + // docs/plan/hybrid.md is built on the kernel *handing over* a device + // rather than a process wrestling it away: two drivers writing one set of + // registers is not a subtle failure, it is hardware that appears to have + // started lying. So the transfer is the incumbent's decision, and this is + // where it is made. + // + // A driver that implements this must leave the device in a state the next + // one can start from, and must stop touching it. A driver that does not + // implement it keeps its device: refusing is always safe, and for anything + // on the path to a shell it is also correct. + int (*release)(struct pci_dev *); }; diff --git a/include/pciuser.h b/include/pciuser.h index 4a18f42..77754c3 100644 --- a/include/pciuser.h +++ b/include/pciuser.h @@ -69,6 +69,16 @@ struct pciuser_info { // A DMA buffer. The kernel allocates it from the DMA arena, maps it into the // caller, and reports the physical address the *device* must be given -- which // is the entire reason a driver cannot do this for itself with mmap. +// A dword of PCI configuration space. A virtio device says where its registers +// live in vendor capabilities, which are here and nowhere else -- so without +// this a driver cannot find its own device. +struct pciuser_cfg { + unsigned int index; // in + unsigned int offset; // in: dword-aligned, 0..0xFC + unsigned int value; // out + unsigned int pad; +}; + struct pciuser_dma { unsigned long long size; // in: bytes, rounded up to a page unsigned long long uaddr; // out: where it landed in this process @@ -80,6 +90,7 @@ 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_CFG 0x9006 // struct pciuser_cfg -- read config space #define PCIIOC_IRQ 0x9005 // in: int index -- deliver this device's // interrupts to this process diff --git a/kernel/drivers/gpu/virtio_gpu/virtio.h b/include/virtio.h similarity index 83% rename from kernel/drivers/gpu/virtio_gpu/virtio.h rename to include/virtio.h index 5cbff3f..c2ed601 100644 --- a/kernel/drivers/gpu/virtio_gpu/virtio.h +++ b/include/virtio.h @@ -2,11 +2,14 @@ // virtio 1.0 over PCI — the transport, and virtio-gpu's 2D protocol. // -// Next to the driver rather than in include/, because exactly one driver reads -// it. When a second virtio device arrives -- virtio-net and virtio-blk are the -// obvious ones -- the *transport* half of this belongs in a shared place and -// the 2D protocol stays here; splitting it before there is a second caller -// would be guessing at where the seam goes. +// It lived next to the driver while exactly one driver read it, with a note +// saying it should move when a second reader appeared. One has: cmd/vgpud is +// the same device driven from userspace (docs/plan/hybrid.md's first +// eviction), and it needs the same register offsets and the same commands. +// +// Still one file rather than transport-and-protocol split in two. Both readers +// want both halves, so a seam here would be drawn for the sake of having one. +// virtio-net or virtio-blk arriving is when that changes. // // Written from the OASIS specification (virtio 1.1, chapters 2, 4.1 and 5.7), // which is a public document with no licence on what you write from it. Same @@ -68,6 +71,16 @@ // --- split virtqueue ------------------------------------------------------- #define VRING_DESC_F_NEXT 1 + +// Set in vring_avail.flags to tell the device not to interrupt on completion. +// +// A driver that polls the used ring wants this, and wants it before the queue +// is enabled. Without it the device raises its interrupt line on every +// completed command -- and if nothing is registered to acknowledge it, a +// level-triggered PCI line stays asserted and re-fires for ever. That is the +// failure in docs/history/kernel.md section 13, and cmd/vgpud reproduced it +// exactly on its first working run. +#define VRING_AVAIL_F_NO_INTERRUPT 1 #define VRING_DESC_F_WRITE 2 // device writes it; i.e. this is our buffer struct vring_desc { diff --git a/kernel/dev/pci.c b/kernel/dev/pci.c index fb517eb..5c10af2 100644 --- a/kernel/dev/pci.c +++ b/kernel/dev/pci.c @@ -374,6 +374,7 @@ pci_bind(const struct pci_driver *drv) continue; if (drv->probe(d) == 0) { d->driver = drv->name; + d->drv = drv; // Publish it in the bus-agnostic device list. Done here rather than in // each driver's probe() so that a driver cannot forget to, and so the // list is populated in bind order -- which is the order a device diff --git a/kernel/dev/pciuser.c b/kernel/dev/pciuser.c index c0a620c..abfb4de 100644 --- a/kernel/dev/pciuser.c +++ b/kernel/dev/pciuser.c @@ -253,8 +253,18 @@ do_claim(uint64 argp) // has started lying. The eviction path (docs/plan/hybrid.md) is for the // kernel to *hand over* a device deliberately; until a driver can be told to // let go, the honest answer to "may I have the one you are using" is no. - if (d->driver) - return -EBUSY; + if (d->driver) { + // Ask it to let go. A driver with no release hook keeps its device, which + // is always safe and, for anything on the path to a shell, correct: the + // rule in docs/plan/hybrid.md is that the kernel keeps a minimal driver + // for everything needed to reach a prompt and userspace owns the good one. + if (!d->drv || !d->drv->release) + return -EBUSY; + if (d->drv->release((struct pci_dev *)d) != 0) + return -EBUSY; + ((struct pci_dev *)d)->driver = 0; + ((struct pci_dev *)d)->drv = 0; + } acquire(&pciuser_lock); if (owner[idx] && owner[idx] != myproc()) { @@ -391,6 +401,39 @@ do_irq(uint64 argp) return 0; } +// PCI configuration space. +// +// The first real driver asked for this and the test program never had to: a +// virtio device describes where its registers are in *vendor capabilities*, +// which live in config space, so a driver that cannot read config space cannot +// find its own registers. Anything modern is discovered the same way -- MSI-X +// tables, PCIe extended capabilities, power management. +// +// Reads are unrestricted for a claimed device; writes are not offered at all +// yet. Nothing needs them, and a write to the wrong offset re-programs a BAR +// or disables bus mastering under the kernel's feet. +static int +do_cfg(uint64 argp) +{ + struct pciuser_cfg *c = (struct pciuser_cfg *)user_ptr(argp, sizeof(*c)); + if (!c) + return -EFAULT; + + unsigned int idx = c->index; + if (idx >= (unsigned int)pci_count() || idx >= NPCI) + return -ENODEV; + if (owner[idx] != myproc()) + return -EACCES; + if (c->offset > 0xFC) + return -EINVAL; + + const struct pci_dev *d = pci_get((int)idx); + if (!d) + return -ENODEV; + c->value = pci_read32(d->bus, d->slot, d->func, (uint8)(c->offset & 0xFC)); + return 0; +} + int pciuser_ioctl(uint64 request, uint64 argp) { @@ -405,6 +448,7 @@ pciuser_ioctl(uint64 request, uint64 argp) case PCIIOC_RELEASE: return do_release(argp); case PCIIOC_DMA: return do_dma(argp); case PCIIOC_IRQ: return do_irq(argp); + case PCIIOC_CFG: return do_cfg(argp); default: return -ENOTTY; } } diff --git a/kernel/drivers/gpu/vga/vga_fb.c b/kernel/drivers/gpu/vga/vga_fb.c index 1c61703..b1f9b8e 100644 --- a/kernel/drivers/gpu/vga/vga_fb.c +++ b/kernel/drivers/gpu/vga/vga_fb.c @@ -214,3 +214,31 @@ const struct display_ops vga_display_ops = { .present = vga_disp_present, .wait_vblank = vga_disp_wait_vblank, }; + +// Put the VBE framebuffer back on screen. +// +// Called when a better display driver lets go of the hardware -- see +// virtio-gpu's release hook. gfx holds one scanout, last writer wins, so the +// driver that leaves has to name its replacement rather than simply detaching; +// detaching alone leaves gfx pointing at a device nobody is driving. +// +// This is the fallback rule from docs/plan/hybrid.md made real: the kernel +// keeps a minimal driver for everything on the path to a shell, so a fancy +// driver leaving degrades to a working screen instead of to no screen. +void +vga_fb_restore(void) +{ + struct fb_info info; + + if (!fb || !fb_width || !fb_height) + return; // nothing was ever set up; nothing to go back to + + info.ptr = fb; + info.phys = fb_phys; + info.size = vga_fb_size(); + info.stride = fb_stride; + info.bpp = fb_bpp; + info.width = fb_width; + info.height = fb_height; + gfx_attach_scanout(&info); +} diff --git a/kernel/drivers/gpu/virtio_gpu/virtio_gpu.c b/kernel/drivers/gpu/virtio_gpu/virtio_gpu.c index 384e0df..677107d 100644 --- a/kernel/drivers/gpu/virtio_gpu/virtio_gpu.c +++ b/kernel/drivers/gpu/virtio_gpu/virtio_gpu.c @@ -58,6 +58,7 @@ struct vgpu { // command queue with one entry is not the bottleneck in a 2D compositor. void *cmd; void *resp; + void *ring; // the three vrings, one block, for dma_free int width, height; void *fb; // the framebuffer we scan out from @@ -179,6 +180,7 @@ queue_setup(struct vgpu *g) uint32 used_sz = 6 + 8 * qsize; uint8 *mem = dma_alloc(4096 * 3, 4096); + g->ring = mem; if (!mem) return -1; memset(mem, 0, 4096 * 3); @@ -510,6 +512,62 @@ vgpu_probe(struct pci_dev *d) return 0; } +// Hand the device to a userspace driver. +// +// docs/plan/hybrid.md's first eviction, and the reason it is this device: the +// boundary already exists (struct display_ops), it has exactly one consumer, +// and there is a working fallback underneath. So letting go is safe in a way +// it would not be for the disk the root filesystem is on. +// +// What "letting go" has to mean, in order: +// +// 1. stop drawing -- put the VBE framebuffer back on screen *first*, so +// there is never a moment where gfx points at hardware nobody drives +// 2. reset the device, so the next driver starts from a known state rather +// than inheriting a configured queue it did not set up +// 3. forget the instance, so a later probe or a second claim cannot find +// half-initialised state +static int +vgpu_release(struct pci_dev *d) +{ + struct vgpu *g = 0; + + for (int i = 0; i < VGPU_MAX; i++) + if (gpus[i].ready && gpus[i].pdev == d) { g = &gpus[i]; break; } + if (!g) + return -1; + + vga_fb_restore(); + + // Status 0 is the virtio reset. The spec requires a driver to wait for the + // device to acknowledge by reading the register back as zero. + if (g->common) { + w8(g->common, VCOMMON_STATUS, 0); + for (int spin = 0; spin < 1000000 && r8(g->common, VCOMMON_STATUS); spin++) + ; + } + + if (g->isr) + irq_unregister(d->irq, vgpu_intr, g); + + // Give the arena its memory back. + // + // Releasing the *device* without releasing its memory is half a handover: + // the DMA arena is 16 MB and contiguous, a framebuffer at this resolution is + // 4 MB of it, and the process taking over needs one of its own. Keeping ours + // means the new driver asks for a framebuffer and is told there is no room, + // which is what happened the first time this was tried. + if (g->fb) { dma_free(g->fb, g->fb_bytes); g->fb = 0; } + if (g->ring) { dma_free(g->ring, 4096 * 3); g->ring = 0; } + if (g->cmd) { dma_free(g->cmd, 4096); g->cmd = 0; } + if (g->resp) { dma_free(g->resp, 4096); g->resp = 0; } + + g->ready = 0; + g->pdev = 0; + cprintf("virtio-gpu: released to userspace; screen is the VBE framebuffer\n"); + return 0; +} + static const struct pci_match virtio_gpu_match[] = { { VIRTIO_VENDOR, VIRTIO_DEV_GPU, 0, 0 }, { 0, 0, 0, 0 }, @@ -534,4 +592,5 @@ const struct pci_driver virtio_gpu_driver = { .ops = &virtio_gpu_display_ops, .probe = vgpu_probe, .attached = vgpu_attached, + .release = vgpu_release, };