2:05 a.m.
On 10/13/22 22:18, Jim Fehlig wrote:
On 10/13/22 09:46, Michal Prívozník wrote:
> On 10/13/22 01:24, Jim Fehlig wrote:
>> As qemu becomes more modularized, it is important for libvirt to
>> advertise
>> availability of the modularized functionality through capabilities. This
>> change adds channel devices to domain capabilities, allowing clients
>> such
>> as virt-install to avoid using spicevmc channel devices when not
>> supported
>> by the target qemu.
>>
>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
>> ---
>> docs/formatdomaincaps.rst | 24 +++++++++++++++++++
>> src/conf/domain_capabilities.c | 13 ++++++++++
>> src/conf/domain_capabilities.h | 8 +++++++
>> src/conf/schemas/domaincaps.rng | 10 ++++++++
>> src/qemu/qemu_capabilities.c | 16 +++++++++++++
>> src/qemu/qemu_capabilities.h | 3 +++
>> .../domaincapsdata/qemu_4.2.0-q35.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_4.2.0-tcg.x86_64.xml | 7 ++++++
>> .../qemu_4.2.0-virt.aarch64.xml | 6 +++++
>> tests/domaincapsdata/qemu_4.2.0.aarch64.xml | 6 +++++
>> tests/domaincapsdata/qemu_4.2.0.ppc64.xml | 6 +++++
>> tests/domaincapsdata/qemu_4.2.0.s390x.xml | 6 +++++
>> tests/domaincapsdata/qemu_4.2.0.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_5.0.0-q35.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_5.0.0-tcg.x86_64.xml | 7 ++++++
>> .../qemu_5.0.0-virt.aarch64.xml | 6 +++++
>> tests/domaincapsdata/qemu_5.0.0.aarch64.xml | 6 +++++
>> tests/domaincapsdata/qemu_5.0.0.ppc64.xml | 6 +++++
>> tests/domaincapsdata/qemu_5.0.0.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_5.1.0-q35.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_5.1.0-tcg.x86_64.xml | 7 ++++++
>> tests/domaincapsdata/qemu_5.1.0.sparc.xml | 7 ++++++
>> tests/domaincapsdata/qemu_5.1.0.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_5.2.0-q35.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_5.2.0-tcg.x86_64.xml | 7 ++++++
>> .../qemu_5.2.0-virt.aarch64.xml | 6 +++++
>> tests/domaincapsdata/qemu_5.2.0.aarch64.xml | 6 +++++
>> tests/domaincapsdata/qemu_5.2.0.ppc64.xml | 6 +++++
>> tests/domaincapsdata/qemu_5.2.0.s390x.xml | 6 +++++
>> tests/domaincapsdata/qemu_5.2.0.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_6.0.0-q35.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_6.0.0-tcg.x86_64.xml | 7 ++++++
>> .../qemu_6.0.0-virt.aarch64.xml | 6 +++++
>> tests/domaincapsdata/qemu_6.0.0.aarch64.xml | 6 +++++
>> tests/domaincapsdata/qemu_6.0.0.s390x.xml | 6 +++++
>> tests/domaincapsdata/qemu_6.0.0.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_6.1.0-q35.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_6.1.0-tcg.x86_64.xml | 7 ++++++
>> tests/domaincapsdata/qemu_6.1.0.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_6.2.0-q35.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_6.2.0-tcg.x86_64.xml | 7 ++++++
>> .../qemu_6.2.0-virt.aarch64.xml | 7 ++++++
>> tests/domaincapsdata/qemu_6.2.0.aarch64.xml | 7 ++++++
>> tests/domaincapsdata/qemu_6.2.0.ppc64.xml | 6 +++++
>> tests/domaincapsdata/qemu_6.2.0.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_7.0.0-q35.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_7.0.0-tcg.x86_64.xml | 7 ++++++
>> .../qemu_7.0.0-virt.aarch64.xml | 7 ++++++
>> tests/domaincapsdata/qemu_7.0.0.aarch64.xml | 7 ++++++
>> tests/domaincapsdata/qemu_7.0.0.ppc64.xml | 6 +++++
>> tests/domaincapsdata/qemu_7.0.0.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_7.1.0-q35.x86_64.xml | 7 ++++++
>> .../domaincapsdata/qemu_7.1.0-tcg.x86_64.xml | 7 ++++++
>> tests/domaincapsdata/qemu_7.1.0.x86_64.xml | 7 ++++++
>> .../caps_4.2.0.x86_64.xml | 1 +
>> .../caps_5.0.0.riscv64.xml | 1 +
>> .../caps_5.0.0.x86_64.xml | 1 +
>> .../qemucapabilitiesdata/caps_5.1.0.sparc.xml | 1 +
>> .../caps_5.1.0.x86_64.xml | 1 +
>> .../caps_5.2.0.riscv64.xml | 1 +
>> .../caps_5.2.0.x86_64.xml | 1 +
>> .../caps_6.0.0.x86_64.xml | 1 +
>> .../caps_6.1.0.x86_64.xml | 1 +
>> .../caps_6.2.0.aarch64.xml | 1 +
>> .../caps_6.2.0.x86_64.xml | 1 +
>> .../caps_7.0.0.aarch64.xml | 1 +
>> .../caps_7.0.0.x86_64.xml | 1 +
>> .../caps_7.1.0.x86_64.xml | 1 +
>> 68 files changed, 408 insertions(+)
>>
>> diff --git a/docs/formatdomaincaps.rst b/docs/formatdomaincaps.rst
>> index 93d36f2702..f95d3a7083 100644
>> --- a/docs/formatdomaincaps.rst
>> +++ b/docs/formatdomaincaps.rst
>> @@ -565,6 +565,30 @@ USB redirdev device capabilities are exposed
>> under the ``redirdev`` element. For
>> ``bus``
>> Options for the ``bus`` attribute of the ``<redirdev/>`` element.
>> +Channel device
>> +^^^^^^^^^^^^^^
>> +
>> +Channel device capabilities are exposed under the ``channel``
>> element. For instance:
>> +
>> +::
>> +
>> + <domainCapabilities>
>> + ...
>> + <devices>
>> + <channel supported='yes'>
>> + <enum name='type'>
>> + <value>pty</value>
>> + <value>unix</value>
>> + <value>spicevmc</value>
>> + </enum>
>> + </channel
>> + ...
>> + </devices>
>> + </domainCapabilities>
>> +
>> +``type``
>> + Options for the ``type`` attribute of the ``<channel/>`` element.
>> +
>> Features
>> ~~~~~~~~
>> diff --git a/src/conf/domain_capabilities.c
>> b/src/conf/domain_capabilities.c
>> index f8b2f88376..a7f256e4ec 100644
>> --- a/src/conf/domain_capabilities.c
>> +++ b/src/conf/domain_capabilities.c
>> @@ -574,6 +574,18 @@ virDomainCapsDeviceRedirdevFormat(virBuffer *buf,
>> }
>> +static void
>> +virDomainCapsDeviceChannelFormat(virBuffer *buf,
>> + const virDomainCapsDeviceChannel
>> *channel)
>> +{
>> + FORMAT_PROLOGUE(channel);
>> +
>> + ENUM_PROCESS(channel, type, virDomainChrTypeToString);
>> +
>> + FORMAT_EPILOGUE(channel);
>> +}
>> +
>> +
>> /**
>> * virDomainCapsFeatureGICFormat:
>> * @buf: target buffer
>> @@ -688,6 +700,7 @@ virDomainCapsFormat(const virDomainCaps *caps)
>> virDomainCapsDeviceFilesystemFormat(&buf, &caps->filesystem);
>> virDomainCapsDeviceTPMFormat(&buf, &caps->tpm);
>> virDomainCapsDeviceRedirdevFormat(&buf, &caps->redirdev);
>> + virDomainCapsDeviceChannelFormat(&buf, &caps->channel);
>> virBufferAdjustIndent(&buf, -2);
>> virBufferAddLit(&buf, "</devices>\n");
>> diff --git a/src/conf/domain_capabilities.h
>> b/src/conf/domain_capabilities.h
>> index ba7c2a5e42..e0cfa75531 100644
>> --- a/src/conf/domain_capabilities.h
>> +++ b/src/conf/domain_capabilities.h
>> @@ -137,6 +137,13 @@ struct _virDomainCapsDeviceRedirdev {
>> virDomainCapsEnum bus; /* virDomainRedirdevBus */
>> };
>> +STATIC_ASSERT_ENUM(VIR_DOMAIN_CHR_TYPE_LAST);
>> +typedef struct _virDomainCapsDeviceChannel virDomainCapsDeviceChannel;
>> +struct _virDomainCapsDeviceChannel {
>> + virTristateBool supported;
>> + virDomainCapsEnum type; /* virDomainChrType */
>> +};
>> +
>> STATIC_ASSERT_ENUM(VIR_DOMAIN_FS_DRIVER_TYPE_LAST);
>> typedef struct _virDomainCapsDeviceFilesystem
>> virDomainCapsDeviceFilesystem;
>> struct _virDomainCapsDeviceFilesystem {
>> @@ -234,6 +241,7 @@ struct _virDomainCaps {
>> virDomainCapsDeviceFilesystem filesystem;
>> virDomainCapsDeviceTPM tpm;
>> virDomainCapsDeviceRedirdev redirdev;
>> + virDomainCapsDeviceChannel channel;
>> /* add new domain devices here */
>> virDomainCapsFeatureGIC gic;
>> diff --git a/src/conf/schemas/domaincaps.rng
>> b/src/conf/schemas/domaincaps.rng
>> index cf7a1d1d89..a6747b20ef 100644
>> --- a/src/conf/schemas/domaincaps.rng
>> +++ b/src/conf/schemas/domaincaps.rng
>> @@ -201,6 +201,9 @@
>> <optional>
>> <ref name="redirdev"/>
>> </optional>
>> + <optional>
>> + <ref name="channel"/>
>> + </optional>
>> </element>
>> </define>
>> @@ -260,6 +263,13 @@
>> </element>
>> </define>
>> + <define name="channel">
>> + <element name="channel">
>> + <ref name="supported"/>
>> + <ref name="enum"/>
>> + </element>
>> + </define>
>> +
>> <define name="features">
>> <element name="features">
>> <optional>
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 5a664ec628..98849daf4c 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -1392,6 +1392,7 @@ struct virQEMUCapsStringFlags
>> virQEMUCapsObjectTypes[] = {
>> { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
>> { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
>> { "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
>> + { "chardev-spicevmc", X_QEMU_CAPS_CHARDEV_SPICEVMC },
>
> The X_ prefix means that the capability was retired beause we are sure
> QEMU has it, always. Or another capability reflects the same. In this
> specific case I'd say QEMU_CAPS_SPICE is sufficient.
If you mean s/X_QEMU_CAPS_CHARDEV_SPICEVMC/QEMU_CAPS_SPICE/, it doesn't
work. 'spicevmc' is not shown as a supported channel type in
domcapabilities.
I thought more like:
diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c
index 98849daf4c..c2f6de9363 100644
--- i/src/qemu/qemu_capabilities.c
+++ w/src/qemu/qemu_capabilities.c
@@ -1392,7 +1392,6 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{ "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
{ "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
{ "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
- { "chardev-spicevmc", X_QEMU_CAPS_CHARDEV_SPICEVMC },
};
@@ -6357,7 +6356,7 @@ virQEMUCapsFillDomainDeviceChannelCaps(virQEMUCaps *qemuCaps,
VIR_DOMAIN_CAPS_ENUM_SET(channel->type,
VIR_DOMAIN_CHR_TYPE_PTY, VIR_DOMAIN_CHR_TYPE_UNIX);
- if (virQEMUCapsGet(qemuCaps, X_QEMU_CAPS_CHARDEV_SPICEVMC))
+ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SPICE))
VIR_DOMAIN_CAPS_ENUM_SET(channel->type, VIR_DOMAIN_CHR_TYPE_SPICEVMC);
}
(diff against this patch, minus qemucapabilitiessdata/ modifications)
This renders the same result as your patch. At least according to our test suite.
I thought it was safe to use the X_ variant since it is
already included in CapsFlags
https://gitlab.com/libvirt/libvirt/-/blob/master/src/qemu/qemu_capabiliti...
Well, define safe :-) It is safe in a sense that we can introduce
capabilities as we please, because we have this cache mechanism that
makes libvirt requery caps when needed. And let's leave migration aside
for a moment. But, it's not desirable when we already have a capability
that reflects the same information. In other words, the commit that
retired QEMU_CAPS_CHARDEV_SPICEVMC (v4.3.0-rc1~322) claims, that
qemu-1.2.0 and newer do support -chardev spicevmc. What's not written in
the commit message (and probably should), is that it's enough for us to
rely on QEMU_CAPS_SPICE. But this can be seen in the code (if you know
where to look, which I admit is not developer friendly):
1) qemuValidateDomainChrSourceDef() makes sure that if there's
VIR_DOMAIN_CHR_TYPE_SPICEVMC in the domain XML there's also <graphics
type='spice'/>, then
2) qemuValidateDomainDeviceDefGraphics() makes sure that if there's
spice graphics in the domain XML, QEMU has QEMU_CAPS_SPICE.
Let me break down step 2 a bit more, because it's obfuscated a bit (in
pursuit of code deduplication). The
virQEMUCapsFillDomainDeviceGraphicsCaps() is called, which fills a
portion of domaincaps with supported graphics types (by looking at
qemuCaps), and then VIR_DOMAIN_CAPS_ENUM_IS_SET() checks whether
VIR_DOMAIN_GRAPHICS_TYPE_SPICE was set in the domaincaps.
And indeed, looking into qemu's code base (chardev/meson.build):
if spice.found()
module_ss = ss.source_set()
module_ss.add(when: [spice], if_true: files('spice.c'))
chardev_modules += { 'spice': module_ss }
endif
So it seems that spicevmc can't be compiled out and is available
whenever spice is. The reason we had that capability was that as
qemu/spice gained new features, there was a time when spice was
available in qemu but spicevmc wasn't. But no qemu we support now (4.2.0
and newer - see QEMU_MIN_MAJOR and friends in qemu_capabilities.c)
allows such situation.
I hope this helps.
Michal