2:16 p.m.
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>
>>> ---
>>> 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.