Re: [Qemu-devel] [PATCH v1 1/1] vl.c: Allow sysbus devices to be attached via commandline

[Peter Crosthwaite]

On Wed, Mar 26, 2014 at 10:47 AM, Alistair Francis
<address@hidden> wrote:
> This patch introduces a new command line argument that allows
> sysbus devices to be attached via the command line.
>
> This allows devices to be added after the machine init but
> before anything is booted
>
> The new argument is -sysbusdev
>
> A new argument is being used to avoid confusion and user
> errors that would appear if the same command for hot-plugging
> was used instead. This could be changed to use -device and
> specify sysbus by using something like 'bus=sysbus' if that
> is preferred
>

Can you be more specific about the confusion issue you are seeing? If
you went for -device bus=root (meaning the default singleton root
sysbus), where are the possible issues?

> This patch requires the machine to support this, by passing
> the interrupt controller back to main() OR the device can
> be attached without an IRQ line
>

Your approach assumes a sys-wide singleton for interrupt controllers,
but systems can have multiple interrupt controllers and different
-sysbusdev args may want to connect to different intcs (E.g. Zynq can
have GiC connections as well as xilinx intcs implemented in FPGA
fabric). Going further down the rabbit hole, can I create an intc
itself with -sysbusdev then connect another -sysbusdev to it?

> An example usage is adding UART to zynq using these two options:
> -sysbusdev device=cadence_uart,addr=E0000000,irq=27
> -sysbusdev device=cadence_uart,addr=E0001000,irq=50
>

Is it possible to generalise the irq support to include gpios as well?
Sometimes devices have random gpios connecting between them (flow
control handshakes for example).

Regards,
Peter


> Signed-off-by: Alistair Francis <address@hidden>
> ---
>
>  hw/arm/xilinx_zynq.c    |    1 +
>  include/hw/boards.h     |    1 +
>  include/monitor/qdev.h  |    1 +
>  include/sysemu/sysemu.h |    1 +
>  qdev-monitor.c          |   65 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx         |   13 +++++++++
>  vl.c                    |   27 ++++++++++++++++++-
>  7 files changed, 108 insertions(+), 1 deletions(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 9ee21e7..aae0863 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -172,6 +172,7 @@ static void zynq_init(QEMUMachineInitArgs *args)
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
>
>      dev = qdev_create(NULL, "a9mpcore_priv");
> +    args->intc = dev;
>      qdev_prop_set_uint32(dev, "num-cpu", 1);
>      qdev_init_nofail(dev);
>      busdev = SYS_BUS_DEVICE(dev);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index dd2c70d..112ea61 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -15,6 +15,7 @@ typedef struct QEMUMachineInitArgs {
>      const char *kernel_cmdline;
>      const char *initrd_filename;
>      const char *cpu_model;
> +    DeviceState *intc;
>  } QEMUMachineInitArgs;
>
>  typedef void QEMUMachineInitFunc(QEMUMachineInitArgs *args);
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 8d16e11..3d3afdc 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -11,5 +11,6 @@ void do_info_qdm(Monitor *mon, const QDict *qdict);
>  int do_device_add(Monitor *mon, const QDict *qdict, QObject **ret_data);
>  int qdev_device_help(QemuOpts *opts);
>  DeviceState *qdev_device_add(QemuOpts *opts);
> +DeviceState *qdev_sysbusdev_add(QemuOpts *opts, DeviceState* intc);
>
>  #endif
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 3915ce3..1f1a7b6 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -199,6 +199,7 @@ extern QemuOptsList qemu_common_drive_opts;
>  extern QemuOptsList qemu_drive_opts;
>  extern QemuOptsList qemu_chardev_opts;
>  extern QemuOptsList qemu_device_opts;
> +extern QemuOptsList qemu_sysbusdev_opts;
>  extern QemuOptsList qemu_netdev_opts;
>  extern QemuOptsList qemu_net_opts;
>  extern QemuOptsList qemu_global_opts;
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 9268c87..ba68f88 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -566,6 +566,57 @@ DeviceState *qdev_device_add(QemuOpts *opts)
>      return dev;
>  }
>
> +DeviceState *qdev_sysbusdev_add(QemuOpts *opts, DeviceState* intc)
> +{
> +
> +    ObjectClass *oc;
> +    const char *device;
> +    hwaddr addr;
> +    int irq;
> +
> +    device = qemu_opt_get(opts, "device");
> +    if (!device) {
> +        qerror_report(QERR_MISSING_PARAMETER, "device");
> +        return NULL;
> +    }
> +
> +    /* find the device */
> +    oc = object_class_by_name(device);
> +    if (!oc) {
> +        const char *typename = find_typename_by_alias(device);
> +
> +        if (typename) {
> +            device = typename;
> +            oc = object_class_by_name(device);
> +        }
> +    }
> +
> +    if (!object_class_dynamic_cast(oc, TYPE_DEVICE)) {
> +        qerror_report(ERROR_CLASS_GENERIC_ERROR,
> +                      "'%s' is not a valid device model name", device);
> +        return NULL;
> +    }
> +
> +    if (object_class_is_abstract(oc)) {
> +        qerror_report(QERR_INVALID_PARAMETER_VALUE, "device",
> +                      "non-abstract device type");
> +        return NULL;
> +    }
> +
> +    if (qemu_opt_get(opts, "addr") && qemu_opt_get(opts, "irq") && intc) {
> +        sscanf(qemu_opt_get(opts, "addr"), "%X", (uint *) &addr);
> +        sscanf(qemu_opt_get(opts, "irq"), "%d", &irq);
> +        return sysbus_create_varargs(device, addr,
> +                                     qdev_get_gpio_in(intc, irq), NULL);
> +    }
> +
> +    if (qemu_opt_get(opts, "addr")) {
> +        sscanf(qemu_opt_get(opts, "addr"), "%X", (uint *) &addr);
> +        return sysbus_create_varargs(device, addr, NULL);
> +    }
> +
> +    return NULL;
> +}
>
>  #define qdev_printf(fmt, ...) monitor_printf(mon, "%*s" fmt, indent, "", ## 
> __VA_ARGS__)
>  static void qbus_print(Monitor *mon, BusState *bus, int indent);
> @@ -712,6 +763,20 @@ QemuOptsList qemu_device_opts = {
>      },
>  };
>
> +QemuOptsList qemu_sysbusdev_opts = {
> +    .name = "sysbusdev",
> +    .implied_opt_name = "device",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_sysbusdev_opts.head),
> +    .desc = {
> +        /*
> +         * no elements => accept any
> +         * sanity checking will happen later
> +         * when setting sysbusdev properties
> +         */
> +        { /* end of list */ }
> +    },
> +};
> +
>  QemuOptsList qemu_global_opts = {
>      .name = "global",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_global_opts.head),
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ee5437b..340fb5d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -327,6 +327,19 @@ possible drivers and properties, use @code{-device help} 
> and
>  @code{-device @var{driver},help}.
>  ETEXI
>
> +DEF("sysbusdev", HAS_ARG, QEMU_OPTION_sysbusdev,
> +    "-sysbusdev device=sysbusdevice[,prop[=value][,...]]\n"
> +    "                add a sysbus device\n"
> +    "                prop=value,... sets the sysbus properties\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> address@hidden -sysbusdev @var{device}[,@address@hidden,...]]
> address@hidden -sysbusdev
> +Add sysbusdev @var{device}.  @address@hidden sets sysbus
> +properties.  Valid properties are device, addr and irq.  Which specifies
> +the sysbus device name, base address and irq number respectivly.
> +ETEXI
> +
>  DEF("name", HAS_ARG, QEMU_OPTION_name,
>      "-name string1[,process=string2][,debug-threads=on|off]\n"
>      "                set the name of the guest\n"
> diff --git a/vl.c b/vl.c
> index 02bf8ec..c689d2b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2392,6 +2392,18 @@ static int device_init_func(QemuOpts *opts, void 
> *opaque)
>      return 0;
>  }
>
> +static int sysbusdev_init_func(QemuOpts *opts, void *opaque)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_sysbusdev_add(opts, (DeviceState *) opaque);
> +    if (!dev) {
> +        return -1;
> +    }
> +    object_unref(OBJECT(dev));
> +    return 0;
> +}
> +
>  static int chardev_init_func(QemuOpts *opts, void *opaque)
>  {
>      Error *local_err = NULL;
> @@ -2987,6 +2999,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_add_drive_opts(&qemu_drive_opts);
>      qemu_add_opts(&qemu_chardev_opts);
>      qemu_add_opts(&qemu_device_opts);
> +    qemu_add_opts(&qemu_sysbusdev_opts);
>      qemu_add_opts(&qemu_netdev_opts);
>      qemu_add_opts(&qemu_net_opts);
>      qemu_add_opts(&qemu_rtc_opts);
> @@ -3680,6 +3693,11 @@ int main(int argc, char **argv, char **envp)
>                      exit(1);
>                  }
>                  break;
> +            case QEMU_OPTION_sysbusdev:
> +                if (!qemu_opts_parse(qemu_find_opts("sysbusdev"), optarg, 
> 0)) {
> +                    exit(1);
> +                }
> +                break;
>              case QEMU_OPTION_smp:
>                  if (!qemu_opts_parse(qemu_find_opts("smp-opts"), optarg, 1)) 
> {
>                      exit(1);
> @@ -4384,11 +4402,18 @@ int main(int argc, char **argv, char **envp)
>                                   .kernel_filename = kernel_filename,
>                                   .kernel_cmdline = kernel_cmdline,
>                                   .initrd_filename = initrd_filename,
> -                                 .cpu_model = cpu_model };
> +                                 .cpu_model = cpu_model,
> +                                 .intc = NULL };
>
>      current_machine->init_args = args;
>      machine->init(&current_machine->init_args);
>
> +    /* Init sysbus devices */
> +    if (qemu_opts_foreach(qemu_find_opts("sysbusdev"), sysbusdev_init_func,
> +                          current_machine->init_args.intc, 1) != 0) {
> +        exit(1);
> +    }
> +
>      audio_init();
>
>      cpu_synchronize_all_post_init();
> --
> 1.7.1
>
>