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

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

[Hidetoshi Seto]

"Hao, Xudong" <xudong.hao@intel.com> 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 <seto.hidetoshi@jp.fujitsu.com>
---
 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

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

[Markus Armbruster]

Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> writes:

> "Hao, Xudong" <xudong.hao@intel.com> 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".

As far as I can tell, option "name" is just a leftover from pre-qdev
days, kept for compatibility.

> 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 <seto.hidetoshi@jp.fujitsu.com>

Comments inline.

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

This is style cleanup.  Please don't mix that with functional changes in
the same patch.

> @@ -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);
>  }

Properly covering seg here is an unrelated fix.  Separate patch please.

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

I think you break option id here.  Its value must become the qdev ID,
visible in info qtree and usable as argument to device_del.  And if
option id is missing, option name must become the qdev ID, for backwards
compatibility.

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

Do you really need u_name?  There's id.

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

[Hidetoshi Seto]

Thanks Markus,

(2010/06/29 14:28), Markus Armbruster wrote:
> Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> writes:
> 
>> "Hao, Xudong" <xudong.hao@intel.com> 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".
> 
> As far as I can tell, option "name" is just a leftover from pre-qdev
> days, kept for compatibility.

Yea, I see.
I don't know well about the history of such pre-qdev days...

>> 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 <seto.hidetoshi@jp.fujitsu.com>
> 
> Comments inline.
> 
>> ---
>>  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;
>>  }
> 
> This is style cleanup.  Please don't mix that with functional changes in
> the same patch.

Right, I will.

>> @@ -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);
>>  }
> 
> Properly covering seg here is an unrelated fix.  Separate patch please.

Ditto, I will.

>>  
>>  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);
> 
> I think you break option id here.  Its value must become the qdev ID,
> visible in info qtree and usable as argument to device_del.  And if
> option id is missing, option name must become the qdev ID, for backwards
> compatibility.

Ah, I missed to check hot-add path - I had wonder why id could be here
since I could not find documents that mentions use of id with -pcidevice.

So, I should use id here if specified. That's right,

But in case if id is missing and name is specified, I think there is no
reason that the characters in name should be restricted in same manner
with that in id.

In case that there is neither id nor name but host (in fact it is original
case) now ID "BB:DD.F" (like 00:19.0) will be rejected (because it starts
with digit, plus it uses restricted ':').

In other words, your change already breaks backwards compatibility because it
prevents "-pcidevice host=BB:DD.F" from creating ID "BB:DD.F" that might be
used as argument to device_del etc.

BTW I think "-pcidevice host=BB:DD.F,name=1394" is an another litmus test.

My opinion is that we should not do any fall back here if id is missing.
Using name or host as id is wrong, since it could be said that in such case
id is specified as "unspecified".

>>      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 {
> 
> Do you really need u_name?  There's id.

For backwards compatibility, and to have name string with no restriction,
it is needed, I think.

(Personally speaking I agree to recommend '-device + id=' instead of
 '-pcidevice + name=')


Thanks,
H.Seto

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

[Markus Armbruster]

Summary: upstream qemu commit b560a9ab broke -pcidevice and pci_add host
in two ways:

* Use without options id and name is broken when option host contains
  ':'.  That's because id defaults to host.  I recommend to fix it
  incompatibly: don't default id to host.  The alternative is to get
  upstream qemu to accept ':' in qdev IDs again.

* Funny characters in option name no longer work.  Same as funny
  characters in id options all over the place.  Intentional change.  I
  recommend to do nothing.

Details inline.

Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> writes:

> Thanks Markus,
>
> (2010/06/29 14:28), Markus Armbruster wrote:
>> Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> writes:
>> 
>>> "Hao, Xudong" <xudong.hao@intel.com> 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".
>> 
>> As far as I can tell, option "name" is just a leftover from pre-qdev
>> days, kept for compatibility.
>
> Yea, I see.
> I don't know well about the history of such pre-qdev days...

It's often useful to examine history to figure out what a piece of code
attempts to accomplish.  git-blame and git-log are your friends :)

>>> 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.
[...]
>>> @@ -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);
>> 
>> I think you break option id here.  Its value must become the qdev ID,
>> visible in info qtree and usable as argument to device_del.  And if
>> option id is missing, option name must become the qdev ID, for backwards
>> compatibility.
>
> Ah, I missed to check hot-add path - I had wonder why id could be here
> since I could not find documents that mentions use of id with -pcidevice.
>
> So, I should use id here if specified. That's right,
>
> But in case if id is missing and name is specified, I think there is no
> reason that the characters in name should be restricted in same manner
> with that in id.
>
> In case that there is neither id nor name but host (in fact it is original
> case) now ID "BB:DD.F" (like 00:19.0) will be rejected (because it starts
> with digit, plus it uses restricted ':').
>
> In other words, your change already breaks backwards compatibility because it
> prevents "-pcidevice host=BB:DD.F" from creating ID "BB:DD.F" that might be
> used as argument to device_del etc.
>
> BTW I think "-pcidevice host=BB:DD.F,name=1394" is an another litmus test.
>
> My opinion is that we should not do any fall back here if id is missing.
> Using name or host as id is wrong, since it could be said that in such case
> id is specified as "unspecified".

Commit b560a9ab broke name the same way it broke any other qdev ID: you
can't use funny characters anymore.  Intentional change.

name exists for backward compatibility only.  Changing its meaning
incompatibly (namely not to affect qdev ID) makes no sense to me; we
could just as well remove it then.

The commit also broke defaulting id to host, because host can contain
the funny character ':'.  I wasn't aware of that when I made the change
in upstream qemu (-pcidevice is qemu-kvm only, easy to lose sight of
it).  The one way to fix this compatibly is to get upstream qemu accept
':' in IDs.

If we're fine with incompatible change here, or upstream qemu forces the
incompatible change on us by refusing to accept ':', then I think we
should simply drop the fallback.

We could continue to fall back to strings that pass id_wellformed().
Don't like it.

Without the fallback, there is no use of name left.  Your patch creates
a new one: use it instead of qdev ID in messages.  Why is that useful?

>>>      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 {
>> 
>> Do you really need u_name?  There's id.
>
> For backwards compatibility, and to have name string with no restriction,
> it is needed, I think.

Why do we need a name string for -pcidevice, but not for other devices?

> (Personally speaking I agree to recommend '-device + id=' instead of
>  '-pcidevice + name=')

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

[Hidetoshi Seto]

(2010/06/30 15:53), Markus Armbruster wrote:
> Summary: upstream qemu commit b560a9ab broke -pcidevice and pci_add host
> in two ways:
> 
> * Use without options id and name is broken when option host contains
>   ':'.  That's because id defaults to host.  I recommend to fix it
>   incompatibly: don't default id to host.  The alternative is to get
>   upstream qemu to accept ':' in qdev IDs again.
> 
> * Funny characters in option name no longer work.  Same as funny
>   characters in id options all over the place.  Intentional change.  I
>   recommend to do nothing.

Thanks a lot.
I'm not a person in really need, so now I'm going to follow your
recommendation.

> Details inline.
> 
> Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> writes:
> 
>> Thanks Markus,
>>
>> (2010/06/29 14:28), Markus Armbruster wrote:
>>> Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> writes:
>>>
>>>> "Hao, Xudong" <xudong.hao@intel.com> 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".
>>>
>>> As far as I can tell, option "name" is just a leftover from pre-qdev
>>> days, kept for compatibility.
>>
>> Yea, I see.
>> I don't know well about the history of such pre-qdev days...
> 
> It's often useful to examine history to figure out what a piece of code
> attempts to accomplish.  git-blame and git-log are your friends :)

I often play with git-log, however I have a little trouble here since
qemu tree is too young.

>>>> 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.
> [...]
>>>> @@ -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);
>>>
>>> I think you break option id here.  Its value must become the qdev ID,
>>> visible in info qtree and usable as argument to device_del.  And if
>>> option id is missing, option name must become the qdev ID, for backwards
>>> compatibility.
>>
>> Ah, I missed to check hot-add path - I had wonder why id could be here
>> since I could not find documents that mentions use of id with -pcidevice.
>>
>> So, I should use id here if specified. That's right,
>>
>> But in case if id is missing and name is specified, I think there is no
>> reason that the characters in name should be restricted in same manner
>> with that in id.
>>
>> In case that there is neither id nor name but host (in fact it is original
>> case) now ID "BB:DD.F" (like 00:19.0) will be rejected (because it starts
>> with digit, plus it uses restricted ':').
>>
>> In other words, your change already breaks backwards compatibility because it
>> prevents "-pcidevice host=BB:DD.F" from creating ID "BB:DD.F" that might be
>> used as argument to device_del etc.
>>
>> BTW I think "-pcidevice host=BB:DD.F,name=1394" is an another litmus test.
>>
>> My opinion is that we should not do any fall back here if id is missing.
>> Using name or host as id is wrong, since it could be said that in such case
>> id is specified as "unspecified".
> 
> Commit b560a9ab broke name the same way it broke any other qdev ID: you
> can't use funny characters anymore.  Intentional change.
> 
> name exists for backward compatibility only.  Changing its meaning
> incompatibly (namely not to affect qdev ID) makes no sense to me; we
> could just as well remove it then.

Well, I think I'll very happy if name have gone. 

What I'm concerning here is the impact of incompatible brought by commit
b560a9ab.  It makes some old settings/config files to fail creating VMs.

My first stand point was "it is better to allow old configs with no changes
to work in some degree" ... But now I understand that "original ID change
already has considerable impact on such old configs and it will force
rewrite on them anyway, so this patch for minor compatibility of 'name'
will never be any help".

> The commit also broke defaulting id to host, because host can contain
> the funny character ':'.  I wasn't aware of that when I made the change
> in upstream qemu (-pcidevice is qemu-kvm only, easy to lose sight of
> it).  The one way to fix this compatibly is to get upstream qemu accept
> ':' in IDs.

Still I'm not lectured about the evils of ':' in IDs... :-(

> If we're fine with incompatible change here, or upstream qemu forces the
> incompatible change on us by refusing to accept ':', then I think we
> should simply drop the fallback.
> 
> We could continue to fall back to strings that pass id_wellformed().
> Don't like it.
> 
> Without the fallback, there is no use of name left.  Your patch creates
> a new one: use it instead of qdev ID in messages.  Why is that useful?

I think "[domain:]B:D.F" is enough good standard format to point a PCI
device.  At least it is used for "host=".  I don't know why it is not
acceptable for use as ID.

And I imagined that someone will feel uneasy if outputs start to have
no B:D.F in it... But there could be other approach, like adding B:D.F
as prefix for output of assigned devices, no matter id/name is present
or not.

I don't know why name have invented here... but I guess there should
be a reason because there it is.  I'll wait someone who can tell it.

>>>>      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 {
>>>
>>> Do you really need u_name?  There's id.
>>
>> For backwards compatibility, and to have name string with no restriction,
>> it is needed, I think.
> 
> Why do we need a name string for -pcidevice, but not for other devices?

Though I don't have good answer for that, but it could be propagated for
other devices if needed, as an alias, or something like comm of process
that already have pid.  But maybe driver name is enough for some of such
use.

>> (Personally speaking I agree to recommend '-device + id=' instead of
>>  '-pcidevice + name=')


Thanks,
H.Seto

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

[Markus Armbruster]

[cc: kraxel]

Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> writes:

> (2010/06/30 15:53), Markus Armbruster wrote:
>> Summary: upstream qemu commit b560a9ab broke -pcidevice and pci_add host
>> in two ways:
>> 
>> * Use without options id and name is broken when option host contains
>>   ':'.  That's because id defaults to host.  I recommend to fix it
>>   incompatibly: don't default id to host.  The alternative is to get
>>   upstream qemu to accept ':' in qdev IDs again.
>> 
>> * Funny characters in option name no longer work.  Same as funny
>>   characters in id options all over the place.  Intentional change.  I
>>   recommend to do nothing.
>
> Thanks a lot.
> I'm not a person in really need, so now I'm going to follow your
> recommendation.
>
>> Details inline.
>> 
>> Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> writes:
>> 
>>> Thanks Markus,
>>>
>>> (2010/06/29 14:28), Markus Armbruster wrote:
>>>> Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> writes:
>>>>
>>>>> "Hao, Xudong" <xudong.hao@intel.com> 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".
>>>>
>>>> As far as I can tell, option "name" is just a leftover from pre-qdev
>>>> days, kept for compatibility.
>>>
>>> Yea, I see.
>>> I don't know well about the history of such pre-qdev days...
>> 
>> It's often useful to examine history to figure out what a piece of code
>> attempts to accomplish.  git-blame and git-log are your friends :)
>
> I often play with git-log, however I have a little trouble here since
> qemu tree is too young.
>
>>>>> 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.
>> [...]
>>>>> @@ -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);
>>>>
>>>> I think you break option id here.  Its value must become the qdev ID,
>>>> visible in info qtree and usable as argument to device_del.  And if
>>>> option id is missing, option name must become the qdev ID, for backwards
>>>> compatibility.
>>>
>>> Ah, I missed to check hot-add path - I had wonder why id could be here
>>> since I could not find documents that mentions use of id with -pcidevice.
>>>
>>> So, I should use id here if specified. That's right,
>>>
>>> But in case if id is missing and name is specified, I think there is no
>>> reason that the characters in name should be restricted in same manner
>>> with that in id.
>>>
>>> In case that there is neither id nor name but host (in fact it is original
>>> case) now ID "BB:DD.F" (like 00:19.0) will be rejected (because it starts
>>> with digit, plus it uses restricted ':').
>>>
>>> In other words, your change already breaks backwards compatibility because it
>>> prevents "-pcidevice host=BB:DD.F" from creating ID "BB:DD.F" that might be
>>> used as argument to device_del etc.
>>>
>>> BTW I think "-pcidevice host=BB:DD.F,name=1394" is an another litmus test.
>>>
>>> My opinion is that we should not do any fall back here if id is missing.
>>> Using name or host as id is wrong, since it could be said that in such case
>>> id is specified as "unspecified".
>> 
>> Commit b560a9ab broke name the same way it broke any other qdev ID: you
>> can't use funny characters anymore.  Intentional change.
>> 
>> name exists for backward compatibility only.  Changing its meaning
>> incompatibly (namely not to affect qdev ID) makes no sense to me; we
>> could just as well remove it then.
>
> Well, I think I'll very happy if name have gone. 
>
> What I'm concerning here is the impact of incompatible brought by commit
> b560a9ab.  It makes some old settings/config files to fail creating VMs.
>
> My first stand point was "it is better to allow old configs with no changes
> to work in some degree" ... But now I understand that "original ID change
> already has considerable impact on such old configs and it will force
> rewrite on them anyway, so this patch for minor compatibility of 'name'
> will never be any help".
>
>> The commit also broke defaulting id to host, because host can contain
>> the funny character ':'.  I wasn't aware of that when I made the change
>> in upstream qemu (-pcidevice is qemu-kvm only, easy to lose sight of
>> it).  The one way to fix this compatibly is to get upstream qemu accept
>> ':' in IDs.
>
> Still I'm not lectured about the evils of ':' in IDs... :-(

My initial patch permitted ':', actually.  I took it out to overcome
Paul Brook's objection.  If I had known about -pcidevice's use of ':'...

>> If we're fine with incompatible change here, or upstream qemu forces the
>> incompatible change on us by refusing to accept ':', then I think we
>> should simply drop the fallback.
>> 
>> We could continue to fall back to strings that pass id_wellformed().
>> Don't like it.
>> 
>> Without the fallback, there is no use of name left.  Your patch creates
>> a new one: use it instead of qdev ID in messages.  Why is that useful?
>
> I think "[domain:]B:D.F" is enough good standard format to point a PCI
> device.  At least it is used for "host=".  I don't know why it is not
> acceptable for use as ID.
>
> And I imagined that someone will feel uneasy if outputs start to have
> no B:D.F in it... But there could be other approach, like adding B:D.F
> as prefix for output of assigned devices, no matter id/name is present
> or not.
>
> I don't know why name have invented here... but I guess there should
> be a reason because there it is.  I'll wait someone who can tell it.

As far as I can tell, "name" predates the qdev conversion, and was used
just for error messages and such.  It defaulted to "host".  When Gerd
did the qdev conversion, he made "id" default to "name", then "host".
See commit 6b5bbd04.

Defaulting "id" that way was probably not such a good idea.  We
generally don't make up qdev IDs, because that risks collision with
user-specified IDs.

Since we've broken compatibility already, I figure we could just as well
stop defaulting "id" to "host".  When we need to identify the device to
the user, use "id" if it exists, else its PCI address.

[...]

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

[Gerd Hoffmann]

   Hi,

> As far as I can tell, "name" predates the qdev conversion, and was used
> just for error messages and such.

Yes, was already there when I touched the code the first time.

> It defaulted to "host".  When Gerd
> did the qdev conversion, he made "id" default to "name", then "host".
> See commit 6b5bbd04.
>
> Defaulting "id" that way was probably not such a good idea.  We
> generally don't make up qdev IDs, because that risks collision with
> user-specified IDs.
>
> Since we've broken compatibility already, I figure we could just as well
> stop defaulting "id" to "host".  When we need to identify the device to
> the user, use "id" if it exists, else its PCI address.

Agree, we should not make up defaults for 'id'.  I did that in a few 
places where some naming existed already to ease transition.  nics with 
name= used to get that as default id too.  But in the end it turned out 
it caused more trouble than it helped ...

cheers,
   Gerd