[Qemu-devel] [PATCH] device-assignment: Rework "name" of assigned pci device

[Hidetoshi Seto]

"Hao, Xudong" <address@hidden> writes:
> > When assign one PCI device, qemu fail to parse the command line:
> > qemu-system_x86 -smp 2 -m 1024 -hda /path/to/img -pcidevice host=00:19.0
> > Error:
> > qemu-system-x86_64: Parameter 'id' expects an identifier
> > Identifiers consist of letters, digits, '-', '.', '_', starting with a 
> > letter.
> > pcidevice argument parse error; please check the help text for usage
> > Could not add assigned device host=00:19.0
> >
> > https://bugs.launchpad.net/qemu/+bug/597932
> >
> > This issue caused by qemu-kvm commit 
> > b560a9ab9be06afcbb78b3791ab836dad208a239.

This patch is a response to the above report.

Thanks,
H.Seto

=====

Because use of some characters in "id" is restricted recently, assigned
device start to fail having implicit "id" that uses address string of
host device, like "00:19.0" which includes restricted character ':'.

It seems that this implicit "id" is intended to be run as "name" that
could be passed with option "-pcidevice ... ,name=..." to specify a
string to be used in log outputs.  In other words it seems that
dev->dev.qdev.id of assigned device had been used to have such
"name", that is user-defined string or address string of "host".

The problem is that "name" for specific use is not equal to "id" for
universal use.  So it is better to remove this tricky mix up here.

This patch introduces new function assigned_dev_name() that returns
proper name string for the device.
Now property "name" is explicitly defined in struct AssignedDevice.

When if the device have neither "name" nor "id", address string like
"0000:00:19.0" will be created and passed instead.  Once created, new
field r_name holds the string to be reused and to be released later.

Signed-off-by: Hidetoshi Seto <address@hidden>
---
 hw/device-assignment.c |   59 ++++++++++++++++++++++++++++++++++-------------
 hw/device-assignment.h |    2 +
 2 files changed, 44 insertions(+), 17 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 585162b..d73516f 100644
--- a/hw/device-assignment.c
+++ b/hw/device-assignment.c
@@ -62,6 +62,25 @@ static void assigned_dev_load_option_rom(AssignedDevice 
*dev);
 
 static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev);
 
+static const char *assigned_dev_name(AssignedDevice *dev)
+{
+    /* use user-defined "name" if specified */
+    if (dev->u_name)
+        return dev->u_name;
+    /* else use "id" if available */
+    if (dev->dev.qdev.id)
+        return dev->dev.qdev.id;
+    /* otherwise use address of host device instead */
+    if (!dev->r_name) {
+        char buf[32];
+
+        snprintf(buf, sizeof(buf), "%04x:%02x:%02x.%01x",
+                 dev->host.seg, dev->host.bus, dev->host.dev, dev->host.func);
+        dev->r_name = qemu_strdup(buf);
+    }
+    return dev->r_name;
+}
+
 static uint32_t guest_to_host_ioport(AssignedDevRegion *region, uint32_t addr)
 {
     return region->u.r_baseport + (addr - region->e_physbase);
@@ -798,6 +817,10 @@ static void free_assigned_device(AssignedDevice *dev)
             dev->real_device.config_fd = 0;
         }
 
+    if (dev->r_name) {
+        qemu_free(dev->r_name);
+    }
+
 #ifdef KVM_CAP_IRQ_ROUTING
         free_dev_irq_entries(dev);
 #endif
@@ -885,7 +908,7 @@ static int assign_device(AssignedDevice *dev)
     if (dev->use_iommu) {
         if (!kvm_check_extension(kvm_state, KVM_CAP_IOMMU)) {
             fprintf(stderr, "No IOMMU found.  Unable to assign device 
\"%s\"\n",
-                    dev->dev.qdev.id);
+                    assigned_dev_name(dev));
             return -ENODEV;
         }
         assigned_dev_data.flags |= KVM_DEV_ASSIGN_ENABLE_IOMMU;
@@ -897,7 +920,7 @@ static int assign_device(AssignedDevice *dev)
     r = kvm_assign_pci_device(kvm_context, &assigned_dev_data);
     if (r < 0) {
         fprintf(stderr, "Failed to assign device \"%s\" : %s\n",
-                dev->dev.qdev.id, strerror(-r));
+                assigned_dev_name(dev), strerror(-r));
 
         switch (r) {
             case -EBUSY:
@@ -953,7 +976,7 @@ static int assign_irq(AssignedDevice *dev)
     r = kvm_assign_irq(kvm_context, &assigned_irq_data);
     if (r < 0) {
         fprintf(stderr, "Failed to assign irq for \"%s\": %s\n",
-                dev->dev.qdev.id, strerror(-r));
+                assigned_dev_name(dev), strerror(-r));
         fprintf(stderr, "Perhaps you are assigning a device "
                 "that shares an IRQ with another device?\n");
         return r;
@@ -977,7 +1000,7 @@ static void deassign_device(AssignedDevice *dev)
     r = kvm_deassign_pci_device(kvm_context, &assigned_dev_data);
     if (r < 0)
        fprintf(stderr, "Failed to deassign device \"%s\" : %s\n",
-                dev->dev.qdev.id, strerror(-r));
+                assigned_dev_name(dev), strerror(-r));
 #endif
 }
 
@@ -1421,7 +1444,7 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
     if (get_real_device(dev, dev->host.seg, dev->host.bus,
                         dev->host.dev, dev->host.func)) {
         error_report("pci-assign: Error: Couldn't get real device (%s)!",
-                     dev->dev.qdev.id);
+                     assigned_dev_name(dev));
         goto out;
     }
 
@@ -1487,8 +1510,9 @@ static int parse_hostaddr(DeviceState *dev, Property 
*prop, const char *str)
     PCIHostDevice *ptr = qdev_get_prop_ptr(dev, prop);
     int rc;
 
-    rc = pci_parse_host_devaddr(str, &ptr->seg, &ptr->bus, &ptr->dev, 
&ptr->func);
-    if (rc != 0)
+    rc = pci_parse_host_devaddr(str,
+                                &ptr->seg, &ptr->bus, &ptr->dev, &ptr->func);
+    if (rc)
         return -1;
     return 0;
 }
@@ -1497,7 +1521,8 @@ static int print_hostaddr(DeviceState *dev, Property 
*prop, char *dest, size_t l
 {
     PCIHostDevice *ptr = qdev_get_prop_ptr(dev, prop);
 
-    return snprintf(dest, len, "%02x:%02x.%x", ptr->bus, ptr->dev, ptr->func);
+    return snprintf(dest, len, "%04x:%02x:%02x.%x",
+                    ptr->seg, ptr->bus, ptr->dev, ptr->func);
 }
 
 PropertyInfo qdev_prop_hostaddr = {
@@ -1520,6 +1545,7 @@ static PCIDeviceInfo assign_info = {
         DEFINE_PROP("host", AssignedDevice, host, qdev_prop_hostaddr, 
PCIHostDevice),
         DEFINE_PROP_UINT32("iommu", AssignedDevice, use_iommu, 1),
         DEFINE_PROP_STRING("configfd", AssignedDevice, configfd_name),
+        DEFINE_PROP_STRING("name", AssignedDevice, u_name),
         DEFINE_PROP_END_OF_LIST(),
     },
 };
@@ -1545,24 +1571,25 @@ device_init(assign_register_devices)
 QemuOpts *add_assigned_device(const char *arg)
 {
     QemuOpts *opts = NULL;
-    char host[64], id[64], dma[8];
+    char host[64], buf[64], dma[8];
     int r;
 
+    /* "host" must be with -pcidevice */
     r = get_param_value(host, sizeof(host), "host", arg);
     if (!r)
          goto bad;
-    r = get_param_value(id, sizeof(id), "id", arg);
-    if (!r)
-        r = get_param_value(id, sizeof(id), "name", arg);
-    if (!r)
-        r = get_param_value(id, sizeof(id), "host", arg);
 
-    opts = qemu_opts_create(&qemu_device_opts, id, 0);
+    opts = qemu_opts_create(&qemu_device_opts, NULL, 0);
     if (!opts)
         goto bad;
     qemu_opt_set(opts, "driver", "pci-assign");
     qemu_opt_set(opts, "host", host);
 
+    /* Log outputs use "name" if specified */
+    r = get_param_value(buf, sizeof(buf), "name", arg);
+    if (r)
+        qemu_opt_set(opts, "name", buf);
+
 #ifdef KVM_CAP_IOMMU
     r = get_param_value(dma, sizeof(dma), "dma", arg);
     if (r && !strncmp(dma, "none", 4))
@@ -1574,8 +1601,6 @@ QemuOpts *add_assigned_device(const char *arg)
 bad:
     fprintf(stderr, "pcidevice argument parse error; "
             "please check the help text for usage\n");
-    if (opts)
-        qemu_opts_del(opts);
     return NULL;
 }
 
diff --git a/hw/device-assignment.h b/hw/device-assignment.h
index 4e7fe87..fb00e29 100644
--- a/hw/device-assignment.h
+++ b/hw/device-assignment.h
@@ -86,6 +86,8 @@ typedef struct AssignedDevice {
     unsigned int h_segnr;
     unsigned char h_busnr;
     unsigned int h_devfn;
+    char *u_name;
+    char *r_name;
     int irq_requested_type;
     int bound;
     struct {
-- 
1.7.0