4:50 p.m.
Rebased and resent from about a month ago.
Since version 2.10, QEMU can be built without TCG. This patch
adds capabillity QEMU_CAPS_TCG_DISABLED that allows libvirt
to use a QEMU built without TCG.
Rather than create a capability that is set whenever TCG is
enabled (almost always), QEMU_CAPS_TCG_DISABLED is set only
when the TCG is not available. This avoids some issues
with backwards compatability.
For a domain that was created using QEMU >= 2.10 with KVM,
there is no information in the cached XML file that says
whether or not TCG was enabled. Versions of QEMU older
than 2.10 do not have a QMP interface for determining
whether QEMU is available. Since QEMU_CAPS_TCG_DISABLED
is set only when TCG is disabled, we do not have to do any
extra work to infer an appropriate value in either of these
cases.
New in vesion 4:
Move virQEMUCapsProbeQMPTCGState into
virQEMUCapsProbeQMPDevices so that we can reuse the qom call.
Rename, virQEMUCapsProbeQMPDevices to virQEMUProbeQMPTypes.
All patches compile.
Tobin Feldman-Fitzthum (3):
add QEMU_CAPS_TCG_DISABLED and probe conditionally
add virQEMUCapsGetVirtType convenience function
add virQEMUCapsProbeQMPTCGState function to set QEMU_CAPS_TCG_DISABLED
src/qemu/qemu_capabilities.c | 81 +++++++++++++++++++++++++++---------
src/qemu/qemu_capabilities.h | 3 ++
2 files changed, 65 insertions(+), 19 deletions(-)
--
2.20.1 (Apple Git-117)
4:50 p.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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 */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
--
2.20.1 (Apple Git-117)
10:31 a.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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
10:52 a.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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. 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.
3:15 a.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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
2:16 p.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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.
3 p.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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(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.
No need. I have all the changes needed in a local branch, I will just
squashed them. All I wanted was your blessing :-)
Michal
3:22 p.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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(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.
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.
4:25 a.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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(a)redhat.com>
Congratulations on your first libvirt contribution!
Michal
6:13 a.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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(a)redhat.com>
Congratulations on your first libvirt contribution!
Michal
6:19 a.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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.
8:27 a.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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
8:37 a.m.
New subject: [libvirt PATCH v4 1/3] add QEMU_CAPS_TCG_DISABLED and probe conditionally
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.
4:50 p.m.
New subject: [libvirt PATCH v4 2/3] add virQEMUCapsGetVirtType convenience function
Signed-off-by: Tobin Feldman-Fitzthum <tobin(a)linux.vnet.ibm.com>
---
src/qemu/qemu_capabilities.c | 25 ++++++++++++++++---------
src/qemu/qemu_capabilities.h | 2 ++
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index c56b2d8f0e..e7179ea048 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4984,6 +4984,20 @@ virQEMUCapsProbeQMPSchemaCapabilities(virQEMUCapsPtr qemuCaps,
#define QEMU_MIN_MINOR 5
#define QEMU_MIN_MICRO 0
+virDomainVirtType
+virQEMUCapsGetVirtType(virQEMUCapsPtr qemuCaps)
+{
+ virDomainVirtType type;
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
+ type = VIR_DOMAIN_VIRT_KVM;
+ else if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_TCG_DISABLED))
+ type = VIR_DOMAIN_VIRT_QEMU;
+ else
+ type = VIR_DOMAIN_VIRT_NONE;
+
+ return type;
+}
+
int
virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
qemuMonitorPtr mon)
@@ -5028,11 +5042,7 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
if (virQEMUCapsProbeQMPKVMState(qemuCaps, mon) < 0)
return -1;
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
- type = VIR_DOMAIN_VIRT_KVM;
- else
- type = VIR_DOMAIN_VIRT_QEMU;
-
+ type = virQEMUCapsGetVirtType(qemuCaps);
accel = virQEMUCapsGetAccel(qemuCaps, type);
if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0)
@@ -5525,10 +5535,7 @@ virQEMUCapsCacheLookupDefault(virFileCachePtr cache,
goto cleanup;
}
- if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
- capsType = VIR_DOMAIN_VIRT_KVM;
- else
- capsType = VIR_DOMAIN_VIRT_QEMU;
+ capsType = virQEMUCapsGetVirtType(qemuCaps);
if (virttype == VIR_DOMAIN_VIRT_NONE)
virttype = capsType;
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index abc4ee82cb..9a779912fe 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -636,6 +636,8 @@ int virQEMUCapsGetCPUFeatures(virQEMUCapsPtr qemuCaps,
bool migratable,
char ***features);
+virDomainVirtType virQEMUCapsGetVirtType(virQEMUCapsPtr qemuCaps);
+
bool virQEMUCapsIsArchSupported(virQEMUCapsPtr qemuCaps,
virArch arch);
bool virQEMUCapsIsVirtTypeSupported(virQEMUCapsPtr qemuCaps,
--
2.20.1 (Apple Git-117)
4:50 p.m.
New subject: [libvirt PATCH v4 3/3] add virQEMUCapsProbeQMPTCGState function to set QEMU_CAPS_TCG_DISABLED
Add virQEMUCapsProbeQMPTCGState to set TCG_DISABLED cap if version
is > 2.10, KVM is enabled, and tcg-accel is not present in
qom-list-types result.
Signed-off-by: Tobin Feldman-Fitzthum <tobin(a)linux.vnet.ibm.com>
---
src/qemu/qemu_capabilities.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index e7179ea048..4a3170fc5c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2576,7 +2576,35 @@ virQEMUCapsProbeQMPGenericProps(virQEMUCapsPtr qemuCaps,
}
static int
-virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps,
+virQEMUCapsProbeQMPTCGState(virQEMUCapsPtr qemuCaps,
+ char **values,
+ int nvalues)
+{
+ size_t i;
+ bool found = false;
+ /*
+ * As of version 2.10, QEMU can be built without the TCG.
+ * */
+ if (qemuCaps->version < 2010000)
+ return 0;
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
+ return 0;
+
+ for (i = 0; i < nvalues; i++) {
+ if (STREQ(values[i], "tcg-accel")) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_TCG_DISABLED);
+
+ return 0;
+}
+
+static int
+virQEMUCapsProbeQMPTypes(virQEMUCapsPtr qemuCaps,
qemuMonitorPtr mon)
{
int nvalues;
@@ -2584,6 +2612,8 @@ virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps,
if ((nvalues = qemuMonitorGetObjectTypes(mon, &values)) < 0)
return -1;
+
+ virQEMUCapsProbeQMPTCGState(qemuCaps, values, nvalues);
virQEMUCapsProcessStringFlags(qemuCaps,
G_N_ELEMENTS(virQEMUCapsObjectTypes),
virQEMUCapsObjectTypes,
@@ -5047,7 +5077,7 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0)
return -1;
- if (virQEMUCapsProbeQMPDevices(qemuCaps, mon) < 0)
+ if (virQEMUCapsProbeQMPTypes(qemuCaps, mon) < 0)
return -1;
if (virQEMUCapsProbeQMPMachineTypes(qemuCaps, type, mon) < 0)
return -1;
--
2.20.1 (Apple Git-117)
2:58 a.m.
New subject: [libvirt PATCH v4 3/3] add virQEMUCapsProbeQMPTCGState function to set QEMU_CAPS_TCG_DISABLED
On Wed, Apr 22, 2020 at 17:50:44 -0400, Tobin Feldman-Fitzthum wrote:
Add virQEMUCapsProbeQMPTCGState to set TCG_DISABLED cap if version
is > 2.10, KVM is enabled, and tcg-accel is not present in
qom-list-types result.
Signed-off-by: Tobin Feldman-Fitzthum <tobin(a)linux.vnet.ibm.com>
---
src/qemu/qemu_capabilities.c | 34 ++++++++++++++++++++++++++++++++--
1 file changed, 32 insertions(+), 2 deletions(-)
I don't feel comfortable reviewing the other two patches which deal with
the actual use of the capability, so I'll leave those for somebody else.
diff --git a/src/qemu/qemu_capabilities.c
b/src/qemu/qemu_capabilities.c
index e7179ea048..4a3170fc5c 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2576,7 +2576,35 @@ virQEMUCapsProbeQMPGenericProps(virQEMUCapsPtr qemuCaps,
}
static int
-virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps,
+virQEMUCapsProbeQMPTCGState(virQEMUCapsPtr qemuCaps,
Returned value of the new function is not used, so please define it as
'void'
+ char **values,
+ int nvalues)
+{
+ size_t i;
+ bool found = false;
+ /*
+ * As of version 2.10, QEMU can be built without the TCG.
+ * */
+ if (qemuCaps->version < 2010000)
+ return 0;
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
+ return 0;
+
+ for (i = 0; i < nvalues; i++) {
+ if (STREQ(values[i], "tcg-accel")) {
+ found = true;
You can set the capability here without intermediate variable and just
return.
+ break;
+ }
+ }
+
+ if (!found)
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_TCG_DISABLED);
+
+ return 0;
+}
+
+static int
+virQEMUCapsProbeQMPTypes(virQEMUCapsPtr qemuCaps,
qemuMonitorPtr mon)
{
int nvalues;
With the above resolved:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
10:31 a.m.
New subject: [libvirt PATCH v4 3/3] add virQEMUCapsProbeQMPTCGState function to set QEMU_CAPS_TCG_DISABLED
On 4/24/20 9:58 AM, Peter Krempa wrote:
On Wed, Apr 22, 2020 at 17:50:44 -0400, Tobin Feldman-Fitzthum
wrote:
> Add virQEMUCapsProbeQMPTCGState to set TCG_DISABLED cap if version
> is > 2.10, KVM is enabled, and tcg-accel is not present in
> qom-list-types result.
>
> Signed-off-by: Tobin Feldman-Fitzthum <tobin(a)linux.vnet.ibm.com>
> ---
> src/qemu/qemu_capabilities.c | 34 ++++++++++++++++++++++++++++++++--
> 1 file changed, 32 insertions(+), 2 deletions(-)
I don't feel comfortable reviewing the other two patches which deal with
the actual use of the capability, so I'll leave those for somebody else.
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index e7179ea048..4a3170fc5c 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2576,7 +2576,35 @@ virQEMUCapsProbeQMPGenericProps(virQEMUCapsPtr qemuCaps,
> }
>
> static int
> -virQEMUCapsProbeQMPDevices(virQEMUCapsPtr qemuCaps,
> +virQEMUCapsProbeQMPTCGState(virQEMUCapsPtr qemuCaps,
Returned value of the new function is not used, so please define it as
'void'
> + char **values,
> + int nvalues)
> +{
> + size_t i;
> + bool found = false;
> + /*
> + * As of version 2.10, QEMU can be built without the TCG.
> + * */
> + if (qemuCaps->version < 2010000)
> + return 0;
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM))
> + return 0;
> +
> + for (i = 0; i < nvalues; i++) {
> + if (STREQ(values[i], "tcg-accel")) {
> + found = true;
You can set the capability here without intermediate variable and just
return.
I'm not quite sure what you mean. The capability tracks lack of
tcg-accel. So what can be done here is return and drop the check before
setting the capability below.
> + break;
> + }
> + }
> +
> + if (!found)
> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_TCG_DISABLED);
> +
> + return 0;
> +}
> +
> +static int
> +virQEMUCapsProbeQMPTypes(virQEMUCapsPtr qemuCaps,
> qemuMonitorPtr mon)
> {
> int nvalues;
With the above resolved:
Reviewed-by: Peter Krempa <pkrempa(a)redhat.com>
Michal
1669
days inactive
1675
days old
16 comments
5 participants
participants (5)
-
Ján Tomko -
Michal Privoznik -
Peter Krempa -
tobin -
Tobin Feldman-Fitzthum