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

[tobin]

On 2020-04-27 16:00, Michal Privoznik wrote:
> 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

Yeah fine with me. Thank You.

When it's a positive capability, you don't even need 
virQEMUCapsProbeQMPTCGState,
you can just add the capability to virQEMUCapsObjectTypes.