Re: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally

tobin <tobin@xxxxxxxxxxxxxxxxxx> · Mon, 27 Apr 2020 16:22:39 -0400

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@xxxxxxxxxxxxxxxxxx>
---
  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.









