Re: [PATCH] Add page_per_vq flag to the 'driver' element of virtio devices

[Gavi Teitz <gavi@xxxxxxxxxx>]

 

On 4/29/21 2:51 PM, Peter Krempa wrote:

On Thu, Apr 29, 2021 at 14:12:44 +0300, Gavi Teitz wrote:

https://nam11.safelinks.protection.outlook.com/?url="">

Add support for setting the page-per-vq flag, which is important for

vdpa with vhost-user performance.

Signed-off-by: Gavi Teitz <gavi@xxxxxxxxxx>

---

 docs/formatdomain.rst                              | 11 +++++-

 docs/schemas/domaincommon.rng                      |  5 +++

 src/conf/domain_conf.c                             | 15 ++++++++

 src/conf/domain_conf.h                             |  1 +

 src/qemu/qemu_capabilities.c                       |  2 +

 src/qemu/qemu_capabilities.h                       |  1 +

 src/qemu/qemu_command.c                            |  5 +++

 src/qemu/qemu_hotplug.c                            |  1 +

 tests/qemuxml2argvdata/net-virtio-page-per-vq.args | 31 ++++++++++++++++

 tests/qemuxml2argvdata/net-virtio-page-per-vq.xml  | 29 +++++++++++++++

 tests/qemuxml2argvtest.c                           |  2 +

 .../qemuxml2xmloutdata/net-virtio-page-per-vq.xml  | 43 ++++++++++++++++++++++

 tests/qemuxml2xmltest.c                            |  2 +

 13 files changed, 147 insertions(+), 1 deletion(-)

 create mode 100644 tests/qemuxml2argvdata/net-virtio-page-per-vq.args

 create mode 100644 tests/qemuxml2argvdata/net-virtio-page-per-vq.xml

 create mode 100644 tests/qemuxml2xmloutdata/net-virtio-page-per-vq.xml

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst

index 282176c..f5a9c70 100644

--- a/docs/formatdomain.rst

+++ b/docs/formatdomain.rst

@@ -5120,7 +5120,7 @@ Setting NIC driver-specific options

        <source network='default'/>

        <target dev='vnet1'/>

        <model type='virtio'/>

-       <driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rx_queue_size='256' tx_queue_size='256'>

+       <driver name='vhost' txmode='iothread' ioeventfd='on' event_idx='off' queues='5' rx_queue_size='256' tx_queue_size='256' page_per_vq='on'>

          <host csum='off' gso='off' tso4='off' tso6='off' ecn='off' ufo='off' mrg_rxbuf='off'/>

          <guest csum='off' tso4='off' tso6='off' ecn='off' ufo='off'/>

        </driver>

@@ -5215,6 +5215,15 @@ following attributes are available for the ``"virtio"`` NIC driver:

    only for ``vhostuser`` type. :since:`Since 3.7.0 (QEMU and KVM only)`

    **In general you should leave this option alone, unless you are very certain

    you know what you are doing.**

+``page_per_vq``

+   This optional attribute controls the layout of the notification capabilities

+   exposed to the guest. When enabled, each virtio queue will have a dedicated

+   page on the device BAR exposed to the guest. It is recommended to be used when

+   vDPA is enabled on the hypervisor, as it enables mapping the notification area

+   to the physical device, which is only supported in page granularity. The

+   default is determined by QEMU; as off. :since:`Since 2.8.0 (QEMU only)`

The 'since' tag is primarily used for the libvirt version which

introduces the feature thus you must include that.

Also note that starting from the next release the minimum supported qemu

version will be bumped to qemu-2.10 thus the qemu version note is not

needed.

It isn't really clear what the 'since' tag refers to, if I understand your comment correctly it refers to the libvirt version, so I will change it to 7.4.0 and remove the '(QEMU only)' part.

+   **In general you should leave this option alone, unless you are very certain

+   you know what you are doing.**

 virtio options

    For virtio interfaces, `Virtio-specific options <#elementsVirtio>`__ can also

    be set. ( :since:`Since 3.5.0` )

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng

index a2e5c50..e61ad67 100644

--- a/docs/schemas/domaincommon.rng

+++ b/docs/schemas/domaincommon.rng

@@ -3463,6 +3463,11 @@

                 </attribute>

               </optional>

               <optional>

+                <attribute name="page_per_vq">

+                  <ref name="virOnOff"/>

+                </attribute>

+              </optional>

+              <optional>

                 <attribute name="txmode">

                   <choice>

                     <value>iothread</value>

[...]

@@ -1405,6 +1406,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioNet[] = {

     { "event_idx", QEMU_CAPS_VIRTIO_NET_EVENT_IDX, NULL },

     { "rx_queue_size", QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE, NULL },

     { "tx_queue_size", QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE, NULL },

+    { "page_per_vq", QEMU_CAPS_VIRTIO_NET_PAGE_PER_VQ, NULL },

     { "host_mtu", QEMU_CAPS_VIRTIO_NET_HOST_MTU, NULL },

     { "disable-legacy", QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, NULL },

     { "iommu_platform", QEMU_CAPS_VIRTIO_PCI_IOMMU_PLATFORM, NULL },

diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h

index f54aad5..75feed4 100644

--- a/src/qemu/qemu_capabilities.h

+++ b/src/qemu/qemu_capabilities.h

@@ -609,6 +609,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */

     /* 400 */

     QEMU_CAPS_COMPAT_DEPRECATED, /* -compat deprecated-(input|output) is supported */

     QEMU_CAPS_ACPI_INDEX, /* PCI device 'acpi-index' property */

+    QEMU_CAPS_VIRTIO_NET_PAGE_PER_VQ, /* virtio-net-*.page_per_vq */

     QEMU_CAPS_LAST /* this must always be the last item */

 } virQEMUCapsFlags;

1) qemu capabilities changes are usually separated into a separate commit

2) missing update to the capability data, either this doesn't detect

   anything or tests would be broken.

3) the capability is not necessary since all qemus supported since

   libvirt-7.4 will have it

Since this is not necessary I will remove the changes to qemu_capabilities*.

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c

index d7f1c71..7b5834f 100644

--- a/src/qemu/qemu_command.c

+++ b/src/qemu/qemu_command.c

@@ -3630,6 +3630,11 @@ qemuBuildNicDevStr(virDomainDef *def,

         if (net->driver.virtio.tx_queue_size)

             virBufferAsprintf(&buf, ",tx_queue_size=%u", net->driver.virtio.tx_queue_size);

+        if (net->driver.virtio.page_per_vq) {

+            virBufferAsprintf(&buf, ",page-per-vq=%s",

+                              virTristateSwitchTypeToString(net->driver.virtio.page_per_vq));

+        }

+

         if (net->mtu)

             virBufferAsprintf(&buf, ",host_mtu=%u", net->mtu);

[...]

diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c

index f0efe98..f6b8f34 100644

--- a/tests/qemuxml2argvtest.c

+++ b/tests/qemuxml2argvtest.c

@@ -1654,6 +1654,8 @@ mymain(void)

             QEMU_CAPS_VIRTIO_NET_TX_QUEUE_SIZE);

     DO_TEST_PARSE_ERROR("net-virtio-rxqueuesize-invalid-size",

                         QEMU_CAPS_VIRTIO_NET_RX_QUEUE_SIZE);

+    DO_TEST("net-virtio-page-per-vq",

+            QEMU_CAPS_VIRTIO_NET_PAGE_PER_VQ);

New positive tests should not use DO_TEST, but DO_TEST_CAPS_LATEST.

Ack.

     DO_TEST("net-virtio-teaming",

             QEMU_CAPS_VIRTIO_NET_FAILOVER,

             QEMU_CAPS_DEVICE_VFIO_PCI);

[...]

According to the docs this is also a guest-visible change, thus we are

missing a check in the ABI stability checker code which ensures that the

guest ABI doesn't change.

See virDomainDefCheckABIStabilityFlags to find the appropriate place.

I couldn't find a related function for comparing driver options for virDomainNetDef (I expected to find something in virDomainNetDefCheckABIStability(), but didn't), please clarify what needs to be added here.