[PATCH V2 2/3] conf: Add channel devices to domain capabilities
Jim Fehlig
jfehlig at suse.com
Fri Oct 14 21:34:39 UTC 2022
On 10/14/22 01:05, Michal Prívozník wrote:
> 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 at 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.
Yes, it works, but still reports a spicevmc channel device even when
chardev-spice.so is not installed :-).
>> 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.
Yes, it does. Thanks for the detailed response! I suppose we'll need to rethink
the dependencies in our downstream packages. E.g. currently it's possible to
install qemu-ui-spice-core (which contains ui-spice-core.so), but not
qemu-chardev-spice (which contains chardev-spice.so). That seems to fly in the
face of the upstream logic.
Jim
More information about the libvir-list
mailing list