Re: patch review: MMIO cmdline

[Emile `iMil' Heitor <imil%home.imil.net@localhost>]

On Mon, 13 Jan 2025, Taylor R Campbell wrote:

> > +++ b/sys/arch/amd64/conf/MICROVM
>
> Did you mean to add the MICROVM config in this change?

I did, this is one of the goals for this patch set, to build a minimal,
fast, QEMU and Firecracker compatible kernel.

> The virtio_mmio logic appears to be x86-specific:
[...]
> So I think it should live under sys/arch/x86, or under
> sys/dev/virtio/arch/x86, not under sys/dev/virtio directly.

You're right, I moved it to sys/dev/virtio/arch/x86

> > +CFATTACH_DECL3_NEW(mmio_cmdline,
> > +	sizeof(struct virtio_mmio_cmdline_softc),
> > +	virtio_mmio_cmdline_match, virtio_mmio_cmdline_attach,
> > +	virtio_mmio_cmdline_detach, NULL,
> > +	virtio_mmio_cmdline_rescan, (void *)voidop, DVF_DETACH_SHUTDOWN);
>
> Nit: KNF continuation line is four spaces, not one tab.  Same in
> various other places.

fixed

> Use NULL, not (void *)voidop.

fixed

> 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

[...]

> Same here about continuation lines, plus no line break before closing
> parentheses, and no need to cast to uint64_t because margs->baseaddr
> is already uint64_t:

fixed

Resulting patch (only fixed parts): https://imil.net/NetBSD/mmio_cmdline.patch

------------------------------------------------------------------------
Emile `iMil' Heitor <imil@{home.imil.net,NetBSD.org}> | https://imil.net