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

[Yi Min Zhao]

在 2018/7/24 下午11:09, Andrea Bolognani 写道:
> On Tue, 2018-07-10 at 16:02 +0800, Yi Min Zhao wrote:
> [...]
>> +bool
>> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
>> +{
>> +    if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
>> +        ((info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) ||
>> +         info->addr.pci.zpci))
>> +        return true;
> Missing curly braces.
>
> Also, do you really need to check both the flags and the presence
> of the zPCI address bits? It looks like either one or the other
> should be enough or, if that's not the case, it should be made so
> because having to check for two separate conditions makes me feel
> like it would introduce bugs in the long run.
This is actually a problem. I add info->addr.pci.zpci check in order to 
check
if the user specifies zpci address in xml even though it has no zpci 
support.
The callers of this function checks zpci capability. If no zpci cap but 
the user
specfies zpci address, report an error.

I will change the logic and remove the check on info->addr.pci.zpci.
>
> [...]
>> +char *qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev);
>> +
>> +bool qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info);
> Is this really necessary? Can't these two functions be static?
They are also used in qemu_hotplug.c file.
>
> [...]
>> --- a/tests/qemuxml2argvtest.c
>> +++ b/tests/qemuxml2argvtest.c
>> @@ -1014,6 +1014,8 @@ mymain(void)
>>               QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
>>       DO_TEST("disk-virtio-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI,
>>               QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
>> +    DO_TEST("disk-virtio-s390-zpci", QEMU_CAPS_DEVICE_ZPCI,
>> +            QEMU_CAPS_CCW, QEMU_CAPS_VIRTIO_S390);
>>       DO_TEST("disk-order",
>>               QEMU_CAPS_DRIVE_BOOT, QEMU_CAPS_VIRTIO_BLK_SCSI);
>>       DO_TEST("disk-virtio-drive-queues",
>> @@ -1574,6 +1576,18 @@ mymain(void)
>>               QEMU_CAPS_DEVICE_VFIO_PCI);
>>       DO_TEST_PARSE_ERROR("hostdev-mdev-invalid-target-address",
>>               QEMU_CAPS_DEVICE_VFIO_PCI);
>> +    DO_TEST("hostdev-vfio-zpci",
>> +            QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI);
>> +    DO_TEST("hostdev-vfio-zpci-multidomain-many",
>> +            QEMU_CAPS_DEVICE_VFIO_PCI, X_QEMU_CAPS_HOST_PCI_MULTIDOMAIN,
> Capabilities with X_QEMU prefix are no longer used, so you should
> not list them here.
Sure.
>
>> +            QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_ZPCI);
>> +    DO_TEST("hostdev-vfio-zpci-autogenerate",
>> +            QEMU_CAPS_DEVICE_VFIO_PCI, QEMU_CAPS_DEVICE_ZPCI);
>> +    DO_TEST("hostdev-vfio-zpci-boundaries",
>> +            QEMU_CAPS_DEVICE_VFIO_PCI, X_QEMU_CAPS_HOST_PCI_MULTIDOMAIN,
>> +            QEMU_CAPS_DEVICE_PCI_BRIDGE, QEMU_CAPS_DEVICE_ZPCI);
>> +    DO_TEST_FAILURE("hostdev-vfio-zpci",
>> +                    QEMU_CAPS_DEVICE_VFIO_PCI);
> Please add these to qemuxml2xmltest at the same time.
>