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@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 */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
--
2.20.1 (Apple Git-117)
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@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
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@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. 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.
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@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
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@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.
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@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
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@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.
On 4/27/20 10:22 PM, tobin wrote: > 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. > Yep. I've went with that. This is now pushed. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Congratulations on your first libvirt contribution! Michal
On a Tuesday in 2020, Michal Privoznik wrote: >On 4/27/20 10:22 PM, tobin wrote: > >>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. >> > >Yep. I've went with that. This is now pushed. > Umm, how do you know then if the capability is not missing because the QEMU is too old to support it? Jano >Reviewed-by: Michal Privoznik <mprivozn@redhat.com> > >Congratulations on your first libvirt contribution! > >Michal >
On Tue, Apr 28, 2020 at 13:13:32 +0200, Ján Tomko wrote: > On a Tuesday in 2020, Michal Privoznik wrote: > > On 4/27/20 10:22 PM, tobin wrote: > > > > > 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. > > > > > > > Yep. I've went with that. This is now pushed. > > > > Umm, how do you know then if the capability is not missing because the > QEMU is too old to support it? Yeah, when inverting it, the capability should be assumed by a version check (yuck) with old qemu.
On 4/28/20 1:19 PM, Peter Krempa wrote: > On Tue, Apr 28, 2020 at 13:13:32 +0200, Ján Tomko wrote: >> On a Tuesday in 2020, Michal Privoznik wrote: >>> On 4/27/20 10:22 PM, tobin wrote: >>> >>>> 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. >>>> >>> >>> Yep. I've went with that. This is now pushed. >>> >> >> Umm, how do you know then if the capability is not missing because the >> QEMU is too old to support it? > > Yeah, when inverting it, the capability should be assumed by a version > check (yuck) with old qemu. > Yeah, the qemu commit in question is v2.10.0-rc0~93^2~18 and before that it wasn't possible to compile out TCG. And what do you mean "too old to support TCG"? Isn't TCG how QEMU started (with adaptation to KVM happening later)? The oldest mention of TCG that I bothered to find is in v0.14.0-rc0-936-g303d4e865b which is way older than current minimal 1.5.0 so I think we are safe, aren't we? Or is there something that I am missing? Michal
On Tue, Apr 28, 2020 at 15:27:18 +0200, Michal Privoznik wrote: > On 4/28/20 1:19 PM, Peter Krempa wrote: > > On Tue, Apr 28, 2020 at 13:13:32 +0200, Ján Tomko wrote: > > > On a Tuesday in 2020, Michal Privoznik wrote: > > > > On 4/27/20 10:22 PM, tobin wrote: > > > > > > > > > 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. > > > > > > > > > > > > > Yep. I've went with that. This is now pushed. > > > > > > > > > > Umm, how do you know then if the capability is not missing because the > > > QEMU is too old to support it? > > > > Yeah, when inverting it, the capability should be assumed by a version > > check (yuck) with old qemu. > > > > Yeah, the qemu commit in question is v2.10.0-rc0~93^2~18 and before that it > wasn't possible to compile out TCG. And what do you mean "too old to support > TCG"? Isn't TCG how QEMU started (with adaptation to KVM happening later)? > The oldest mention of TCG that I bothered to find is in > v0.14.0-rc0-936-g303d4e865b which is way older than current minimal 1.5.0 so > I think we are safe, aren't we? Or is there something that I am missing? No, this is the artifact of you taking patches and modifying them and me not checking the pushed code. You correctly added that prior to 2.10 the capability is assumed. I was refering to the need to have such a thing if you want to do a positive capability to prevent regressing with old qemus.
© 2016 - 2026 Red Hat, Inc.