Re: mount root hooks for non-disk-device devices

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Fri, 12 Aug 2022 17:31:31 +1000
> From: Simon Burge <simonb%NetBSD.org@localhost>
> 
> 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.