[libvirt PATCH v4 0/3] QEMU: support QEMU built without TCG
Rebased and resent from about a month ago. Since version 2.10, QEMU can be built without TCG. This patch adds capabillity QEMU_CAPS_TCG_DISABLED that allows libvirt to use a QEMU built without TCG. Rather than create a capability that is set whenever TCG is enabled (almost always), QEMU_CAPS_TCG_DISABLED is set only when the TCG is not available. This avoids some issues with backwards compatability. For a domain that was created using QEMU >= 2.10 with KVM, there is no information in the cached XML file that says whether or not TCG was enabled. Versions of QEMU older than 2.10 do not have a QMP interface for determining whether QEMU is available. Since QEMU_CAPS_TCG_DISABLED is set only when TCG is disabled, we do not have to do any extra work to infer an appropriate value in either of these cases. New in vesion 4: Move virQEMUCapsProbeQMPTCGState into virQEMUCapsProbeQMPDevices so that we can reuse the qom call. Rename, virQEMUCapsProbeQMPDevices to virQEMUProbeQMPTypes. All patches compile. Tobin Feldman-Fitzthum (3): add QEMU_CAPS_TCG_DISABLED and probe conditionally add virQEMUCapsGetVirtType convenience function add virQEMUCapsProbeQMPTCGState function to set QEMU_CAPS_TCG_DISABLED src/qemu/qemu_capabilities.c | 81 +++++++++++++++++++++++++++--------- src/qemu/qemu_capabilities.h | 3 ++ 2 files changed, 65 insertions(+), 19 deletions(-) -- 2.20.1 (Apple Git-117)
Only probe QEMU binary with accel=tcg if TCG is not disabled. Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG is available. Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 22 ++++++++++++++-------- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fe311048d4..c56b2d8f0e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "fsdev.multidevs", "virtio.packed", "pcie-root-port.hotplug", + "tcg-disabled", ); @@ -1014,13 +1015,16 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, virCapabilitiesAddGuestFeatureWithToggle(guest, VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT, true, false); - if (virCapabilitiesAddGuestDomain(guest, - VIR_DOMAIN_VIRT_QEMU, - NULL, - NULL, - 0, - NULL) == NULL) - goto cleanup; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) { + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_QEMU, + NULL, + NULL, + 0, + NULL) == NULL) { + goto cleanup; + } + } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { if (virCapabilitiesAddGuestDomain(guest, @@ -2295,7 +2299,8 @@ bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps, virDomainVirtType virtType) { - if (virtType == VIR_DOMAIN_VIRT_QEMU) + if (virtType == VIR_DOMAIN_VIRT_QEMU && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) return true; if (virtType == VIR_DOMAIN_VIRT_KVM && @@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, * off. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) && virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0) return -1; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 1681fc79a8..abc4ee82cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */ QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */ QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */ + QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; -- 2.20.1 (Apple Git-117)
On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:
Only probe QEMU binary with accel=tcg if TCG is not disabled. Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG is available.
Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 22 ++++++++++++++-------- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fe311048d4..c56b2d8f0e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "fsdev.multidevs", "virtio.packed", "pcie-root-port.hotplug", + "tcg-disabled", );
@@ -1014,13 +1015,16 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, virCapabilitiesAddGuestFeatureWithToggle(guest, VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT, true, false);
- if (virCapabilitiesAddGuestDomain(guest, - VIR_DOMAIN_VIRT_QEMU, - NULL, - NULL, - 0, - NULL) == NULL) - goto cleanup; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) { + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_QEMU, + NULL, + NULL, + 0, + NULL) == NULL) { + goto cleanup; + } + }
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { if (virCapabilitiesAddGuestDomain(guest, @@ -2295,7 +2299,8 @@ bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps, virDomainVirtType virtType) { - if (virtType == VIR_DOMAIN_VIRT_QEMU) + if (virtType == VIR_DOMAIN_VIRT_QEMU && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) return true;
if (virtType == VIR_DOMAIN_VIRT_KVM && @@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, * off. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) && virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0) return -1;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 1681fc79a8..abc4ee82cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */ QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */ QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */ + QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */
Actually, I think this should be a positive capability, e.g. QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do the change locally before pushing, if you agree with the change. Michal
On 2020-04-24 11:31, Michal Privoznik wrote:
On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:
Only probe QEMU binary with accel=tcg if TCG is not disabled. Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG is available.
Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 22 ++++++++++++++-------- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fe311048d4..c56b2d8f0e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "fsdev.multidevs", "virtio.packed", "pcie-root-port.hotplug", + "tcg-disabled", ); @@ -1014,13 +1015,16 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, virCapabilitiesAddGuestFeatureWithToggle(guest, VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT, true, false); - if (virCapabilitiesAddGuestDomain(guest, - VIR_DOMAIN_VIRT_QEMU, - NULL, - NULL, - 0, - NULL) == NULL) - goto cleanup; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) { + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_QEMU, + NULL, + NULL, + 0, + NULL) == NULL) { + goto cleanup; + } + } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { if (virCapabilitiesAddGuestDomain(guest, @@ -2295,7 +2299,8 @@ bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps, virDomainVirtType virtType) { - if (virtType == VIR_DOMAIN_VIRT_QEMU) + if (virtType == VIR_DOMAIN_VIRT_QEMU && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) return true; if (virtType == VIR_DOMAIN_VIRT_KVM && @@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, * off. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) && virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0) return -1; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 1681fc79a8..abc4ee82cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */ QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */ QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */ + QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */
Actually, I think this should be a positive capability, e.g. QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do the change locally before pushing, if you agree with the change.
Michal
I can see why a positive capability might be a little more intuitive, but one thing to keep in mind is that if there is a capability for the TCG being enabled, we will have to do a bit more work to make sure it is always set correctly. Older versions of QEMU don't report that the TCG is enabled, for instance, so we would need to make sure we always set TCG_ENABLED for those versions. This applies not only when probing for caps, but also when reading capabilities from XML. I think these are solvable problems (although I suspect there might be some interesting edge cases), but if we have a negative capability, we don't have to worry about any of this. We set it in the few cases where TCG is disabled and otherwise the TCG is always assumed to be active.
On 4/24/20 5:52 PM, tobin wrote:
On 2020-04-24 11:31, Michal Privoznik wrote:
On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:
Only probe QEMU binary with accel=tcg if TCG is not disabled. Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG is available.
Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 22 ++++++++++++++-------- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fe311048d4..c56b2d8f0e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "fsdev.multidevs", "virtio.packed", "pcie-root-port.hotplug", + "tcg-disabled", ); @@ -1014,13 +1015,16 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, virCapabilitiesAddGuestFeatureWithToggle(guest, VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT, true, false); - if (virCapabilitiesAddGuestDomain(guest, - VIR_DOMAIN_VIRT_QEMU, - NULL, - NULL, - 0, - NULL) == NULL) - goto cleanup; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) { + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_QEMU, + NULL, + NULL, + 0, + NULL) == NULL) { + goto cleanup; + } + } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { if (virCapabilitiesAddGuestDomain(guest, @@ -2295,7 +2299,8 @@ bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps, virDomainVirtType virtType) { - if (virtType == VIR_DOMAIN_VIRT_QEMU) + if (virtType == VIR_DOMAIN_VIRT_QEMU && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) return true; if (virtType == VIR_DOMAIN_VIRT_KVM && @@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, * off. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) && virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0) return -1; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 1681fc79a8..abc4ee82cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */ QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */ QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */ + QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */
Actually, I think this should be a positive capability, e.g. QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do the change locally before pushing, if you agree with the change.
Michal
I can see why a positive capability might be a little more intuitive, but one thing to keep in mind is that if there is a capability for the TCG being enabled, we will have to do a bit more work to make sure it is always set correctly. Older versions of QEMU don't report that the TCG is enabled, for instance, so we would need to make sure we always set TCG_ENABLED for those versions. This applies not only when probing for caps, but also when reading capabilities from XML.
That is okay, if libvirtd's or qemu's ctime changes, or any version of the two then capabilities are invalidated and reprobed. This means that introducing a new capability causes libvirt to reprobe all caps on startup. So we don't have to be that careful about old caps XMLs (note, this is different to case when reading caps XMLs on a say daemon restart when nothing changed, we have to be careful there). And as far as positive/negative boolean flag goes - in either cases we will have false positives. Say, a given QEMU binary doesn't support TCG but also the binary is old and doesn't allow TCG detection. I don't see any difference between caps reporting TCG flag set (erroneously), or TCG_DISABLED flag not set (again erroneously). Since we are not able to detect the flag for older versions, we have to guess - and that is what we are doing already in these cases - see virQEMUCapsInitQMPVersionCaps().
I think these are solvable problems (although I suspect there might be some interesting edge cases), but if we have a negative capability, we don't have to worry about any of this. We set it in the few cases where TCG is disabled and otherwise the TCG is always assumed to be active.
Again, I don't see any difference here, sorry. Michal
On 2020-04-27 04:15, Michal Privoznik wrote:
On 4/24/20 5:52 PM, tobin wrote:
On 2020-04-24 11:31, Michal Privoznik wrote:
On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:
Only probe QEMU binary with accel=tcg if TCG is not disabled. Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG is available.
Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 22 ++++++++++++++-------- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fe311048d4..c56b2d8f0e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "fsdev.multidevs", "virtio.packed", "pcie-root-port.hotplug", + "tcg-disabled", ); @@ -1014,13 +1015,16 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, virCapabilitiesAddGuestFeatureWithToggle(guest, VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT, true, false); - if (virCapabilitiesAddGuestDomain(guest, - VIR_DOMAIN_VIRT_QEMU, - NULL, - NULL, - 0, - NULL) == NULL) - goto cleanup; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) { + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_QEMU, + NULL, + NULL, + 0, + NULL) == NULL) { + goto cleanup; + } + } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { if (virCapabilitiesAddGuestDomain(guest, @@ -2295,7 +2299,8 @@ bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps, virDomainVirtType virtType) { - if (virtType == VIR_DOMAIN_VIRT_QEMU) + if (virtType == VIR_DOMAIN_VIRT_QEMU && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) return true; if (virtType == VIR_DOMAIN_VIRT_KVM && @@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, * off. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) && virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0) return -1; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 1681fc79a8..abc4ee82cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */ QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */ QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */ + QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */
Actually, I think this should be a positive capability, e.g. QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do the change locally before pushing, if you agree with the change.
Michal
I can see why a positive capability might be a little more intuitive, but one thing to keep in mind is that if there is a capability for the TCG being enabled, we will have to do a bit more work to make sure it is always set correctly. Older versions of QEMU don't report that the TCG is enabled, for instance, so we would need to make sure we always set TCG_ENABLED for those versions. This applies not only when probing for caps, but also when reading capabilities from XML.
That is okay, if libvirtd's or qemu's ctime changes, or any version of the two then capabilities are invalidated and reprobed. This means that introducing a new capability causes libvirt to reprobe all caps on startup. So we don't have to be that careful about old caps XMLs (note, this is different to case when reading caps XMLs on a say daemon restart when nothing changed, we have to be careful there).
And as far as positive/negative boolean flag goes - in either cases we will have false positives. Say, a given QEMU binary doesn't support TCG but also the binary is old and doesn't allow TCG detection. I don't see any difference between caps reporting TCG flag set (erroneously), or TCG_DISABLED flag not set (again erroneously). Since we are not able to detect the flag for older versions, we have to guess - and that is what we are doing already in these cases - see virQEMUCapsInitQMPVersionCaps().
I think these are solvable problems (although I suspect there might be some interesting edge cases), but if we have a negative capability, we don't have to worry about any of this. We set it in the few cases where TCG is disabled and otherwise the TCG is always assumed to be active.
Again, I don't see any difference here, sorry.
Michal
Ok, I have some doubts, but I have an earlier version of the patch with a positive capability. I will dig that up and send it through in the next day or two.
On 4/27/20 9:16 PM, tobin wrote:
On 2020-04-27 04:15, Michal Privoznik wrote:
On 4/24/20 5:52 PM, tobin wrote:
On 2020-04-24 11:31, Michal Privoznik wrote:
On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:
Only probe QEMU binary with accel=tcg if TCG is not disabled. Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG is available.
Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 22 ++++++++++++++-------- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fe311048d4..c56b2d8f0e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "fsdev.multidevs", "virtio.packed", "pcie-root-port.hotplug", + "tcg-disabled", ); @@ -1014,13 +1015,16 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, virCapabilitiesAddGuestFeatureWithToggle(guest, VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT, true, false); - if (virCapabilitiesAddGuestDomain(guest, - VIR_DOMAIN_VIRT_QEMU, - NULL, - NULL, - 0, - NULL) == NULL) - goto cleanup; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) { + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_QEMU, + NULL, + NULL, + 0, + NULL) == NULL) { + goto cleanup; + } + } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { if (virCapabilitiesAddGuestDomain(guest, @@ -2295,7 +2299,8 @@ bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps, virDomainVirtType virtType) { - if (virtType == VIR_DOMAIN_VIRT_QEMU) + if (virtType == VIR_DOMAIN_VIRT_QEMU && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) return true; if (virtType == VIR_DOMAIN_VIRT_KVM && @@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, * off. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) && virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0) return -1; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 1681fc79a8..abc4ee82cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */ QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */ QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */ + QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */
Actually, I think this should be a positive capability, e.g. QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do the change locally before pushing, if you agree with the change.
Michal
I can see why a positive capability might be a little more intuitive, but one thing to keep in mind is that if there is a capability for the TCG being enabled, we will have to do a bit more work to make sure it is always set correctly. Older versions of QEMU don't report that the TCG is enabled, for instance, so we would need to make sure we always set TCG_ENABLED for those versions. This applies not only when probing for caps, but also when reading capabilities from XML.
That is okay, if libvirtd's or qemu's ctime changes, or any version of the two then capabilities are invalidated and reprobed. This means that introducing a new capability causes libvirt to reprobe all caps on startup. So we don't have to be that careful about old caps XMLs (note, this is different to case when reading caps XMLs on a say daemon restart when nothing changed, we have to be careful there).
And as far as positive/negative boolean flag goes - in either cases we will have false positives. Say, a given QEMU binary doesn't support TCG but also the binary is old and doesn't allow TCG detection. I don't see any difference between caps reporting TCG flag set (erroneously), or TCG_DISABLED flag not set (again erroneously). Since we are not able to detect the flag for older versions, we have to guess - and that is what we are doing already in these cases - see virQEMUCapsInitQMPVersionCaps().
I think these are solvable problems (although I suspect there might be some interesting edge cases), but if we have a negative capability, we don't have to worry about any of this. We set it in the few cases where TCG is disabled and otherwise the TCG is always assumed to be active.
Again, I don't see any difference here, sorry.
Michal
Ok, I have some doubts, but I have an earlier version of the patch with a positive capability. I will dig that up and send it through in the next day or two.
No need. I have all the changes needed in a local branch, I will just squashed them. All I wanted was your blessing :-) Michal
On 2020-04-27 16:00, Michal Privoznik wrote:
On 4/27/20 9:16 PM, tobin wrote:
On 2020-04-27 04:15, Michal Privoznik wrote:
On 4/24/20 5:52 PM, tobin wrote:
On 2020-04-24 11:31, Michal Privoznik wrote:
On 4/22/20 11:50 PM, Tobin Feldman-Fitzthum wrote:
Only probe QEMU binary with accel=tcg if TCG is not disabled. Similarly, only add a VIR_DOMAIN_VIRT_QEMU guest if TCG is available.
Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 22 ++++++++++++++-------- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index fe311048d4..c56b2d8f0e 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -573,6 +573,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "fsdev.multidevs", "virtio.packed", "pcie-root-port.hotplug", + "tcg-disabled", ); @@ -1014,13 +1015,16 @@ virQEMUCapsInitGuestFromBinary(virCapsPtr caps, virCapabilitiesAddGuestFeatureWithToggle(guest, VIR_CAPS_GUEST_FEATURE_TYPE_DISKSNAPSHOT, true, false); - if (virCapabilitiesAddGuestDomain(guest, - VIR_DOMAIN_VIRT_QEMU, - NULL, - NULL, - 0, - NULL) == NULL) - goto cleanup; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) { + if (virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_QEMU, + NULL, + NULL, + 0, + NULL) == NULL) { + goto cleanup; + } + } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { if (virCapabilitiesAddGuestDomain(guest, @@ -2295,7 +2299,8 @@ bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps, virDomainVirtType virtType) { - if (virtType == VIR_DOMAIN_VIRT_QEMU) + if (virtType == VIR_DOMAIN_VIRT_QEMU && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) return true; if (virtType == VIR_DOMAIN_VIRT_KVM && @@ -5150,6 +5155,7 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, * off. */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM) && + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED) && virQEMUCapsInitQMPSingle(qemuCaps, libDir, runUid, runGid, true) < 0) return -1; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 1681fc79a8..abc4ee82cb 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -554,6 +554,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_FSDEV_MULTIDEVS, /* fsdev.multidevs */ QEMU_CAPS_VIRTIO_PACKED_QUEUES, /* virtio.packed */ QEMU_CAPS_PCIE_ROOT_PORT_HOTPLUG, /* pcie-root-port.hotplug */ + QEMU_CAPS_TCG_DISABLED, /* QEMU does not support TCG */
Actually, I think this should be a positive capability, e.g. QEMU_CAPS_TCG to reflect whether QEMU supports TCG or not. I can do the change locally before pushing, if you agree with the change.
Michal
I can see why a positive capability might be a little more intuitive, but one thing to keep in mind is that if there is a capability for the TCG being enabled, we will have to do a bit more work to make sure it is always set correctly. Older versions of QEMU don't report that the TCG is enabled, for instance, so we would need to make sure we always set TCG_ENABLED for those versions. This applies not only when probing for caps, but also when reading capabilities from XML.
That is okay, if libvirtd's or qemu's ctime changes, or any version of the two then capabilities are invalidated and reprobed. This means that introducing a new capability causes libvirt to reprobe all caps on startup. So we don't have to be that careful about old caps XMLs (note, this is different to case when reading caps XMLs on a say daemon restart when nothing changed, we have to be careful there).
And as far as positive/negative boolean flag goes - in either cases we will have false positives. Say, a given QEMU binary doesn't support TCG but also the binary is old and doesn't allow TCG detection. I don't see any difference between caps reporting TCG flag set (erroneously), or TCG_DISABLED flag not set (again erroneously). Since we are not able to detect the flag for older versions, we have to guess - and that is what we are doing already in these cases - see virQEMUCapsInitQMPVersionCaps().
I think these are solvable problems (although I suspect there might be some interesting edge cases), but if we have a negative capability, we don't have to worry about any of this. We set it in the few cases where TCG is disabled and otherwise the TCG is always assumed to be active.
Again, I don't see any difference here, sorry.
Michal
Ok, I have some doubts, but I have an earlier version of the patch with a positive capability. I will dig that up and send it through in the next day or two.
No need. I have all the changes needed in a local branch, I will just squashed them. All I wanted was your blessing :-)
Michal
Yeah fine with me. Thank You. When it's a positive capability, you don't even need virQEMUCapsProbeQMPTCGState, you can just add the capability to virQEMUCapsObjectTypes.
On 4/27/20 10:22 PM, tobin wrote:
Yeah fine with me. Thank You.
When it's a positive capability, you don't even need virQEMUCapsProbeQMPTCGState, you can just add the capability to virQEMUCapsObjectTypes.
Yep. I've went with that. This is now pushed. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Congratulations on your first libvirt contribution! Michal
On a Tuesday in 2020, Michal Privoznik wrote:
On 4/27/20 10:22 PM, tobin wrote:
Yeah fine with me. Thank You.
When it's a positive capability, you don't even need virQEMUCapsProbeQMPTCGState, you can just add the capability to virQEMUCapsObjectTypes.
Yep. I've went with that. This is now pushed.
Umm, how do you know then if the capability is not missing because the QEMU is too old to support it? Jano
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Congratulations on your first libvirt contribution!
Michal
On Tue, Apr 28, 2020 at 13:13:32 +0200, Ján Tomko wrote:
On a Tuesday in 2020, Michal Privoznik wrote:
On 4/27/20 10:22 PM, tobin wrote:
Yeah fine with me. Thank You.
When it's a positive capability, you don't even need virQEMUCapsProbeQMPTCGState, you can just add the capability to virQEMUCapsObjectTypes.
Yep. I've went with that. This is now pushed.
Umm, how do you know then if the capability is not missing because the QEMU is too old to support it?
Yeah, when inverting it, the capability should be assumed by a version check (yuck) with old qemu.
On 4/28/20 1:19 PM, Peter Krempa wrote:
On Tue, Apr 28, 2020 at 13:13:32 +0200, Ján Tomko wrote:
On a Tuesday in 2020, Michal Privoznik wrote:
On 4/27/20 10:22 PM, tobin wrote:
Yeah fine with me. Thank You.
When it's a positive capability, you don't even need virQEMUCapsProbeQMPTCGState, you can just add the capability to virQEMUCapsObjectTypes.
Yep. I've went with that. This is now pushed.
Umm, how do you know then if the capability is not missing because the QEMU is too old to support it?
Yeah, when inverting it, the capability should be assumed by a version check (yuck) with old qemu.
Yeah, the qemu commit in question is v2.10.0-rc0~93^2~18 and before that it wasn't possible to compile out TCG. And what do you mean "too old to support TCG"? Isn't TCG how QEMU started (with adaptation to KVM happening later)? The oldest mention of TCG that I bothered to find is in v0.14.0-rc0-936-g303d4e865b which is way older than current minimal 1.5.0 so I think we are safe, aren't we? Or is there something that I am missing? Michal
On Tue, Apr 28, 2020 at 15:27:18 +0200, Michal Privoznik wrote:
On 4/28/20 1:19 PM, Peter Krempa wrote:
On Tue, Apr 28, 2020 at 13:13:32 +0200, Ján Tomko wrote:
On a Tuesday in 2020, Michal Privoznik wrote:
On 4/27/20 10:22 PM, tobin wrote:
Yeah fine with me. Thank You.
When it's a positive capability, you don't even need virQEMUCapsProbeQMPTCGState, you can just add the capability to virQEMUCapsObjectTypes.
Yep. I've went with that. This is now pushed.
Umm, how do you know then if the capability is not missing because the QEMU is too old to support it?
Yeah, when inverting it, the capability should be assumed by a version check (yuck) with old qemu.
Yeah, the qemu commit in question is v2.10.0-rc0~93^2~18 and before that it wasn't possible to compile out TCG. And what do you mean "too old to support TCG"? Isn't TCG how QEMU started (with adaptation to KVM happening later)? The oldest mention of TCG that I bothered to find is in v0.14.0-rc0-936-g303d4e865b which is way older than current minimal 1.5.0 so I think we are safe, aren't we? Or is there something that I am missing?
No, this is the artifact of you taking patches and modifying them and me not checking the pushed code. You correctly added that prior to 2.10 the capability is assumed. I was refering to the need to have such a thing if you want to do a positive capability to prevent regressing with old qemus.
Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 25 ++++++++++++++++--------- src/qemu/qemu_capabilities.h | 2 ++ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c56b2d8f0e..e7179ea048 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4984,6 +4984,20 @@ virQEMUCapsProbeQMPSchemaCapabilities(virQEMUCapsPtr qemuCaps, #define QEMU_MIN_MINOR 5 #define QEMU_MIN_MICRO 0 +virDomainVirtType +virQEMUCapsGetVirtType(virQEMUCapsPtr qemuCaps) +{ + virDomainVirtType type; + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) + type = VIR_DOMAIN_VIRT_KVM; + else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED)) + type = VIR_DOMAIN_VIRT_QEMU; + else + type = VIR_DOMAIN_VIRT_NONE; + + return type; +} + int virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon) @@ -5028,11 +5042,7 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (virQEMUCapsProbeQMPKVMState(qemuCaps, mon) < 0) return -1; - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) - type = VIR_DOMAIN_VIRT_KVM; - else - type = VIR_DOMAIN_VIRT_QEMU; - + type = virQEMUCapsGetVirtType(qemuCaps); accel = virQEMUCapsGetAccel(qemuCaps, type); if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0) @@ -5525,10 +5535,7 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache, goto cleanup; } - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) - capsType = VIR_DOMAIN_VIRT_KVM; - else - capsType = VIR_DOMAIN_VIRT_QEMU; + capsType = virQEMUCapsGetVirtType(qemuCaps); if (virttype == VIR_DOMAIN_VIRT_NONE) virttype = capsType; diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index abc4ee82cb..9a779912fe 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -636,6 +636,8 @@ int virQEMUCapsGetCPUFeatures(virQEMUCapsPtr qemuCaps, bool migratable, char ***features); +virDomainVirtType virQEMUCapsGetVirtType(virQEMUCapsPtr qemuCaps); + bool virQEMUCapsIsArchSupported(virQEMUCapsPtr qemuCaps, virArch arch); bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps, -- 2.20.1 (Apple Git-117)
Add virQEMUCapsProbeQMPTCGState to set TCG_DISABLED cap if version is > 2.10, KVM is enabled, and tcg-accel is not present in qom-list-types result. Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e7179ea048..4a3170fc5c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2576,7 +2576,35 @@ virQEMUCapsProbeQMPGenericProps(virQEMUCapsPtr qemuCaps, } static int -virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps, +virQEMUCapsProbeQMPTCGState(virQEMUCapsPtr qemuCaps, + char **values, + int nvalues) +{ + size_t i; + bool found = false; + /* + * As of version 2.10, QEMU can be built without the TCG. + * */ + if (qemuCaps->version < 2010000) + return 0; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) + return 0; + + for (i = 0; i < nvalues; i++) { + if (STREQ(values[i], "tcg-accel")) { + found = true; + break; + } + } + + if (!found) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_TCG_DISABLED); + + return 0; +} + +static int +virQEMUCapsProbeQMPTypes(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon) { int nvalues; @@ -2584,6 +2612,8 @@ virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps, if ((nvalues = qemuMonitorGetObjectTypes(mon, &values)) < 0) return -1; + + virQEMUCapsProbeQMPTCGState(qemuCaps, values, nvalues); virQEMUCapsProcessStringFlags(qemuCaps, G_N_ELEMENTS(virQEMUCapsObjectTypes), virQEMUCapsObjectTypes, @@ -5047,7 +5077,7 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0) return -1; - if (virQEMUCapsProbeQMPDevices(qemuCaps, mon) < 0) + if (virQEMUCapsProbeQMPTypes(qemuCaps, mon) < 0) return -1; if (virQEMUCapsProbeQMPMachineTypes(qemuCaps, type, mon) < 0) return -1; -- 2.20.1 (Apple Git-117)
On Wed, Apr 22, 2020 at 17:50:44 -0400, Tobin Feldman-Fitzthum wrote:
Add virQEMUCapsProbeQMPTCGState to set TCG_DISABLED cap if version is > 2.10, KVM is enabled, and tcg-accel is not present in qom-list-types result.
Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)
I don't feel comfortable reviewing the other two patches which deal with the actual use of the capability, so I'll leave those for somebody else.
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e7179ea048..4a3170fc5c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2576,7 +2576,35 @@ virQEMUCapsProbeQMPGenericProps(virQEMUCapsPtr qemuCaps, }
static int -virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps, +virQEMUCapsProbeQMPTCGState(virQEMUCapsPtr qemuCaps,
Returned value of the new function is not used, so please define it as 'void'
+ char **values, + int nvalues) +{ + size_t i; + bool found = false; + /* + * As of version 2.10, QEMU can be built without the TCG. + * */ + if (qemuCaps->version < 2010000) + return 0; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) + return 0; + + for (i = 0; i < nvalues; i++) { + if (STREQ(values[i], "tcg-accel")) { + found = true;
You can set the capability here without intermediate variable and just return.
+ break; + } + } + + if (!found) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_TCG_DISABLED); + + return 0; +} + +static int +virQEMUCapsProbeQMPTypes(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon) { int nvalues;
With the above resolved: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 4/24/20 9:58 AM, Peter Krempa wrote:
On Wed, Apr 22, 2020 at 17:50:44 -0400, Tobin Feldman-Fitzthum wrote:
Add virQEMUCapsProbeQMPTCGState to set TCG_DISABLED cap if version is > 2.10, KVM is enabled, and tcg-accel is not present in qom-list-types result.
Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 34 ++++++++++++++++++++++++++++++++-- 1 file changed, 32 insertions(+), 2 deletions(-)
I don't feel comfortable reviewing the other two patches which deal with the actual use of the capability, so I'll leave those for somebody else.
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e7179ea048..4a3170fc5c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2576,7 +2576,35 @@ virQEMUCapsProbeQMPGenericProps(virQEMUCapsPtr qemuCaps, }
static int -virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps, +virQEMUCapsProbeQMPTCGState(virQEMUCapsPtr qemuCaps,
Returned value of the new function is not used, so please define it as 'void'
+ char **values, + int nvalues) +{ + size_t i; + bool found = false; + /* + * As of version 2.10, QEMU can be built without the TCG. + * */ + if (qemuCaps->version < 2010000) + return 0; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) + return 0; + + for (i = 0; i < nvalues; i++) { + if (STREQ(values[i], "tcg-accel")) { + found = true;
You can set the capability here without intermediate variable and just return.
I'm not quite sure what you mean. The capability tracks lack of tcg-accel. So what can be done here is return and drop the check before setting the capability below.
+ break; + } + } + + if (!found) + virQEMUCapsSet(qemuCaps, QEMU_CAPS_TCG_DISABLED); + + return 0; +} + +static int +virQEMUCapsProbeQMPTypes(virQEMUCapsPtr qemuCaps, qemuMonitorPtr mon) { int nvalues;
With the above resolved:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Michal
participants (5)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa -
tobin -
Tobin Feldman-Fitzthum