[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH] device-assignment: Rework "name" of assigned pc
From: |
Markus Armbruster |
Subject: |
[Qemu-devel] Re: [PATCH] device-assignment: Rework "name" of assigned pci device |
Date: |
Tue, 29 Jun 2010 07:28:26 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
Hidetoshi Seto <address@hidden> writes:
> "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".
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 <address@hidden>
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.