| Message ID | d03b83731d924682afa040af62f0b2310cc085fa.1395794261.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 26, 2014 at 10:47 AM, Alistair Francis <alistair.francis@xilinx.com> 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 <alistair.francis@xilinx.com> > --- > > 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 > +@item -sysbusdev @var{device}[,@var{prop}[=@var{value}][,...]] > +@findex -sysbusdev > +Add sysbusdev @var{device}. @var{prop}=@var{value} 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(¤t_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 > >
On Wed, Apr 9, 2014 at 11:28 AM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Wed, Mar 26, 2014 at 10:47 AM, Alistair Francis > <alistair.francis@xilinx.com> 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? > Using -device would be possible (and easy to implement). The reason I started with a different argument name was just to avoid confusion and to make sure I didn't interfere with hot-pluggable devices. I still feel that separating hot-pluggable devices from bus devices is a good idea. They are initialised in slightly different parts of the code (although this wouldn't be hard to fix). The name 'sysbusdev' is confusing though, as it doesn't have to connect to the sysbus. >> 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? It does assume a single interrupt controller. There isn't really anything stopping you having multiple interrupt controllers as they are passed into the qdev_sysbusdev_add() function. The biggest problem with having multiple interrupt controllers is storing them all and knowing which one the device should be connected to. It is possible to create an interrupt controller with -sysbusdev and then connect other -sysbusdev devices to it. This patch can't do it, but I have a newer version that will allow that. I was going to wait until after the release of 2.0 before I submit the next version. The only disadvantage is that it requires the correct order for arguments (the interrupt controller must be specified before anything connecting to it). This also makes it possible to use multiple interrupt controllers in the following fashion: Specify intc 1 (Either in machine init or via -sysbusdev): - Attach devices Specify intc 2 (replaces pointer for the first one): - Attach other devices using the second intc > >> 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). Not at the moment, connections between devices is very difficult with this method. Although I am looking into it > > Regards, > Peter > > >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> >> 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 >> +@item -sysbusdev @var{device}[,@var{prop}[=@var{value}][,...]] >> +@findex -sysbusdev >> +Add sysbusdev @var{device}. @var{prop}=@var{value} 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(¤t_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 >> >> >
Alistair Francis <alistair.francis@xilinx.com> writes: > On Wed, Apr 9, 2014 at 11:28 AM, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Wed, Mar 26, 2014 at 10:47 AM, Alistair Francis >> <alistair.francis@xilinx.com> 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? >> > > Using -device would be possible (and easy to implement). The reason I > started with > a different argument name was just to avoid confusion and to make sure > I didn't interfere with hot-pluggable devices. I still feel that separating > hot-pluggable devices from bus devices is a good idea. They are initialised > in slightly different parts of the code (although this wouldn't > be hard to fix). The name 'sysbusdev' is confusing though, as it doesn't > have to connect to the sysbus. -device / device_add already covers both hot plug and cold plug. In fact, only device_add can hot plug; all -device ever does is cold plug. Plugging in a device involves wiring it up. Hot plug additionally involves communication with guest software, but let's ignore that for now. -device uses generic wiring code, guided by configuration: property "bus". This suffices for straightforward cases like PCI and USB devices. It doesn't for sysbus devices, because these may require arbitrarily complex wirings. Therefore, sysbus devices are unavailable with -device; see commit 837d371. What we need is a way for configuration to guide more general wiring. Perhaps -device can be extended. If that turns out to be impractical, we need something more expressive that also covers everything -device does now, and can cover "everything" eventually. Andreas may have ideas here. What we don't need, in my opinion, is more special-case options :) [...]
On Wed, Apr 9, 2014 at 6:02 PM, Markus Armbruster <armbru@redhat.com> wrote: > Alistair Francis <alistair.francis@xilinx.com> writes: > >> On Wed, Apr 9, 2014 at 11:28 AM, Peter Crosthwaite >> <peter.crosthwaite@xilinx.com> wrote: >>> On Wed, Mar 26, 2014 at 10:47 AM, Alistair Francis >>> <alistair.francis@xilinx.com> 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? >>> >> >> Using -device would be possible (and easy to implement). The reason I >> started with >> a different argument name was just to avoid confusion and to make sure >> I didn't interfere with hot-pluggable devices. I still feel that separating >> hot-pluggable devices from bus devices is a good idea. They are initialised >> in slightly different parts of the code (although this wouldn't >> be hard to fix). The name 'sysbusdev' is confusing though, as it doesn't >> have to connect to the sysbus. > > -device / device_add already covers both hot plug and cold plug. In > fact, only device_add can hot plug; all -device ever does is cold plug. > > Plugging in a device involves wiring it up. Hot plug additionally > involves communication with guest software, but let's ignore that for > now. > > -device uses generic wiring code, guided by configuration: property > "bus". This suffices for straightforward cases like PCI and USB > devices. It doesn't for sysbus devices, because these may require > arbitrarily complex wirings. Therefore, sysbus devices are unavailable > with -device; see commit 837d371. > > What we need is a way for configuration to guide more general wiring. > Perhaps -device can be extended. If that turns out to be impractical, > we need something more expressive that also covers everything -device > does now, and can cover "everything" eventually. Andreas may have ideas > here. I have managed to extended -device to allow QOM devices to be attached to the system bus. I removed the -sysbusdev argument and am now back to just using "-device" where the user can specify which bus to connect to. This allows entire machines to be built from command line arguments using "-M none". I'm just tidying up the patch now > > What we don't need, in my opinion, is more special-case options :) > > [...] >
cc Alex On Thu, Apr 10, 2014 at 10:54 AM, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Wed, Apr 9, 2014 at 6:02 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Alistair Francis <alistair.francis@xilinx.com> writes: >> >>> On Wed, Apr 9, 2014 at 11:28 AM, Peter Crosthwaite >>> <peter.crosthwaite@xilinx.com> wrote: >>>> On Wed, Mar 26, 2014 at 10:47 AM, Alistair Francis >>>> <alistair.francis@xilinx.com> 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? >>>> >>> >>> Using -device would be possible (and easy to implement). The reason I >>> started with >>> a different argument name was just to avoid confusion and to make sure >>> I didn't interfere with hot-pluggable devices. I still feel that separating >>> hot-pluggable devices from bus devices is a good idea. They are initialised >>> in slightly different parts of the code (although this wouldn't >>> be hard to fix). The name 'sysbusdev' is confusing though, as it doesn't >>> have to connect to the sysbus. >> >> -device / device_add already covers both hot plug and cold plug. In >> fact, only device_add can hot plug; all -device ever does is cold plug. >> >> Plugging in a device involves wiring it up. Hot plug additionally >> involves communication with guest software, but let's ignore that for >> now. >> >> -device uses generic wiring code, guided by configuration: property >> "bus". This suffices for straightforward cases like PCI and USB >> devices. It doesn't for sysbus devices, because these may require >> arbitrarily complex wirings. Therefore, sysbus devices are unavailable >> with -device; see commit 837d371. >> >> What we need is a way for configuration to guide more general wiring. >> Perhaps -device can be extended. If that turns out to be impractical, >> we need something more expressive that also covers everything -device >> does now, and can cover "everything" eventually. Andreas may have ideas >> here. > > I have managed to extended -device to allow QOM devices to be attached > to the system bus. > I removed the -sysbusdev argument and am now back to just using > "-device" where the > user can specify which bus to connect to. > This allows entire machines to be built from command line arguments > using "-M none". > > I'm just tidying up the patch now > >> >> What we don't need, in my opinion, is more special-case options :) >> >> [...] >> >
Hi, Am 26.03.2014 01:47, schrieb Alistair Francis: > 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 > > 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 > > 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 > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> Since this topic came up on today's KVM call, let me repeat here that considering how many subtle fixes have gone into the -device and device_add code paths lately, I am seriously opposed to duplicating that functionality into a new function. Checks on DeviceClass fields, reference counting etc. absolutely need to be reused for easy maintenance. Also please note that there can be multiple MemoryRegions per SysBusDevice as well as multiple IRQs, so "addr" and "irq" seem insufficient. This obviously does not yet constitute a full review of the code. Regards, Andreas
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 +@item -sysbusdev @var{device}[,@var{prop}[=@var{value}][,...]] +@findex -sysbusdev +Add sysbusdev @var{device}. @var{prop}=@var{value} 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(¤t_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();
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 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 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 Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- 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(-)