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@xxxxxxxx>
>>> ---
>>> 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_capabilities.h#L121
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
