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

[Markus Armbruster <armbru@redhat.com>]

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@jp.fujitsu.com> writes:

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