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

[tobin <tobin@xxxxxxxxxxxxxxxxxx>]

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