Re: patch review: MMIO cmdline

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

> diff --git a/sys/arch/amd64/conf/MICROVM b/sys/arch/amd64/conf/MICROVM
> new file mode 100644
> index 00000000000..5995220bd20
> --- /dev/null
> +++ b/sys/arch/amd64/conf/MICROVM

Did you mean to add the MICROVM config in this change?

The virtio_mmio logic appears to be x86-specific:

> +#include <arch/x86/pv/pvvar.h>
> +#include <xen/hypervisor.h>
> +
> +#include <machine/i82093var.h>
> ...
> +	ioapic = ioapic_find_bybase(irq);
> +
> +	if (ioapic != NULL) {
> +		KASSERT(ioapic->sc_pic.pic_type == PIC_IOAPIC);
> +		pic = &ioapic->sc_pic;
> +		pin = irq - pic->pic_vecbase;
> +		irq = -1;
> +	} else
> +		pic = &i8259_pic;
> +
> +	mpsafe = (0 != (vsc->sc_flags & VIRTIO_F_INTR_MPSAFE));
> +
> +	msc->sc_ih = intr_establish_xname(irq, pic, pin, IST_LEVEL, vsc->sc_ipl,
> +		virtio_mmio_intr, msc, mpsafe, device_xname(vsc->sc_dev));

So I think it should live under sys/arch/x86, or under
sys/dev/virtio/arch/x86, not under sys/dev/virtio directly.

> +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.

Use NULL, not (void *)voidop.

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?

> +	error = bus_space_map(
> +			msc->sc_iot, margs->baseaddr,
> +			margs->sz, 0, &msc->sc_ioh
> +		);
> +	if (error) {
> +		aprint_error_dev(self,
> +			"couldn't map %#" PRIx64 ": %d",
> +			(uint64_t)margs->baseaddr, error
> +		);
> +		return error;
> +	}

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:

	error = bus_space_map(msc->sc_iot, margs->baseaddr, margs->sz, 0,
	    &msc->sc_ioh);
	if (error) {
		aprint_error_dev(self, "couldn't map %#"PRIx64": %d",
		    margs->baseaddr, error);
		return error;
	}