[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: |
Wed, 30 Jun 2010 08:53:53 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) |
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 <address@hidden> writes:
> Thanks Markus,
>
> (2010/06/29 14:28), Markus Armbruster wrote:
>> 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.
>
> 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=')