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

[tobin]

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.