[Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Gerd Hoffmann]

Ok, lets start to collect some low-hanging fruit we can get from the
qdev bits even at the current early state.

==> stop adding cmd line switches for each and every device <==
==>        do funky stuff you could not do before           <==

Here is a patch adding a generic -pcidevice command line switch.  Works
only for qdev-converted devices.  Works (for now) only for devices which
don't need configuration (i.e. nics don't work or maybe use vlan0
unconditionally).  Most useful with my qdev patch queue applied.

Examples:

  -pcidevice ES1370,addr=42        (replaces -audio es1370).
  -pcidevice virtio-balloon-pci    (replaces -balloon).
  -pcidevice lsi53c895a,addr=23    (add scsi adapter in specified slot).
  -pcidevice "PIIX3 USB-UHCI"      (add second usb bus).

Comments?

Note that we proably need a bunch of additional cleanups like killing
the -balloon switch and maybe some safety bells to stop users from doing
crazy stuff like adding a second acpi adapter.  Might be the scsi
example doesn't actually work with disks due to initialization order
issues.  None of this is in this RfC patch yet.

Cc: Markus Armbruster <armbru@redhat.com>
---
 hw/pci.c        |   29 +++++++++++++++++++++++++++++
 hw/pci.h        |    1 +
 hw/qdev.c       |   13 +++++++++++++
 hw/qdev.h       |    3 +++
 qemu-options.hx |    2 ++
 vl.c            |   28 +++++++++++++++++++++++-----
 6 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 5925617..22af5b8 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -828,6 +828,35 @@ PCIDevice *pci_create(const char *name, const char *devaddr)
     return (PCIDevice *)dev;
 }
 
+PCIDevice *pci_device_add(const char *cmdline)
+{
+    PCIDevice *dev;
+    char driver[32], addr[32] = "";
+    const char *params;
+    int n = 0;
+
+    if (1 != sscanf(cmdline, "%32[^,],%n", driver, &n)) {
+        fprintf(stderr, "pcidevice parse error: \"%s\"\n", cmdline);
+        return NULL;
+    }
+    if (strcmp(driver, "?") == 0) {
+        do_list_qdrv(&pci_bus_info);
+        return NULL;
+    }
+    if (n) {
+        params = cmdline + n;
+        get_param_value(addr, sizeof(addr), "addr", params);
+    }
+    fprintf(stderr, "%s: driver \"%s\" addr \"%s\"\n",
+            __FUNCTION__, driver, addr);
+    dev = pci_create(driver, strlen(addr) ? addr : NULL);
+
+    /* TODO: parse device specific args and add as attributes */
+
+    qdev_init(&dev->qdev);
+    return dev;
+}
+
 static const char * const pci_nic_models[] = {
     "ne2k_pci",
     "i82551",
diff --git a/hw/pci.h b/hw/pci.h
index 6d82801..aca369d 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -229,6 +229,7 @@ void pci_qdev_register_many(PCIDeviceInfo *info);
 
 PCIDevice *pci_create(const char *name, const char *devaddr);
 PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
+PCIDevice *pci_device_add(const char *cmdline);
 
 /* lsi53c895a.c */
 #define LSI_MAX_DEVS 7
diff --git a/hw/qdev.c b/hw/qdev.c
index b5068ba..7ae71aa 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -411,3 +411,16 @@ void do_info_qdrv(Monitor *mon)
                        info->name, info->bus_info->name);
     }
 }
+
+void do_list_qdrv(BusInfo *bus)
+{
+    DeviceInfo *info;
+
+    if (bus)
+        fprintf(stderr, "%s bus drivers:\n", bus->name);
+    for (info = device_info_list; info != NULL; info = info->next) {
+        if (bus && info->bus_info != bus)
+            continue;
+        fprintf(stderr, "  %s\n", info->name);
+    }
+}
diff --git a/hw/qdev.h b/hw/qdev.h
index 5a84fbf..2dbf414 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -126,4 +126,7 @@ void do_info_qtree(Monitor *mon);
 void do_info_qbus(Monitor *mon);
 void do_info_qdrv(Monitor *mon);
 
+/* helper for "-${bus}device ?" */
+void do_list_qdrv(BusInfo *bus);
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index 503da33..2ff3353 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -364,6 +364,8 @@ Network adapter that supports CDC ethernet and RNDIS protocols.
 @end table
 ETEXI
 
+DEF("pcidevice", HAS_ARG, QEMU_OPTION_pcidevice,
+    "-pcidevice name add the host or guest PCI device 'name'\n")
 DEF("name", HAS_ARG, QEMU_OPTION_name,
     "-name string    set the name of the guest\n")
 STEXI
diff --git a/vl.c b/vl.c
index cfcf437..61865d2 100644
--- a/vl.c
+++ b/vl.c
@@ -142,6 +142,7 @@ int main(int argc, char **argv)
 #include "hw/watchdog.h"
 #include "hw/smbios.h"
 #include "hw/xen.h"
+#include "hw/pci.h"
 #include "bt-host.h"
 #include "net.h"
 #include "monitor.h"
@@ -183,11 +184,10 @@ int main(int argc, char **argv)
 
 #define DEFAULT_RAM_SIZE 128
 
-/* Max number of USB devices that can be specified on the commandline.  */
-#define MAX_USB_CMDLINE 8
-
-/* Max number of bluetooth switches on the commandline.  */
-#define MAX_BT_CMDLINE 10
+/* Max number of devices that can be specified on the commandline.  */
+#define MAX_USB_CMDLINE  8
+#define MAX_PCI_CMDLINE  8
+#define MAX_BT_CMDLINE  10
 
 /* XXX: use a two level table to limit memory usage */
 #define MAX_IOPORTS 65536
@@ -4944,6 +4944,8 @@ int main(int argc, char **argv, char **envp)
     const char *cpu_model;
     const char *usb_devices[MAX_USB_CMDLINE];
     int usb_devices_index;
+    const char *pci_devices[MAX_PCI_CMDLINE];
+    int pci_devices_index;
 #ifndef _WIN32
     int fds[2];
 #endif
@@ -5024,6 +5026,7 @@ int main(int argc, char **argv, char **envp)
     }
 
     usb_devices_index = 0;
+    pci_devices_index = 0;
 
     nb_net_clients = 0;
     nb_bt_opts = 0;
@@ -5526,6 +5529,14 @@ int main(int argc, char **argv, char **envp)
                 usb_devices[usb_devices_index] = optarg;
                 usb_devices_index++;
                 break;
+            case QEMU_OPTION_pcidevice:
+                if (pci_devices_index >= MAX_PCI_CMDLINE) {
+                    fprintf(stderr, "Too many PCI devices\n");
+                    exit(1);
+                }
+                pci_devices[pci_devices_index] = optarg;
+                pci_devices_index++;
+                break;
             case QEMU_OPTION_smp:
                 smp_cpus = atoi(optarg);
                 if (smp_cpus < 1) {
@@ -6041,6 +6052,13 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
+    /* init PCI devices */
+    for (i = 0; i < pci_devices_index; i++) {
+        if (pci_device_add(pci_devices[i]) == NULL) {
+            exit(1);
+        }
+    }
+
     if (!display_state)
         dumb_display_init();
     /* just use the first displaystate for the moment */
-- 
1.6.2.5

Re: [Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Anthony Liguori]

Gerd Hoffmann wrote:
> Ok, lets start to collect some low-hanging fruit we can get from the
> qdev bits even at the current early state.
>
> ==> stop adding cmd line switches for each and every device <==
> ==>        do funky stuff you could not do before           <==
>
> Here is a patch adding a generic -pcidevice command line switch.  Works
> only for qdev-converted devices.  Works (for now) only for devices which
> don't need configuration (i.e. nics don't work or maybe use vlan0
> unconditionally).  Most useful with my qdev patch queue applied.
>
> Examples:
>
>   -pcidevice ES1370,addr=42        (replaces -audio es1370).
>   -pcidevice virtio-balloon-pci    (replaces -balloon).
>   -pcidevice lsi53c895a,addr=23    (add scsi adapter in specified slot).
>   -pcidevice "PIIX3 USB-UHCI"      (add second usb bus).
>
> Comments?
>   
You read my mind :-)

I was just thinking about this as I was looking over staging.

What I would like to see is that this internal mechanism be used to 
implement the switches it replaces.  That is, -balloon should just add 
just be a convenience option that expands to -pcidevice.  The same for 
-audio.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Ok, lets start to collect some low-hanging fruit we can get from the
> qdev bits even at the current early state.
>
> ==> stop adding cmd line switches for each and every device <==
> ==>        do funky stuff you could not do before           <==
>
> Here is a patch adding a generic -pcidevice command line switch.  Works
> only for qdev-converted devices.  Works (for now) only for devices which
> don't need configuration (i.e. nics don't work or maybe use vlan0
> unconditionally).  Most useful with my qdev patch queue applied.
>
> Examples:
>
>   -pcidevice ES1370,addr=42        (replaces -audio es1370).
>   -pcidevice virtio-balloon-pci    (replaces -balloon).
>   -pcidevice lsi53c895a,addr=23    (add scsi adapter in specified slot).
>   -pcidevice "PIIX3 USB-UHCI"      (add second usb bus).
>
> Comments?
>
> Note that we proably need a bunch of additional cleanups like killing
> the -balloon switch and maybe some safety bells to stop users from doing
> crazy stuff like adding a second acpi adapter.  Might be the scsi
> example doesn't actually work with disks due to initialization order
> issues.  None of this is in this RfC patch yet.
>
> Cc: Markus Armbruster <armbru@redhat.com>

Why limit this to PCI devices?  The device name indirectly specifies the
bus type.  Parameter addr then specifies bus instance and device address
on the bus, in a bus-specific syntax.  Of course, it's perfectly fine to
implement only PCI initially, but I'd rather not encode that restriction
in the option name :)

To extend this to devices that need configuration: what about
interpreting additional parameters as qdev properties?  Oh, there's a
TODO in pci_device_add() that suggests exactly that.

Re: [Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Gerd Hoffmann]

On 06/29/09 16:51, Anthony Liguori wrote:
> Gerd Hoffmann wrote:

>> -pcidevice ES1370,addr=42 (replaces -audio es1370).
>> -pcidevice virtio-balloon-pci (replaces -balloon).
>> -pcidevice lsi53c895a,addr=23 (add scsi adapter in specified slot).
>> -pcidevice "PIIX3 USB-UHCI" (add second usb bus).

> What I would like to see is that this internal mechanism be used to
> implement the switches it replaces. That is, -balloon should just add
> just be a convenience option that expands to -pcidevice. The same for
> -audio.

Hmm, I was thinking about dropping the -balloon and -audio switches 
altogether.  Or do we need them for backward compatibility?

cheers,
   Gerd

Re: [Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Gerd Hoffmann]

On 06/29/09 23:02, Markus Armbruster wrote:

> Why limit this to PCI devices?

Good question.  I think we can go straight for a -device option.

> The device name indirectly specifies the
> bus type.  Parameter addr then specifies bus instance and device address
> on the bus, in a bus-specific syntax.  Of course, it's perfectly fine to
> implement only PCI initially, but I'd rather not encode that restriction
> in the option name :)

usb will be easy as well once my patches are in.

> To extend this to devices that need configuration: what about
> interpreting additional parameters as qdev properties?  Oh, there's a
> TODO in pci_device_add() that suggests exactly that.

cheers,
   Gerd

Re: [Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Gerd Hoffmann]

On 06/30/09 10:00, Gerd Hoffmann wrote:
> On 06/29/09 23:02, Markus Armbruster wrote:
>
>> Why limit this to PCI devices?
>
> Good question. I think we can go straight for a -device option.

Hmm.  Not *that* simple, need to dig into attributes first.  Hope I have 
something later this week.

cheers,
   Gerd

Re: [Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Paul Brook]

On Monday 29 June 2009, Gerd Hoffmann wrote:
> Here is a patch adding a generic -pcidevice command line switch.

This sort of feature is one of the reasons I object to "partial" qdev 
conversions. A partially converted device will break in unfriendly ways when 
you try to instantiate it via this option or a machine config file.

Paul

Re: [Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Markus Armbruster]

Paul Brook <paul@codesourcery.com> writes:

> On Monday 29 June 2009, Gerd Hoffmann wrote:
>> Here is a patch adding a generic -pcidevice command line switch.
>
> This sort of feature is one of the reasons I object to "partial" qdev 
> conversions. A partially converted device will break in unfriendly ways when 
> you try to instantiate it via this option or a machine config file.
>
> Paul

I don't think these are sensible reasons to block progress on qdev
conversions.

The machine config file does not exist, and won't be useful until the
devices are converted to qdev.

-pcidevice can be useful now, for devices converted to qdev, but it
doesn't fully work for certain incomplete conversions.  While that's
inacceptable for a release, is it really so horrible as a transient step
now?  Besides, we can easily flag incompletely converted devices, and
make -pcidevice not touch them.

Re: [Qemu-devel] [RFC PATCH] qdev: add -pcidevice command line option.

[Gerd Hoffmann]

   Hi,

> -pcidevice can be useful now, for devices converted to qdev, but it
> doesn't fully work for certain incomplete conversions.

For incomplete conversions it may cause all sorts of strange effects, so 
Paul has a point here IMHO.

> While that's
> inacceptable for a release, is it really so horrible as a transient step
> now?  Besides, we can easily flag incompletely converted devices, and
> make -pcidevice not touch them.

While thinking about that:  We probably need a flag in DeviceInfo anyway 
to tag devices which are not supposed to be added via -pcidevice (soon 
to be -device).  It doesn't make much sense to allow the user adding - 
say - interrupt controllers via command line.

Such a flag could also be used to prevent incompletely converted drivers 
to be used via -device.

cheers,
   Gerd