> Date: Mon, 13 Jan 2025 21:32:16 +0100 (CET) > From: Emile `iMil' Heitor <i...@home.imil.net> > > On Mon, 13 Jan 2025, Taylor R Campbell wrote: > > > Is DVF_DETACH_SHUTDOWN necessary here? Does this driver have to run > > its detach routine in order to safely, e.g., force buffered writes to > > persistent storage? > > virtio can be the root of many drivers, including ld(4), so yes I > thought it was reasonable, also I looked at what virtio_mmio_fdt.c > and virtio_acpi.c did and they both use DVF_DETACH_SHUTDOWN
Individual drivers can opt into this without requiring the buses they attach at to be opted into this, so DVF_DETACH_SHUTDOWN should work for ld(4) even if the virtio(4) bus it attaches to doesn't use it. Setting DVF_ATTACH_SHUTDOWN in the bus means detaching _all_ the children at shutdown even if they _don't_ need to force buffered writes to persistent storage. It's fine to leave it in for now, but I think it is a mistake to use it for any of the virtio(4) attachments (note virtio@pci does not); maybe jakllsch@ or jmcneill@ can chime in about why they did this. > Resulting patch (only fixed parts): https://imil.net/NetBSD/mmio_cmdline.patch Thanks! I think this is good to go, just some minor nits below. + margs->sz = strtoull(arg, (char **)&p, 0); + if ((margs->sz == 0) || (margs->sz == ULLONG_MAX)) + goto bad; Not a problem in your code, but we should really have a man page that describes the error detection protocol of strtoull in the kernel! The protocol of strtoull in userland is hard enough (see the strtoull(3) man page!) but a cursory skim of _strtoul.h suggests that it's different in subtle ways from userland, so while this is probably right, I'm not 100% sure about reviewing it. + case 'E': case 'e': + margs->sz <<= 10; Not really urgent since anyone who can control the bootloader command line can probably control anything in the kernel, but it might be nice for usability if this did overflow checks. case 'E': case 'e': if (margs->sz > UINT64_MAX >> 60) goto bad; margs->sz <<= 10; ... +virtio_mmio_cmdline_attach(device_t parent, device_t self, void *aux) +{ ... + static char cmdline[LINE_MAX], *parg = NULL; ... + if (parg == NULL) { /* first pass */ I guess this will work but it's kind of gross! Seems like this should be something the parent device iterates over instead. + aprint_verbose("kernel parameters: %s\n", cmdline); Maybe keep this on the first line so it's more obviously attributable to virtio(4), or use aprint_verbose_dev? (Not especially important because these lines come one after the other but I generally like to err on the side of making attribution of kernel messages obvious; we've had a lot of random unattributable messages that were a pain to chase down in the past.) + aprint_normal("viommio: %s\n", parg); Maybe use aprint_normal_dev here instead? +virtio_mmio_cmdline_do_attach(device_t self, + struct pv_attach_args *pvaa, + struct mmio_args *margs) four-space indent for continuation lines + struct virtio_mmio_cmdline_softc *const sc = + (struct virtio_mmio_cmdline_softc *)msc; four-space indent for continuation lines
