Hidetoshi Seto <seto.hidetoshi@xxxxxxxxxxxxxx> writes: > "Hao, Xudong" <xudong.hao@xxxxxxxxx> 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@xxxxxxxxxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
