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

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(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. No need. I have all the changes needed in a local branch, I will just squashed them. All I wanted was your blessing :-) Michal