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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux