qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alistair Francis
Subject: Re: [Qemu-devel] [PATCH v1 1/1] vl.c: Allow sysbus devices to be attached via commandline
Date: Wed, 9 Apr 2014 14:48:13 +1000

On Wed, Apr 9, 2014 at 11:28 AM, Peter Crosthwaite
<address@hidden> wrote:
> 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?
>

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]