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

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(a)linux.vnet.ibm.com&gt; >>>> --- >>>>   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.