Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
在 2018/8/22 下午4:42, Andrea Bolognani 写道: On Wed, 2018-08-22 at 11:06 +0800, Yi Min Zhao wrote: 在 2018/8/20 下午7:14, Andrea Bolognani 写道: 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. Got it, thanks. 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. 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? You mean moving the check to the caller? Yes. I think qemuDomainDeviceDefValidate() is a better choice because it should allow you to implement the checks once, potentially with more context such as information about the domain and QEMU capabilities, and have them performed regardless of whether the device is being processed eg. during regular parsing or hotplug. IIUC, in qemuDomainDeviceDefValidate(), we have to directly access device_info from DeviceDef to check zpci. I think direct accessing is insecure and we have to process those device types that has device_info. In addition, these is a special case that address is not defined and pci address type will be chosen by default. I found there are a lot of caps check code while building command line. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
On Wed, 2018-08-22 at 11:06 +0800, Yi Min Zhao wrote: > 在 2018/8/20 下午7:14, Andrea Bolognani 写道: > > 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. Got it, thanks. > > 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. > > > 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? You mean moving the check to the caller? I think qemuDomainDeviceDefValidate() is a better choice because it should allow you to implement the checks once, potentially with more context such as information about the domain and QEMU capabilities, and have them performed regardless of whether the device is being processed eg. during regular parsing or hotplug. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
在 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? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
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? > > > +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. > > 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. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
在 2018/8/17 上午12:31, Andrea Bolognani 写道:
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
+char *
+qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
+{
+virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+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.
+
+if (virBufferCheckError(&buf) < 0) {
+virBufferFreeAndReset(&buf);
+return NULL;
+}
+
+return virBufferContentAndReset(&buf);
+}
+
+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.
+{
+if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
+info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
+return 1;
+}
+
+if (info->addr.pci.zpci) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("This QEMU doesn't support zpci devices"));
+return -1;
+}
Not sure about this second check. I guess the logic is supposed to
be that, when passed a "proper" zPCI device, the function would
have returned with the first check, and if we find a zPCI address
after that it's an error. Makes sense, but it's kinda obfuscated
and I'm not sure the error message is accurate: can it really only
be a problem of the QEMU binary not supporting the zPCI feature,
or could we have gotten here because the user is trying to assing
zPCI addresses to devices that don't support them?
Yes, it's because user could define zpci address in xml but
there's no zpci support.
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.
[...]
+++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
@@ -0,0 +1,18 @@
+
+ QEMUGuest1
+ c7a5fdbd-edaf-9455-926a-d65c16db1809
+ 219100
+
+hvm
+
+
+/usr/bin/qemu-system-s390x
+
+
+
+
+
+
+
+
+
This test case would have been a great addition to the previous
commit, where you actually introduced the ability to automatically
allocate zPCI addresses...
Another thing that I forgot to ask earlier and this is as good a
time as any: once zPCI support has been merged, will users have
to opt-in to it using
or will they get zPCI devices by default? And if so, will it still
be possible for them to get CCW devices instead if wanted?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
On Fri, 2018-08-17 at 08:35 +0200, Boris Fiuczynski wrote: > On 08/16/2018 06:31 PM, Andrea Bolognani wrote: > > Another thing that I forgot to ask earlier and this is as good a > > time as any: once zPCI support has been merged, will users have > > to opt-in to it using > > > > > > > > or will they get zPCI devices by default? And if so, will it still > > be possible for them to get CCW devices instead if wanted? > > On S390 the default behavior is to assign CCW addresses. This has not > and is not going to be changed. Therefore a user has to explicitly > specify a PCI address or in your words: zPCI is an opt-in. Sounds good to me! Just making sure :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
On 08/16/2018 06:31 PM, Andrea Bolognani wrote: Another thing that I forgot to ask earlier and this is as good a time as any: once zPCI support has been merged, will users have to opt-in to it using or will they get zPCI devices by default? And if so, will it still be possible for them to get CCW devices instead if wanted? On S390 the default behavior is to assign CCW addresses. This has not and is not going to be changed. Therefore a user has to explicitly specify a PCI address or in your words: zPCI is an opt-in. -- Mit freundlichen Grüßen/Kind regards Boris Fiuczynski IBM Deutschland Research & Development GmbH Vorsitzender des Aufsichtsrats: Martina Köderitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3 09/12] qemu: Generate and use zPCI device in QEMU command line
On Tue, 2018-08-07 at 17:10 +0800, Yi Min Zhao wrote:
[...]
> +char *
> +qemuBuildZPCIDevStr(virDomainDeviceInfoPtr dev)
> +{
> +virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +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...
> +
> +if (virBufferCheckError(&buf) < 0) {
> +virBufferFreeAndReset(&buf);
> +return NULL;
> +}
> +
> +return virBufferContentAndReset(&buf);
> +}
> +
> +int
> +qemuCheckDeviceIsZPCI(virDomainDeviceInfoPtr info)
Based on the name, this is a predicate: it should return a
boolean value rather than an int.
> +{
> +if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI &&
> +info->pciAddressExtFlags & VIR_PCI_ADDRESS_EXTENSION_ZPCI) {
> +return 1;
> +}
> +
> +if (info->addr.pci.zpci) {
> +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("This QEMU doesn't support zpci devices"));
> +return -1;
> +}
Not sure about this second check. I guess the logic is supposed to
be that, when passed a "proper" zPCI device, the function would
have returned with the first check, and if we find a zPCI address
after that it's an error. Makes sense, but it's kinda obfuscated
and I'm not sure the error message is accurate: can it really only
be a problem of the QEMU binary not supporting the zPCI feature,
or could we have gotten here because the user is trying to assing
zPCI addresses to devices that don't support them?
Either way, calling a predicate should never result in an error
being reported, so you need to move this check somewhere else.
[...]
> +++ b/tests/qemuxml2argvdata/hostdev-vfio-zpci-autogenerate.xml
> @@ -0,0 +1,18 @@
> +
> + QEMUGuest1
> + c7a5fdbd-edaf-9455-926a-d65c16db1809
> + 219100
> +
> +hvm
> +
> +
> +/usr/bin/qemu-system-s390x
> +
> +
> +
> +
> +
> +
> +
> +
> +
This test case would have been a great addition to the previous
commit, where you actually introduced the ability to automatically
allocate zPCI addresses...
Another thing that I forgot to ask earlier and this is as good a
time as any: once zPCI support has been merged, will users have
to opt-in to it using
or will they get zPCI devices by default? And if so, will it still
be possible for them to get CCW devices instead if wanted?
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
