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

[Michal Privoznik]

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 at linux.vnet.ibm.com>
>>>>> ---
>>>>>   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