Re: git: 0e1f5ab7db2c - main - virtio_mmio: Support command-line parameters
Date: Tue, 18 Oct 2022 15:12:59 UTC
On 18 Oct 2022, at 07:03, Colin Percival <cperciva@FreeBSD.org> wrote:
>
> The branch main has been updated by cperciva:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=0e1f5ab7db2cd2837f97f169122897b19c185dbd
>
> commit 0e1f5ab7db2cd2837f97f169122897b19c185dbd
> Author: Colin Percival <cperciva@FreeBSD.org>
> AuthorDate: 2022-08-13 00:54:26 +0000
> Commit: Colin Percival <cperciva@FreeBSD.org>
> CommitDate: 2022-10-18 06:02:21 +0000
>
> virtio_mmio: Support command-line parameters
>
> The Virtio MMIO bus driver was added in 2014 with support for devices
> exposed via FDT; in 2018 support was added to discover Virtio MMIO
> devices via ACPI tables, as in QEMU. The Firecracker VMM eschews both
> FDT and ACPI, instead presenting device information via kernel command
> line arguments of the form virtio_mmio.device=<parameters>.
>
> These command line parameters get converted into kernel environment
> variables; this adds support for parsing those variables and attaching
> virtio_mmio children to nexus.
>
> There is a case to be made that it would be cleaner to have a new
> "cmdlinebus" attached to nexus and virtio_mmio children attached to
> that. A future commit might do that.
>
> Discussed with: imp, jrtc27
> Sponsored by: https://patreon.com/cperciva
> Differential Revision: https://reviews.freebsd.org/D36189
> ---
> sys/conf/files | 1 +
> sys/dev/virtio/mmio/virtio_mmio_cmdline.c | 150 ++++++++++++++++++++++++++++++
> 2 files changed, 151 insertions(+)
>
> diff --git a/sys/conf/files b/sys/conf/files
> index b7da3626111d..39e6d4aaf0a8 100644
> --- a/sys/conf/files
> +++ b/sys/conf/files
> @@ -3437,6 +3437,7 @@ dev/virtio/pci/virtio_pci_legacy.c optional virtio_pci
> dev/virtio/pci/virtio_pci_modern.c optional virtio_pci
> dev/virtio/mmio/virtio_mmio.c optional virtio_mmio
> dev/virtio/mmio/virtio_mmio_acpi.c optional virtio_mmio acpi
> +dev/virtio/mmio/virtio_mmio_cmdline.c optional virtio_mmio
As I said in the review, this definitely doesn’t work for INTRNG due to
IRQ number virtualisation, among other things, yet this enables it for
all architectures, and I believe it should also get its own config
option as it’s rather dangerous to use (Linux separates it out from
virtio_mmio and has it off-by-default).
Jess
> dev/virtio/mmio/virtio_mmio_fdt.c optional virtio_mmio fdt
> dev/virtio/mmio/virtio_mmio_if.m optional virtio_mmio
> dev/virtio/network/if_vtnet.c optional vtnet
> diff --git a/sys/dev/virtio/mmio/virtio_mmio_cmdline.c b/sys/dev/virtio/mmio/virtio_mmio_cmdline.c
> new file mode 100644
> index 000000000000..cb4762adc964
> --- /dev/null
> +++ b/sys/dev/virtio/mmio/virtio_mmio_cmdline.c
> @@ -0,0 +1,150 @@
> +/*-
> + * Copyright (c) 2022 Colin Percival
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
> + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include <sys/param.h>
> +#include <sys/systm.h>
> +#include <sys/bus.h>
> +#include <sys/kernel.h>
> +#include <sys/limits.h>
> +#include <sys/malloc.h>
> +#include <sys/module.h>
> +#include <sys/rman.h>
> +
> +#include <machine/bus.h>
> +#include <machine/resource.h>
> +
> +#include <dev/virtio/mmio/virtio_mmio.h>
> +
> +/* Parse <size>@<baseaddr>:<irq>[:<id>] and add a child. */
> +static void
> +parsearg(driver_t *driver, device_t parent, char * arg)
> +{
> + device_t child;
> + char * p;
> + unsigned long sz;
> + unsigned long baseaddr;
> + unsigned long irq;
> + unsigned long id;
> +
> + /* <size> */
> + sz = strtoul(arg, &p, 0);
> + if ((sz == 0) || (sz == ULONG_MAX))
> + goto bad;
> + switch (*p) {
> + case 'E': case 'e':
> + sz <<= 10;
> + /* FALLTHROUGH */
> + case 'P': case 'p':
> + sz <<= 10;
> + /* FALLTHROUGH */
> + case 'T': case 't':
> + sz <<= 10;
> + /* FALLTHROUGH */
> + case 'G': case 'g':
> + sz <<= 10;
> + /* FALLTHROUGH */
> + case 'M': case 'm':
> + sz <<= 10;
> + /* FALLTHROUGH */
> + case 'K': case 'k':
> + sz <<= 10;
> + p++;
> + break;
> + }
> +
> + /* @<baseaddr> */
> + if (*p++ != '@')
> + goto bad;
> + baseaddr = strtoul(p, &p, 0);
> + if ((baseaddr == 0) || (baseaddr == ULONG_MAX))
> + goto bad;
> +
> + /* :<irq> */
> + if (*p++ != ':')
> + goto bad;
> + irq = strtoul(p, &p, 0);
> + if ((irq == 0) || (irq == ULONG_MAX))
> + goto bad;
> +
> + /* Optionally, :<id> */
> + if (*p) {
> + if (*p++ != ':')
> + goto bad;
> + id = strtoul(p, &p, 0);
> + if ((id == 0) || (id == ULONG_MAX))
> + goto bad;
> + } else {
> + id = 0;
> + }
> +
> + /* Should have reached the end of the string. */
> + if (*p)
> + goto bad;
> +
> + /* Create the child and assign its resources. */
> + child = BUS_ADD_CHILD(parent, 0, driver->name, id ? id : -1);
> + bus_set_resource(child, SYS_RES_MEMORY, 0, baseaddr, sz);
> + bus_set_resource(child, SYS_RES_IRQ, 0, irq, 1);
> + device_set_driver(child, driver);
> +
> + return;
> +
> +bad:
> + printf("Error parsing virtio_mmio parameter: %s\n", arg);
> +}
> +
> +static void
> +vtmmio_cmdline_identify(driver_t *driver, device_t parent)
> +{
> + size_t n;
> + char name[] = "virtio_mmio.device_XXXX";
> + char * val;
> +
> + /* First variable just has its own name. */
> + if ((val = kern_getenv("virtio_mmio.device")) == NULL)
> + return;
> + parsearg(driver, parent, val);
> + freeenv(val);
> +
> + /* The rest have _%zu suffixes. */
> + for (n = 1; n <= 9999; n++) {
> + sprintf(name, "virtio_mmio.device_%zu", n);
> + if ((val = kern_getenv(name)) == NULL)
> + return;
> + parsearg(driver, parent, val);
> + freeenv(val);
> + }
> +}
> +
> +static device_method_t vtmmio_cmdline_methods[] = {
> + /* Device interface. */
> + DEVMETHOD(device_identify, vtmmio_cmdline_identify),
> + DEVMETHOD(device_probe, vtmmio_probe),
> +
> + DEVMETHOD_END
> +};
> +DEFINE_CLASS_1(virtio_mmio, vtmmio_cmdline_driver, vtmmio_cmdline_methods,
> + sizeof(struct vtmmio_softc), vtmmio_driver);
> +DRIVER_MODULE(vtmmio_cmdline, nexus, vtmmio_cmdline_driver, 0, 0);