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

[Markus Armbruster <armbru@redhat.com>]

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.