Re: [PATCH V2 2/3] conf: Add channel devices to domain capabilities

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&gt; >>> --- >>>   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.