> Date: Fri, 12 Aug 2022 17:31:31 +1000 > From: Simon Burge <sim...@netbsd.org> > > As part of my boot-from-ZFS work, I've generalised how "non-disk" > disk-type storage devices interface with the mountroot mechanism and > also tidied up how devices can report that they can contain a virtual > storage device (eg, a RAID, LVM, ZFS, etc).
Nice! Can you be sure to commit separate changes? 1. D_STORAGEPOOL, 2. device_has_partitions, 3. mountroot rootspec hooks? > - A new bdev/cdev device flag D_STORAGEPOOL, which indicates that the > device can contain other storage device. We should have these bdevsw/cdevsw::d_flag D_* flags documented somewhere. Can you go into more detail on what criteria determine whether to use D_STORAGEPOOL or not? > - A new device_t flag DVF_NO_PARTITIONS, for storage devices which > don't contain partititons (eg, wedges, flash). You've added a new function device_set_flag to change the flag dynamically. Why dynamically? Why not make it part of the cfattach declaration? If it has to be dynamic, then we have to think about serializing access to dv_flags (which is relevant even before mountroot, because some disks are discovered in threads). But for the cases that were covered before it could be entirely static, I think. For that matter: Why is it in the device_t and not the cdevsw/bdevsw? (I realize that moving from one to the other might require a more invasive change, so don't worry about that -- just wondering about the design.) > - New mountroot_rootspec hooks, which allows a device driver to > advertise that it can contain a virtual storage device. The new > interface is: > > mountroot_rootspec_hook_establish() > Called by a driver to register a callback to check if the passed > boot device string is matched by a driver and a callback to > print a list of any boot devices provided by the driver. > > The match function also passes a string prefix (eg "wedge") to > register the hook. Sounds reasonable. +void * +mountroot_rootspec_hook_establish(const char *prefix, + device_t (*hookfn)(const char *), void (*printfn)(void)) This should return a non-void pointer, e.g. struct mountroot_rootspec_hook *. It doesn't have to be exposed in a header file -- a forward declaration `struct mountroot_rootspec_hook;' in the header file is enough. (We should do the same for other things like softint_establish and pci_intr_establish. No reason to abuse void * when we can have distinct named types.) + hd = malloc(sizeof(*hd), M_DEVBUF, M_NOWAIT); Use kmem, not malloc. + LIST_INSERT_HEAD(&rootspec_hook_list, hd, hk_list); Seems to me this should check for duplicates and panic if so. +device_bdev_flags(device_t dv) Maybe this should just be device_is_storagepool or something? I'm not sure exposing the mapping from device_t to the bdevsw flags is generally useful.
