Re: patch review: MMIO cmdline

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Mon, 13 Jan 2025 21:32:16 +0100 (CET)
> From: Emile `iMil' Heitor <imil%home.imil.net@localhost>
> 
> 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