[libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line

[Yi Min Zhao]

在 2018/8/20 下午7:14, Andrea Bolognani 写道:
> On Mon, 2018-08-20 at 16:45 +0800, Yi Min Zhao wrote:
>> 在 2018/8/17 上午12:31, Andrea Bolognani 写道:
>>> On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
>>>> +    virBufferAddLit(&buf, "zpci");
>>>> +    virBufferAsprintf(&buf, ",uid=%u", dev->addr.pci.zpci->zpci_uid);
>>>> +    virBufferAsprintf(&buf, ",fid=%u", dev->addr.pci.zpci->zpci_fid);
>>>> +    virBufferAsprintf(&buf, ",target=%s", dev->alias);
>>>> +    virBufferAsprintf(&buf, ",id=zpci%u", dev->addr.pci.zpci->zpci_uid);
>>> Just making sure: is the uid a good choice for naming the zpci
>>> device? I would perhaps expect the fid to be in there as well,
>>> but since both have to be unique (right?) I guess it doesn't
>>> really matter...
>> Either uid or fid, the value must be unique. But uid is defined by user.
>> Of course, we also give the permission to define fid. But uid is more
>> appropriate.
> So which is unique: uid, fid, or the combination of the two?
> Could I have
>
>    -device zpci,uid=1,fid=1
>    -device zpci,uid=1,fid=2
>
> or would that not work? What about
>
>    -device zpci,uid=1,fid=1
>    -device zpci,uid=2,fid=1
>
> would that be okay?
Both can't work. FID must be unique and UID must be also unique.
They are independent.
>
>>>> +int
>>>> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
>>> Based on the name, this is a predicate: it should return a
>>> boolean value rather than an int.
>> 0 means it's not zpci. 1 means it's zpci. -1 means error.
> My point is that a funtion called fooIsBaz() should only ever
> check whether the foo in question is indeed a baz, without taking
> any other action such as reporting errors. Leave that to the
> caller, or give the function a different name.
I think moving to the caller is proper.
>
>>> Either way, calling a predicate should never result in an error
>>> being reported, so you need to move this check somewhere else.
>> Acutally I have found out a proper place to add this check for a long time.
>> So this is the best place to add, at least I think for now.
> qemuDomainDeviceDefValidate() looks like a reasonable entry point
> for checks such as "you specified a zPCI address but this is not
> an s390 guest" or "your configuration requires zPCI but the QEMU
> binary doesn't support that feature". Both of the cases above
> should have proper test suite coverage, by the way.
>
How about my idea mentioned above?