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@xxxxxxxxxxxxxx> writes: > Thanks Markus, > > (2010/06/29 14:28), Markus Armbruster wrote: >> 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. > > 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=') -- 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
