Re: [libvirt] [PATCH v2 RESEND 09/12] qemu: Generate and use zPCI device in QEMU command line
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. [...] > +char *qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev); > + > +bool qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info); Is this really necessary? Can't these two functions be static? [...] > --- 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. > +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. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 RESEND 09/12] qemu: Generate and use zPCI device in QEMU command line
在 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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2 RESEND 09/12] qemu: Generate and use zPCI device in QEMU command line
On Fri, 2018-07-27 at 15:53 +0800, Yi Min Zhao wrote: > 在 2018/7/24 下午11:09, Andrea Bolognani 写道: > > 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. Yeah, you definitely want to report an error if the QEMU binary doesn't support zPCI or the user is attempting to do something silly like add zPCI-related information to devices attached to an x86_64 guest... > > > +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. Right, I overlooked that :) That said, they aren't used in the hotplug code until the next patch, so it would IMHO make sense to define them as static in this patch and export them in the next one, at the same time as you actually start using them outside of the file. [...] > > > +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. I forgot to mention, please have a single capability per line and make sure the set of capabilities used in xml2xml and xml2argv is exactly the same. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list