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

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

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