[PATCH v2 00/15] virtio-gpio and various virtio cleanups

[PATCH v2 00/15] virtio-gpio and various virtio cleanups

[Alex Bennée]

Hi,

This series ostensibly adds virtio-user-gpio stubs to the build for
use with an external vhost-user daemon. We've been testing it with our
rust daemons from:

  https://github.com/rust-vmm/vhost-device

Getting the test enabled took some doing most likely because the need
for CONFIG support exercised additional paths in the code that were
not used for the simpler virtio-net tests. As a result the series has
a number of cleanup and documentation patches.

The final thing that needed fixing was the ensuring that
VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation
process. This was the hardest thing to track down as we store the
feature bits in several places variously as:

  in VirtIODevice as:
    uint64_t guest_features;
    uint64_t host_features;
    uint64_t backend_features;

 in vhost_dev as:
    uint64_t features;
    uint64_t acked_features;
    uint64_t backend_features;

and a number of the other device structures also have various features
fields along with get/set function handlers. It wasn't super clear
what the flow through this structures was meant to be but I'm fairly
sure there is unnecessary duplication in there somewhere. We could
certainly do with some docstrings to make the point of each field
clear.

Going forward I wonder if having a fake vhost-user daemon is
sustainable in the long term. Maybe it would be better to spawn real
vhost-user daemons in a test mode so we don't end up duplicating the
whole protocol?

Anyway please review.

Alex Bennée (13):
  contrib/vhost-user-blk: fix 32 bit build and enable
  include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE
  include/hw/virtio: document vhost_get_features
  include/hw/virtio: document vhost_ack_features
  tests/qtest: pass stdout/stderr down to subtests
  tests/qtest: add a timeout for subprocess_run_one_test
  tests/qtest: use qos_printf instead of g_test_message
  tests/qtest: catch unhandled vhost-user messages
  tests/qtest: use g_autofree for test_server_create_chr
  tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES
  tests/qtest: implement stub for VHOST_USER_GET_CONFIG
  tests/qtest: add a get_features op to vhost-user-test
  tests/qtest: enable tests for virtio-gpio

Viresh Kumar (2):
  hw/virtio: add boilerplate for vhost-user-gpio device
  hw/virtio: add vhost-user-gpio-pci boilerplate

 meson.build                             |   2 +-
 include/hw/virtio/vhost-user-gpio.h     |  35 +++
 include/hw/virtio/vhost.h               |  21 ++
 include/hw/virtio/virtio.h              |   7 +-
 tests/qtest/libqos/virtio-gpio.h        |  35 +++
 contrib/vhost-user-blk/vhost-user-blk.c |   6 +-
 hw/virtio/vhost-user-gpio-pci.c         |  69 +++++
 hw/virtio/vhost-user-gpio.c             | 357 ++++++++++++++++++++++++
 tests/qtest/libqos/virtio-gpio.c        | 171 ++++++++++++
 tests/qtest/libqos/virtio.c             |   2 +-
 tests/qtest/qos-test.c                  |   8 +-
 tests/qtest/vhost-user-test.c           | 179 ++++++++++--
 contrib/vhost-user-blk/meson.build      |   3 +-
 hw/virtio/Kconfig                       |   5 +
 hw/virtio/meson.build                   |   2 +
 tests/qtest/libqos/meson.build          |   1 +
 16 files changed, 872 insertions(+), 31 deletions(-)
 create mode 100644 include/hw/virtio/vhost-user-gpio.h
 create mode 100644 tests/qtest/libqos/virtio-gpio.h
 create mode 100644 hw/virtio/vhost-user-gpio-pci.c
 create mode 100644 hw/virtio/vhost-user-gpio.c
 create mode 100644 tests/qtest/libqos/virtio-gpio.c

-- 
2.30.2

[PATCH v2 01/15] contrib/vhost-user-blk: fix 32 bit build and enable

[Alex Bennée]

We were not building the vhost-user-blk server due to 32 bit
compilation problems. The problem was due to format string types so
fix that and then enable the build. Tweak the rule to follow the same
rules as other vhost-user daemons.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220321153037.3622127-12-alex.bennee@linaro.org>
---
 meson.build                             | 2 +-
 contrib/vhost-user-blk/vhost-user-blk.c | 6 +++---
 contrib/vhost-user-blk/meson.build      | 3 +--
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/meson.build b/meson.build
index 9ebc00f032..a33ed52b7a 100644
--- a/meson.build
+++ b/meson.build
@@ -1489,7 +1489,7 @@ have_vhost_user_blk_server = get_option('vhost_user_blk_server') \
            error_message: 'vhost_user_blk_server requires linux') \
   .require(have_vhost_user,
            error_message: 'vhost_user_blk_server requires vhost-user support') \
-  .disable_auto_if(not have_system) \
+  .disable_auto_if(not have_tools and not have_system) \
   .allowed()
 
 if get_option('fuse').disabled() and get_option('fuse_lseek').enabled()
diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
index cd4a5d7335..9cb78ca1d0 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -146,7 +146,7 @@ vub_readv(VubReq *req, struct iovec *iov, uint32_t iovcnt)
     req->size = vub_iov_size(iov, iovcnt);
     rc = preadv(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
     if (rc < 0) {
-        fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
+        fprintf(stderr, "%s, Sector %"PRIu64", Size %zu failed with %s\n",
                 vdev_blk->blk_name, req->sector_num, req->size,
                 strerror(errno));
         return -1;
@@ -169,7 +169,7 @@ vub_writev(VubReq *req, struct iovec *iov, uint32_t iovcnt)
     req->size = vub_iov_size(iov, iovcnt);
     rc = pwritev(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
     if (rc < 0) {
-        fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
+        fprintf(stderr, "%s, Sector %"PRIu64", Size %zu failed with %s\n",
                 vdev_blk->blk_name, req->sector_num, req->size,
                 strerror(errno));
         return -1;
@@ -188,7 +188,7 @@ vub_discard_write_zeroes(VubReq *req, struct iovec *iov, uint32_t iovcnt,
 
     size = vub_iov_size(iov, iovcnt);
     if (size != sizeof(*desc)) {
-        fprintf(stderr, "Invalid size %ld, expect %ld\n", size, sizeof(*desc));
+        fprintf(stderr, "Invalid size %zd, expect %zd\n", size, sizeof(*desc));
         return -1;
     }
     buf = g_new0(char, size);
diff --git a/contrib/vhost-user-blk/meson.build b/contrib/vhost-user-blk/meson.build
index 601ea15ef5..dcb9e2ffcd 100644
--- a/contrib/vhost-user-blk/meson.build
+++ b/contrib/vhost-user-blk/meson.build
@@ -1,5 +1,4 @@
-# FIXME: broken on 32-bit architectures
 executable('vhost-user-blk', files('vhost-user-blk.c'),
            dependencies: [qemuutil, vhost_user],
-           build_by_default: false,
+           build_by_default: targetos == 'linux',
            install: false)
-- 
2.30.2

Re: [PATCH v2 01/15] contrib/vhost-user-blk: fix 32 bit build and enable

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Tue, May 24, 2022 at 04:40:42PM +0100, Alex Bennée wrote:
> We were not building the vhost-user-blk server due to 32 bit
> compilation problems. The problem was due to format string types so
> fix that and then enable the build. Tweak the rule to follow the same
> rules as other vhost-user daemons.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20220321153037.3622127-12-alex.bennee@linaro.org>
> ---
>  meson.build                             | 2 +-
>  contrib/vhost-user-blk/vhost-user-blk.c | 6 +++---
>  contrib/vhost-user-blk/meson.build      | 3 +--
>  3 files changed, 5 insertions(+), 6 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 01/15] contrib/vhost-user-blk: fix 32 bit build and enable

[Raphael Norwitz]

On Tue, May 24, 2022 at 04:40:42PM +0100, Alex Bennée wrote:
> We were not building the vhost-user-blk server due to 32 bit
> compilation problems. The problem was due to format string types so
> fix that and then enable the build. Tweak the rule to follow the same
> rules as other vhost-user daemons.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20220321153037.3622127-12-alex.bennee@linaro.org>

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> ---
>  meson.build                             | 2 +-
>  contrib/vhost-user-blk/vhost-user-blk.c | 6 +++---
>  contrib/vhost-user-blk/meson.build      | 3 +--
>  3 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 9ebc00f032..a33ed52b7a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1489,7 +1489,7 @@ have_vhost_user_blk_server = get_option('vhost_user_blk_server') \
>             error_message: 'vhost_user_blk_server requires linux') \
>    .require(have_vhost_user,
>             error_message: 'vhost_user_blk_server requires vhost-user support') \
> -  .disable_auto_if(not have_system) \
> +  .disable_auto_if(not have_tools and not have_system) \
>    .allowed()
>  
>  if get_option('fuse').disabled() and get_option('fuse_lseek').enabled()
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c b/contrib/vhost-user-blk/vhost-user-blk.c
> index cd4a5d7335..9cb78ca1d0 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -146,7 +146,7 @@ vub_readv(VubReq *req, struct iovec *iov, uint32_t iovcnt)
>      req->size = vub_iov_size(iov, iovcnt);
>      rc = preadv(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
>      if (rc < 0) {
> -        fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
> +        fprintf(stderr, "%s, Sector %"PRIu64", Size %zu failed with %s\n",
>                  vdev_blk->blk_name, req->sector_num, req->size,
>                  strerror(errno));
>          return -1;
> @@ -169,7 +169,7 @@ vub_writev(VubReq *req, struct iovec *iov, uint32_t iovcnt)
>      req->size = vub_iov_size(iov, iovcnt);
>      rc = pwritev(vdev_blk->blk_fd, iov, iovcnt, req->sector_num * 512);
>      if (rc < 0) {
> -        fprintf(stderr, "%s, Sector %"PRIu64", Size %lu failed with %s\n",
> +        fprintf(stderr, "%s, Sector %"PRIu64", Size %zu failed with %s\n",
>                  vdev_blk->blk_name, req->sector_num, req->size,
>                  strerror(errno));
>          return -1;
> @@ -188,7 +188,7 @@ vub_discard_write_zeroes(VubReq *req, struct iovec *iov, uint32_t iovcnt,
>  
>      size = vub_iov_size(iov, iovcnt);
>      if (size != sizeof(*desc)) {
> -        fprintf(stderr, "Invalid size %ld, expect %ld\n", size, sizeof(*desc));
> +        fprintf(stderr, "Invalid size %zd, expect %zd\n", size, sizeof(*desc));
>          return -1;
>      }
>      buf = g_new0(char, size);
> diff --git a/contrib/vhost-user-blk/meson.build b/contrib/vhost-user-blk/meson.build
> index 601ea15ef5..dcb9e2ffcd 100644
> --- a/contrib/vhost-user-blk/meson.build
> +++ b/contrib/vhost-user-blk/meson.build
> @@ -1,5 +1,4 @@
> -# FIXME: broken on 32-bit architectures
>  executable('vhost-user-blk', files('vhost-user-blk.c'),
>             dependencies: [qemuutil, vhost_user],
> -           build_by_default: false,
> +           build_by_default: targetos == 'linux',
>             install: false)
> -- 
> 2.30.2
> 

[PATCH v2 02/15] include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE

[Alex Bennée]

When debugging a new vhost user you may be surprised to see
VHOST_USER_F_PROTOCOL getting squashed in the maze of
backend_features, acked_features and guest_features. Expand the
description here to help the next poor soul trying to work through
this.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/virtio.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index db1c0ddf6b..2b2587d324 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -24,7 +24,12 @@
 #include "qom/object.h"
 #include "hw/virtio/vhost.h"
 
-/* A guest should never accept this.  It implies negotiation is broken. */
+/*
+ * A guest should never accept this.  It implies negotiation is
+ * broken between the driver frontend and the device. This bit is
+ * re-used for vhost to advertise VHOST_USER_F_PROTOCOL_FEATURES
+ * between QEMU and a vhost backend.
+ */
 #define VIRTIO_F_BAD_FEATURE		30
 
 #define VIRTIO_LEGACY_FEATURES ((0x1ULL << VIRTIO_F_BAD_FEATURE) | \
-- 
2.30.2

Re: [PATCH v2 02/15] include/hw/virtio: more comment for VIRTIO_F_BAD_FEATURE

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

On Tue, May 24, 2022 at 04:40:43PM +0100, Alex Bennée wrote:
> When debugging a new vhost user you may be surprised to see
> VHOST_USER_F_PROTOCOL getting squashed in the maze of
> backend_features, acked_features and guest_features. Expand the
> description here to help the next poor soul trying to work through
> this.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/hw/virtio/virtio.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index db1c0ddf6b..2b2587d324 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -24,7 +24,12 @@
>  #include "qom/object.h"
>  #include "hw/virtio/vhost.h"
>  
> -/* A guest should never accept this.  It implies negotiation is broken. */
> +/*
> + * A guest should never accept this.  It implies negotiation is
> + * broken between the driver frontend and the device. This bit is
> + * re-used for vhost to advertise VHOST_USER_F_PROTOCOL_FEATURES

s/vhost/vhost-user/

> + * between QEMU and a vhost backend.
> + */
>  #define VIRTIO_F_BAD_FEATURE		30
>  
>  #define VIRTIO_LEGACY_FEATURES ((0x1ULL << VIRTIO_F_BAD_FEATURE) | \
> -- 
> 2.30.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 03/15] include/hw/virtio: document vhost_get_features

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index b291fe4e24..9f9f57c46e 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -246,6 +246,17 @@ bool vhost_virtqueue_pending(struct vhost_dev *hdev, int n);
  */
 void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
                           bool mask);
+
+/**
+ * vhost_get_features() - return a sanitised set of feature bits
+ * @hdev: common vhost_dev structure
+ * @feature_bits: pointer to terminated table of feature bits
+ * @features: original feature set
+ *
+ * This returns a set of features bits that is an intersection of what
+ * is supported by the vhost backend (hdev->features), the supported
+ * feature_bits and the requested feature set.
+ */
 uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
                             uint64_t features);
 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
-- 
2.30.2

Re: [PATCH v2 03/15] include/hw/virtio: document vhost_get_features

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 262 bytes --]

On Tue, May 24, 2022 at 04:40:44PM +0100, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/hw/virtio/vhost.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 04/15] include/hw/virtio: document vhost_ack_features

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 9f9f57c46e..bfe868e341 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -259,6 +259,16 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
  */
 uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
                             uint64_t features);
+
+/**
+ * vhost_ack_features() - set vhost acked_features
+ * @hdev: common vhost_dev structure
+ * @feature_bits: pointer to terminated table of feature bits
+ * @features: requested feature set
+ *
+ * This sets the internal hdev->acked_features to the intersection of
+ * the backends advertised features and the supported feature_bits.
+ */
 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                         uint64_t features);
 bool vhost_has_free_slot(void);
-- 
2.30.2

Re: [PATCH v2 04/15] include/hw/virtio: document vhost_ack_features

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

On Tue, May 24, 2022 at 04:40:45PM +0100, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/hw/virtio/vhost.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 05/15] hw/virtio: add boilerplate for vhost-user-gpio device

[Alex Bennée]

From: Viresh Kumar <viresh.kumar@linaro.org>

This creates the QEMU side of the vhost-user-gpio device which connects
to the remote daemon. It is based of vhost-user-i2c code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <5390324a748194a21bc99b1538e19761a8c64092.1641987128.git.viresh.kumar@linaro.org>
[AJB: fixes for qtest]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - set VIRTIO_F_VERSION_1
  - set VHOST_USER_F_PROTOCOL_FEATURES
  - terminate feature_bits with VHOST_INVALID_FEATURE_BIT
  - ensure vdev->backend_features set
  - ensure vhost_dev.acked_features set
---
 include/hw/virtio/vhost-user-gpio.h |  35 +++
 hw/virtio/vhost-user-gpio.c         | 357 ++++++++++++++++++++++++++++
 hw/virtio/Kconfig                   |   5 +
 hw/virtio/meson.build               |   1 +
 4 files changed, 398 insertions(+)
 create mode 100644 include/hw/virtio/vhost-user-gpio.h
 create mode 100644 hw/virtio/vhost-user-gpio.c

diff --git a/include/hw/virtio/vhost-user-gpio.h b/include/hw/virtio/vhost-user-gpio.h
new file mode 100644
index 0000000000..afeb56f53e
--- /dev/null
+++ b/include/hw/virtio/vhost-user-gpio.h
@@ -0,0 +1,35 @@
+/*
+ * Vhost-user GPIO virtio device
+ *
+ * Copyright (c) 2021 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef _QEMU_VHOST_USER_GPIO_H
+#define _QEMU_VHOST_USER_GPIO_H
+
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-user.h"
+#include "standard-headers/linux/virtio_gpio.h"
+#include "chardev/char-fe.h"
+
+#define TYPE_VHOST_USER_GPIO "vhost-user-gpio-device"
+OBJECT_DECLARE_SIMPLE_TYPE(VHostUserGPIO, VHOST_USER_GPIO);
+
+struct VHostUserGPIO {
+    /*< private >*/
+    VirtIODevice parent;
+    CharBackend chardev;
+    struct virtio_gpio_config config;
+    struct vhost_virtqueue *vhost_vq;
+    struct vhost_dev vhost_dev;
+    VhostUserState vhost_user;
+    VirtQueue *command_vq;
+    VirtQueue *interrupt_vq;
+    bool connected;
+    /*< public >*/
+};
+
+#endif /* _QEMU_VHOST_USER_GPIO_H */
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
new file mode 100644
index 0000000000..87e3976880
--- /dev/null
+++ b/hw/virtio/vhost-user-gpio.c
@@ -0,0 +1,357 @@
+/*
+ * Vhost-user GPIO virtio device
+ *
+ * Copyright (c) 2022 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/vhost-user-gpio.h"
+#include "qemu/error-report.h"
+#include "standard-headers/linux/virtio_ids.h"
+
+/* do no other vhost-user daemons need this? */
+#define VHOST_USER_F_PROTOCOL_FEATURES 30
+
+static const int feature_bits[] = {
+    VIRTIO_F_VERSION_1,
+    VIRTIO_GPIO_F_IRQ,
+    VHOST_USER_F_PROTOCOL_FEATURES,
+    VHOST_INVALID_FEATURE_BIT
+};
+
+static void vu_gpio_get_config(VirtIODevice *vdev, uint8_t *config)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+
+    memcpy(config, &gpio->config, sizeof(gpio->config));
+}
+
+static int vu_gpio_config_notifier(struct vhost_dev *dev)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(dev->vdev);
+
+    memcpy(dev->vdev->config, &gpio->config, sizeof(gpio->config));
+    virtio_notify_config(dev->vdev);
+
+    return 0;
+}
+
+const VhostDevConfigOps gpio_ops = {
+    .vhost_dev_config_notifier = vu_gpio_config_notifier,
+};
+
+static int vu_gpio_start(VirtIODevice *vdev)
+{
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    int ret, i;
+
+    if (!k->set_guest_notifiers) {
+        error_report("binding does not support guest notifiers");
+        return -ENOSYS;
+    }
+
+    ret = vhost_dev_enable_notifiers(&gpio->vhost_dev, vdev);
+    if (ret < 0) {
+        error_report("Error enabling host notifiers: %d", ret);
+        return ret;
+    }
+
+    ret = k->set_guest_notifiers(qbus->parent, gpio->vhost_dev.nvqs, true);
+    if (ret < 0) {
+        error_report("Error binding guest notifier: %d", ret);
+        goto err_host_notifiers;
+    }
+
+    /*
+     * Before we start up we need to ensure we have the final feature
+     * set needed for the vhost configuration.
+     */
+    vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->backend_features);
+
+    ret = vhost_dev_start(&gpio->vhost_dev, vdev);
+    if (ret < 0) {
+        error_report("Error starting vhost-user-gpio: %d", ret);
+        goto err_guest_notifiers;
+    }
+
+    /*
+     * guest_notifier_mask/pending not used yet, so just unmask
+     * everything here. virtio-pci will do the right thing by
+     * enabling/disabling irqfd.
+     */
+    for (i = 0; i < gpio->vhost_dev.nvqs; i++) {
+        vhost_virtqueue_mask(&gpio->vhost_dev, vdev, i, false);
+    }
+
+    return 0;
+
+err_guest_notifiers:
+    k->set_guest_notifiers(qbus->parent, gpio->vhost_dev.nvqs, false);
+err_host_notifiers:
+    vhost_dev_disable_notifiers(&gpio->vhost_dev, vdev);
+
+    return ret;
+}
+
+static void vu_gpio_stop(VirtIODevice *vdev)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    int ret;
+
+    if (!k->set_guest_notifiers) {
+        return;
+    }
+
+    vhost_dev_stop(&gpio->vhost_dev, vdev);
+
+    ret = k->set_guest_notifiers(qbus->parent, gpio->vhost_dev.nvqs, false);
+    if (ret < 0) {
+        error_report("vhost guest notifier cleanup failed: %d", ret);
+        return;
+    }
+
+    vhost_dev_disable_notifiers(&gpio->vhost_dev, vdev);
+}
+
+static void vu_gpio_set_status(VirtIODevice *vdev, uint8_t status)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    bool should_start = status & VIRTIO_CONFIG_S_DRIVER_OK;
+
+    if (!vdev->vm_running) {
+        should_start = false;
+    }
+
+    if (!gpio->connected) {
+        return;
+    }
+
+    if (gpio->vhost_dev.started == should_start) {
+        return;
+    }
+
+    if (should_start) {
+        if (vu_gpio_start(vdev)) {
+            qemu_chr_fe_disconnect(&gpio->chardev);
+        }
+    } else {
+        vu_gpio_stop(vdev);
+    }
+}
+
+static uint64_t vu_gpio_get_features(VirtIODevice *vdev, uint64_t features,
+                                     Error **errp)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+
+    virtio_add_feature(&features, VIRTIO_GPIO_F_IRQ);
+    virtio_add_feature(&features, VIRTIO_F_VERSION_1);
+
+    vdev->backend_features = vhost_get_features(&gpio->vhost_dev, feature_bits,
+                                                features);
+    return vdev->backend_features;
+}
+
+static void vu_gpio_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+    /*
+     * Not normally called; it's the daemon that handles the queue;
+     * however virtio's cleanup path can call this.
+     */
+}
+
+static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
+{
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+
+    vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
+}
+
+static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
+{
+    virtio_delete_queue(gpio->command_vq);
+    virtio_delete_queue(gpio->interrupt_vq);
+    g_free(gpio->vhost_dev.vqs);
+    gpio->vhost_dev.vqs = NULL;
+    virtio_cleanup(vdev);
+    vhost_user_cleanup(&gpio->vhost_user);
+}
+
+static int vu_gpio_connect(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+    Error *local_err = NULL;
+    int ret;
+
+    if (gpio->connected) {
+        return 0;
+    }
+    gpio->connected = true;
+
+    vhost_dev_set_config_notifier(&gpio->vhost_dev, &gpio_ops);
+
+    ret = vhost_dev_init(&gpio->vhost_dev, &gpio->vhost_user,
+                         VHOST_BACKEND_TYPE_USER, 0, &local_err);
+    if (ret < 0) {
+        error_report("vhost-user-gpio: vhost initialization failed: %s",
+                     strerror(-ret));
+        return ret;
+    }
+
+    /* restore vhost state */
+    if (virtio_device_started(vdev, vdev->status)) {
+        vu_gpio_start(vdev);
+    }
+
+    return 0;
+}
+
+static void vu_gpio_disconnect(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+
+    if (!gpio->connected) {
+        return;
+    }
+    gpio->connected = false;
+
+    vu_gpio_stop(vdev);
+    vhost_dev_cleanup(&gpio->vhost_dev);
+}
+
+static void vu_gpio_event(void *opaque, QEMUChrEvent event)
+{
+    DeviceState *dev = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
+
+    switch (event) {
+    case CHR_EVENT_OPENED:
+        if (vu_gpio_connect(dev) < 0) {
+            qemu_chr_fe_disconnect(&gpio->chardev);
+            return;
+        }
+        break;
+    case CHR_EVENT_CLOSED:
+        vu_gpio_disconnect(dev);
+        break;
+    case CHR_EVENT_BREAK:
+    case CHR_EVENT_MUX_IN:
+    case CHR_EVENT_MUX_OUT:
+        /* Ignore */
+        break;
+    }
+}
+
+static void vu_gpio_device_realize(DeviceState *dev, Error **errp)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(dev);
+    Error *err = NULL;
+    int ret;
+
+    if (!gpio->chardev.chr) {
+        error_setg(errp, "vhost-user-gpio: chardev is mandatory");
+        return;
+    }
+
+    if (!vhost_user_init(&gpio->vhost_user, &gpio->chardev, errp)) {
+        return;
+    }
+
+    virtio_init(vdev, VIRTIO_ID_GPIO, sizeof(gpio->config));
+
+    gpio->vhost_dev.nvqs = 2;
+    gpio->command_vq = virtio_add_queue(vdev, 256, vu_gpio_handle_output);
+    gpio->interrupt_vq = virtio_add_queue(vdev, 256, vu_gpio_handle_output);
+    gpio->vhost_dev.vqs = g_new0(struct vhost_virtqueue, gpio->vhost_dev.nvqs);
+
+    gpio->connected = false;
+
+    qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, vu_gpio_event, NULL,
+                             dev, NULL, true);
+
+reconnect:
+    if (qemu_chr_fe_wait_connected(&gpio->chardev, &err) < 0) {
+        error_report_err(err);
+        do_vhost_user_cleanup(vdev, gpio);
+        return;
+    }
+
+    /* check whether vhost_user_gpio_connect() failed or not */
+    if (!gpio->connected) {
+        goto reconnect;
+    }
+
+    ret = vhost_dev_get_config(&gpio->vhost_dev, (uint8_t *)&gpio->config,
+                               sizeof(gpio->config), errp);
+    if (ret < 0) {
+        error_report("vhost-user-gpio: get config failed");
+        goto reconnect;
+    }
+
+    return;
+}
+
+static void vu_gpio_device_unrealize(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserGPIO *gpio = VHOST_USER_GPIO(dev);
+
+    vu_gpio_set_status(vdev, 0);
+    qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, NULL, NULL, NULL, NULL,
+                             false);
+    vhost_dev_cleanup(&gpio->vhost_dev);
+    do_vhost_user_cleanup(vdev, gpio);
+}
+
+static const VMStateDescription vu_gpio_vmstate = {
+    .name = "vhost-user-gpio",
+    .unmigratable = 1,
+};
+
+static Property vu_gpio_properties[] = {
+    DEFINE_PROP_CHR("chardev", VHostUserGPIO, chardev),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vu_gpio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+
+    device_class_set_props(dc, vu_gpio_properties);
+    dc->vmsd = &vu_gpio_vmstate;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+    vdc->realize = vu_gpio_device_realize;
+    vdc->unrealize = vu_gpio_device_unrealize;
+    vdc->get_features = vu_gpio_get_features;
+    vdc->get_config = vu_gpio_get_config;
+    vdc->set_status = vu_gpio_set_status;
+    vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
+}
+
+static const TypeInfo vu_gpio_info = {
+    .name = TYPE_VHOST_USER_GPIO,
+    .parent = TYPE_VIRTIO_DEVICE,
+    .instance_size = sizeof(VHostUserGPIO),
+    .class_init = vu_gpio_class_init,
+};
+
+static void vu_gpio_register_types(void)
+{
+    type_register_static(&vu_gpio_info);
+}
+
+type_init(vu_gpio_register_types)
diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index e9ecae1f50..cbfd8c7173 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -80,3 +80,8 @@ config VHOST_USER_FS
     bool
     default y
     depends on VIRTIO && VHOST_USER
+
+config VHOST_USER_GPIO
+    bool
+    default y
+    depends on VIRTIO && VHOST_USER
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 7e8877fd64..33c8e71fab 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -29,6 +29,7 @@ virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
 virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
+virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
 
 virtio_pci_ss = ss.source_set()
 virtio_pci_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-pci.c'))
-- 
2.30.2

Re: [PATCH v2 05/15] hw/virtio: add boilerplate for vhost-user-gpio device

[Vincent Whitchurch]

On Tue, May 24, 2022 at 04:40:46PM +0100, Alex Bennée wrote:
> +static int vu_gpio_start(VirtIODevice *vdev)
> +{
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
> +    int ret, i;
> +
> +    if (!k->set_guest_notifiers) {
> +        error_report("binding does not support guest notifiers");
> +        return -ENOSYS;
> +    }
> +
> +    ret = vhost_dev_enable_notifiers(&gpio->vhost_dev, vdev);
> +    if (ret < 0) {
> +        error_report("Error enabling host notifiers: %d", ret);
> +        return ret;
> +    }
> +
> +    ret = k->set_guest_notifiers(qbus->parent, gpio->vhost_dev.nvqs, true);
> +    if (ret < 0) {
> +        error_report("Error binding guest notifier: %d", ret);
> +        goto err_host_notifiers;
> +    }
> +
> +    /*
> +     * Before we start up we need to ensure we have the final feature
> +     * set needed for the vhost configuration.
> +     */
> +    vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->backend_features);

This is doing the feature handling differently from the other
vhost-user-* implementations, and it doesn't seem to work for me.
Negotiated features (I noticed it with VIRTIO_RING_F_EVENT_IDX) never
make it to VHOST_USER_SET_FEATURES.

If I change this code to match vhost-user-i2c and the other
implementations like in the patch below, it works.

The guest is Linux v5.18.  The backend uses libvhost-user and is the one
posted here (with a few fixes):

 https://lore.kernel.org/lkml/20220311162445.346685-3-vincent.whitchurch@axis.com/

8<-------
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 87e3976880..1dc7af6b03 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -73,7 +73,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
      * Before we start up we need to ensure we have the final feature
      * set needed for the vhost configuration.
      */
-    vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->backend_features);
+    gpio->vhost_dev.acked_features = vdev->guest_features;
 
     ret = vhost_dev_start(&gpio->vhost_dev, vdev);
     if (ret < 0) {
@@ -156,9 +156,7 @@ static uint64_t vu_gpio_get_features(VirtIODevice *vdev, uint64_t features,
     virtio_add_feature(&features, VIRTIO_GPIO_F_IRQ);
     virtio_add_feature(&features, VIRTIO_F_VERSION_1);
 
-    vdev->backend_features = vhost_get_features(&gpio->vhost_dev, feature_bits,
-                                                features);
-    return vdev->backend_features;
+    return vhost_get_features(&gpio->vhost_dev, feature_bits, features);
 }
 
 static void vu_gpio_handle_output(VirtIODevice *vdev, VirtQueue *vq)
-- 
2.28.0

Re: [PATCH v2 05/15] hw/virtio: add boilerplate for vhost-user-gpio device

[Alex Bennée]

Vincent Whitchurch <vincent.whitchurch@axis.com> writes:

> On Tue, May 24, 2022 at 04:40:46PM +0100, Alex Bennée wrote:
>> +static int vu_gpio_start(VirtIODevice *vdev)
>> +{
>> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>> +    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
>> +    int ret, i;
>> +
>> +    if (!k->set_guest_notifiers) {
>> +        error_report("binding does not support guest notifiers");
>> +        return -ENOSYS;
>> +    }
>> +
>> +    ret = vhost_dev_enable_notifiers(&gpio->vhost_dev, vdev);
>> +    if (ret < 0) {
>> +        error_report("Error enabling host notifiers: %d", ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = k->set_guest_notifiers(qbus->parent, gpio->vhost_dev.nvqs, true);
>> +    if (ret < 0) {
>> +        error_report("Error binding guest notifier: %d", ret);
>> +        goto err_host_notifiers;
>> +    }
>> +
>> +    /*
>> +     * Before we start up we need to ensure we have the final feature
>> +     * set needed for the vhost configuration.
>> +     */
>> +    vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->backend_features);
>
> This is doing the feature handling differently from the other
> vhost-user-* implementations, and it doesn't seem to work for me.
> Negotiated features (I noticed it with VIRTIO_RING_F_EVENT_IDX) never
> make it to VHOST_USER_SET_FEATURES.
>
> If I change this code to match vhost-user-i2c and the other
> implementations like in the patch below, it works.

Unfortunately the virtio-i2c backend doesn't need
VHOST_USER_F_PROTOCOL_FEATURES which gets squashed with the bellow
changes which is the cause of the eventual failure in the qos-test:

  # Start of read-guest-mem tests                                                        
  vu_net_set_features: 0                                                                  
  **                                                                                     
  ERROR:../../tests/qtest/vhost-user-test.c:1031:vu_net_set_features: assertion failed: (msg->payload.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES))

which adds to my confusion about the exact route the negotiation of
vhost-user feature bits is meant to make through the code.

>
> The guest is Linux v5.18.  The backend uses libvhost-user and is the one
> posted here (with a few fixes):
>
>  https://lore.kernel.org/lkml/20220311162445.346685-3-vincent.whitchurch@axis.com/
>
> 8<-------
> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index 87e3976880..1dc7af6b03 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -73,7 +73,7 @@ static int vu_gpio_start(VirtIODevice *vdev)
>       * Before we start up we need to ensure we have the final feature
>       * set needed for the vhost configuration.
>       */
> -    vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->backend_features);
> +    gpio->vhost_dev.acked_features = vdev->guest_features;
>  
>      ret = vhost_dev_start(&gpio->vhost_dev, vdev);
>      if (ret < 0) {
> @@ -156,9 +156,7 @@ static uint64_t vu_gpio_get_features(VirtIODevice *vdev, uint64_t features,
>      virtio_add_feature(&features, VIRTIO_GPIO_F_IRQ);
>      virtio_add_feature(&features, VIRTIO_F_VERSION_1);
>  
> -    vdev->backend_features = vhost_get_features(&gpio->vhost_dev, feature_bits,
> -                                                features);
> -    return vdev->backend_features;
> +    return vhost_get_features(&gpio->vhost_dev, feature_bits, features);
>  }
>  
>  static void vu_gpio_handle_output(VirtIODevice *vdev, VirtQueue *vq)


-- 
Alex Bennée

[PATCH v2 06/15] hw/virtio: add vhost-user-gpio-pci boilerplate

[Alex Bennée]

From: Viresh Kumar <viresh.kumar@linaro.org>

This allows is to instantiate a vhost-user-gpio device as part of a PCI
bus. It is mostly boilerplate which looks pretty similar to the
vhost-user-fs-pci device.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <5f560cab92d0d789b1c94295ec74b9952907d69d.1641987128.git.viresh.kumar@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/virtio/vhost-user-gpio-pci.c | 69 +++++++++++++++++++++++++++++++++
 hw/virtio/meson.build           |  1 +
 2 files changed, 70 insertions(+)
 create mode 100644 hw/virtio/vhost-user-gpio-pci.c

diff --git a/hw/virtio/vhost-user-gpio-pci.c b/hw/virtio/vhost-user-gpio-pci.c
new file mode 100644
index 0000000000..b3028a24a1
--- /dev/null
+++ b/hw/virtio/vhost-user-gpio-pci.c
@@ -0,0 +1,69 @@
+/*
+ * Vhost-user gpio virtio device PCI glue
+ *
+ * Copyright (c) 2022 Viresh Kumar <viresh.kumar@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/vhost-user-gpio.h"
+#include "hw/virtio/virtio-pci.h"
+
+struct VHostUserGPIOPCI {
+    VirtIOPCIProxy parent_obj;
+    VHostUserGPIO vdev;
+};
+
+typedef struct VHostUserGPIOPCI VHostUserGPIOPCI;
+
+#define TYPE_VHOST_USER_GPIO_PCI "vhost-user-gpio-pci-base"
+
+DECLARE_INSTANCE_CHECKER(VHostUserGPIOPCI, VHOST_USER_GPIO_PCI,
+                         TYPE_VHOST_USER_GPIO_PCI)
+
+static void vhost_user_gpio_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+    VHostUserGPIOPCI *dev = VHOST_USER_GPIO_PCI(vpci_dev);
+    DeviceState *vdev = DEVICE(&dev->vdev);
+
+    vpci_dev->nvectors = 1;
+    qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
+}
+
+static void vhost_user_gpio_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+    PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+    k->realize = vhost_user_gpio_pci_realize;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+    pcidev_k->device_id = 0; /* Set by virtio-pci based on virtio id */
+    pcidev_k->revision = 0x00;
+    pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
+}
+
+static void vhost_user_gpio_pci_instance_init(Object *obj)
+{
+    VHostUserGPIOPCI *dev = VHOST_USER_GPIO_PCI(obj);
+
+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+                                TYPE_VHOST_USER_GPIO);
+}
+
+static const VirtioPCIDeviceTypeInfo vhost_user_gpio_pci_info = {
+    .base_name = TYPE_VHOST_USER_GPIO_PCI,
+    .non_transitional_name = "vhost-user-gpio-pci",
+    .instance_size = sizeof(VHostUserGPIOPCI),
+    .instance_init = vhost_user_gpio_pci_instance_init,
+    .class_init = vhost_user_gpio_pci_class_init,
+};
+
+static void vhost_user_gpio_pci_register(void)
+{
+    virtio_pci_types_register(&vhost_user_gpio_pci_info);
+}
+
+type_init(vhost_user_gpio_pci_register);
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 33c8e71fab..c14e3db10a 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -30,6 +30,7 @@ virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
 virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
+virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'], if_true: files('vhost-user-gpio-pci.c'))
 
 virtio_pci_ss = ss.source_set()
 virtio_pci_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-pci.c'))
-- 
2.30.2

[PATCH v2 07/15] tests/qtest: pass stdout/stderr down to subtests

[Alex Bennée]

When trying to work out what the virtio-net-tests where doing it was
hard because the g_test_trap_subprocess redirects all output to
/dev/null. Lift this restriction by using the appropriate flags so you
can see something similar to what the vhost-user-blk tests show when
running.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220407150042.2338562-1-alex.bennee@linaro.org>

---
v2
  - keep dumping of CLI behind the g_test_verbose flag
---
 tests/qtest/qos-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index f97d0a08fd..7e1c8fc579 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -185,7 +185,8 @@ static void run_one_test(const void *arg)
 static void subprocess_run_one_test(const void *arg)
 {
     const gchar *path = arg;
-    g_test_trap_subprocess(path, 0, 0);
+    g_test_trap_subprocess(path, 0,
+                           G_TEST_SUBPROCESS_INHERIT_STDOUT | G_TEST_SUBPROCESS_INHERIT_STDERR);
     g_test_trap_assert_passed();
 }
 
-- 
2.30.2

Re: [PATCH v2 07/15] tests/qtest: pass stdout/stderr down to subtests

[Thomas Huth]

On 24/05/2022 17.40, Alex Bennée wrote:
> When trying to work out what the virtio-net-tests where doing it was
> hard because the g_test_trap_subprocess redirects all output to
> /dev/null. Lift this restriction by using the appropriate flags so you
> can see something similar to what the vhost-user-blk tests show when
> running.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20220407150042.2338562-1-alex.bennee@linaro.org>
> 
> ---
> v2
>    - keep dumping of CLI behind the g_test_verbose flag
> ---
>   tests/qtest/qos-test.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
> index f97d0a08fd..7e1c8fc579 100644
> --- a/tests/qtest/qos-test.c
> +++ b/tests/qtest/qos-test.c
> @@ -185,7 +185,8 @@ static void run_one_test(const void *arg)
>   static void subprocess_run_one_test(const void *arg)
>   {
>       const gchar *path = arg;
> -    g_test_trap_subprocess(path, 0, 0);
> +    g_test_trap_subprocess(path, 0,
> +                           G_TEST_SUBPROCESS_INHERIT_STDOUT | G_TEST_SUBPROCESS_INHERIT_STDERR);
>       g_test_trap_assert_passed();
>   }
>   

Acked-by: Thomas Huth <thuth@redhat.com>

[PATCH v2 08/15] tests/qtest: add a timeout for subprocess_run_one_test

[Alex Bennée]

Hangs have been observed in the tests and currently we don't timeout
if a subprocess hangs. Rectify that.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/qos-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 7e1c8fc579..46623da731 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -185,7 +185,7 @@ static void run_one_test(const void *arg)
 static void subprocess_run_one_test(const void *arg)
 {
     const gchar *path = arg;
-    g_test_trap_subprocess(path, 0,
+    g_test_trap_subprocess(path, 60 * G_USEC_PER_SEC,
                            G_TEST_SUBPROCESS_INHERIT_STDOUT | G_TEST_SUBPROCESS_INHERIT_STDERR);
     g_test_trap_assert_passed();
 }
-- 
2.30.2

Re: [PATCH v2 08/15] tests/qtest: add a timeout for subprocess_run_one_test

[Thomas Huth]

On 24/05/2022 17.40, Alex Bennée wrote:
> Hangs have been observed in the tests and currently we don't timeout
> if a subprocess hangs. Rectify that.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/qtest/qos-test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
> index 7e1c8fc579..46623da731 100644
> --- a/tests/qtest/qos-test.c
> +++ b/tests/qtest/qos-test.c
> @@ -185,7 +185,7 @@ static void run_one_test(const void *arg)
>   static void subprocess_run_one_test(const void *arg)
>   {
>       const gchar *path = arg;
> -    g_test_trap_subprocess(path, 0,
> +    g_test_trap_subprocess(path, 60 * G_USEC_PER_SEC,

60 seconds is not that much for a slow test running on a slow and overloaded 
CI host ... maybe rather go for 180 seconds or even more?

  Thomas

[PATCH v2 09/15] tests/qtest: use qos_printf instead of g_test_message

[Alex Bennée]

The vhost-user tests respawn qos-test as a standalone process. As a
result the gtester framework squashes all messages coming out of it
which make it hard to debug. As the test does not care about asserting
certain messages just convert the tests to use the direct qos_printf.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/qos-test.c        |  5 +++++
 tests/qtest/vhost-user-test.c | 13 +++++++------
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/qtest/qos-test.c b/tests/qtest/qos-test.c
index 46623da731..fef2471f8f 100644
--- a/tests/qtest/qos-test.c
+++ b/tests/qtest/qos-test.c
@@ -320,6 +320,11 @@ static void walk_path(QOSGraphNode *orig_path, int len)
 int main(int argc, char **argv, char** envp)
 {
     g_test_init(&argc, &argv, NULL);
+
+    if (g_test_subprocess()) {
+        qos_printf("qos_test running single test in subprocess\n");
+    }
+
     if (g_test_verbose()) {
         qos_printf("ENVIRONMENT VARIABLES: {\n");
         for (char **env = envp; *env != 0; env++) {
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index a2cec87684..90749c147e 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -26,6 +26,7 @@
 #include "libqos/virtio-pci.h"
 
 #include "libqos/malloc-pc.h"
+#include "libqos/qgraph_internal.h"
 #include "hw/virtio/virtio-net.h"
 
 #include "standard-headers/linux/vhost_types.h"
@@ -316,7 +317,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
     }
 
     if (size != VHOST_USER_HDR_SIZE) {
-        g_test_message("Wrong message size received %d", size);
+        qos_printf("%s: Wrong message size received %d\n", __func__, size);
         return;
     }
 
@@ -327,8 +328,8 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         p += VHOST_USER_HDR_SIZE;
         size = qemu_chr_fe_read_all(chr, p, msg.size);
         if (size != msg.size) {
-            g_test_message("Wrong message size received %d != %d",
-                           size, msg.size);
+            qos_printf("%s: Wrong message size received %d != %d\n",
+                       __func__, size, msg.size);
             return;
         }
     }
@@ -450,7 +451,7 @@ static const char *init_hugepagefs(void)
     }
 
     if (access(path, R_OK | W_OK | X_OK)) {
-        g_test_message("access on path (%s): %s", path, strerror(errno));
+        qos_printf("access on path (%s): %s", path, strerror(errno));
         g_test_fail();
         return NULL;
     }
@@ -460,13 +461,13 @@ static const char *init_hugepagefs(void)
     } while (ret != 0 && errno == EINTR);
 
     if (ret != 0) {
-        g_test_message("statfs on path (%s): %s", path, strerror(errno));
+        qos_printf("statfs on path (%s): %s", path, strerror(errno));
         g_test_fail();
         return NULL;
     }
 
     if (fs.f_type != HUGETLBFS_MAGIC) {
-        g_test_message("Warning: path not on HugeTLBFS: %s", path);
+        qos_printf("Warning: path not on HugeTLBFS: %s", path);
         g_test_fail();
         return NULL;
     }
-- 
2.30.2

[PATCH v2 10/15] tests/qtest: catch unhandled vhost-user messages

[Alex Bennée]

We don't need to action every message but lets document the ones we
are expecting to consume so future tests don't get confused about
unhandled bits.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v1
  - drop g_test_fail() when we get unexpected result, that just hangs
---
 tests/qtest/vhost-user-test.c | 40 +++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 90749c147e..11da6ff07a 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -358,12 +358,41 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         }
         break;
 
+    case VHOST_USER_SET_OWNER:
+        /*
+         * We don't need to do anything here, the remote is just
+         * letting us know it is in charge. Just log it.
+         */
+        qos_printf("set_owner: start of session\n");
+        break;
+
     case VHOST_USER_GET_PROTOCOL_FEATURES:
         if (s->vu_ops->get_protocol_features) {
             s->vu_ops->get_protocol_features(s, chr, &msg);
         }
         break;
 
+    case VHOST_USER_SET_PROTOCOL_FEATURES:
+        /*
+         * We did set VHOST_USER_F_PROTOCOL_FEATURES so its valid for
+         * the remote end to send this. There is no handshake reply so
+         * just log the details for debugging.
+         */
+        qos_printf("set_protocol_features: 0x%"PRIx64 "\n", msg.payload.u64);
+        break;
+
+        /*
+         * A real vhost-user backend would actually set the size and
+         * address of the vrings but we can simply report them.
+         */
+    case VHOST_USER_SET_VRING_NUM:
+        qos_printf("set_vring_num: %d/%d\n",
+                   msg.payload.state.index, msg.payload.state.num);
+        break;
+    case VHOST_USER_SET_VRING_ADDR:
+        qos_printf("set_vring_addr:\n");
+        break;
+
     case VHOST_USER_GET_VRING_BASE:
         /* send back vring base to qemu */
         msg.flags |= VHOST_USER_REPLY_MASK;
@@ -428,7 +457,18 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
 
+    case VHOST_USER_SET_VRING_ENABLE:
+        /*
+         * Another case we ignore as we don't need to respond. With a
+         * fully functioning vhost-user we would enable/disable the
+         * vring monitoring.
+         */
+        qos_printf("set_vring(%d)=%s\n", msg.payload.state.index,
+                   msg.payload.state.num ? "enabled" : "disabled");
+        break;
+
     default:
+        qos_printf("vhost-user: un-handled message: %d\n", msg.request);
         break;
     }
 
-- 
2.30.2

[PATCH v2 11/15] tests/qtest: use g_autofree for test_server_create_chr

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/vhost-user-test.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 11da6ff07a..d0fa034601 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -565,14 +565,13 @@ static void chr_event(void *opaque, QEMUChrEvent event)
 
 static void test_server_create_chr(TestServer *server, const gchar *opt)
 {
-    gchar *chr_path;
+    g_autofree gchar *chr_path = g_strdup_printf("unix:%s%s",
+                                                 server->socket_path, opt);
     Chardev *chr;
 
-    chr_path = g_strdup_printf("unix:%s%s", server->socket_path, opt);
     chr = qemu_chr_new(server->chr_name, chr_path, server->context);
-    g_free(chr_path);
+    g_assert(chr);
 
-    g_assert_nonnull(chr);
     qemu_chr_fe_init(&server->chr, chr, &error_abort);
     qemu_chr_fe_set_handlers(&server->chr, chr_can_read, chr_read,
                              chr_event, NULL, server, server->context, true);
-- 
2.30.2

[PATCH v2 12/15] tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES

[Alex Bennée]

checkpatch.pl warns that non-plain asserts should be avoided so
convert the check to a plain g_assert.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/vhost-user-test.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index d0fa034601..db18e0b664 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -980,8 +980,7 @@ static void test_multiqueue(void *obj, void *arg, QGuestAllocator *alloc)
 static void vu_net_set_features(TestServer *s, CharBackend *chr,
         VhostUserMsg *msg)
 {
-    g_assert_cmpint(msg->payload.u64 &
-            (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES), !=, 0ULL);
+    g_assert(msg->payload.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
     if (s->test_flags == TEST_FLAGS_DISCONNECT) {
         qemu_chr_fe_disconnect(chr);
         s->test_flags = TEST_FLAGS_BAD;
-- 
2.30.2

Re: [PATCH v2 12/15] tests/qtest: plain g_assert for VHOST_USER_F_PROTOCOL_FEATURES

[Thomas Huth]

On 24/05/2022 17.40, Alex Bennée wrote:
> checkpatch.pl warns that non-plain asserts should be avoided so
> convert the check to a plain g_assert.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/qtest/vhost-user-test.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
> index d0fa034601..db18e0b664 100644
> --- a/tests/qtest/vhost-user-test.c
> +++ b/tests/qtest/vhost-user-test.c
> @@ -980,8 +980,7 @@ static void test_multiqueue(void *obj, void *arg, QGuestAllocator *alloc)
>   static void vu_net_set_features(TestServer *s, CharBackend *chr,
>           VhostUserMsg *msg)
>   {
> -    g_assert_cmpint(msg->payload.u64 &
> -            (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES), !=, 0ULL);
> +    g_assert(msg->payload.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
>       if (s->test_flags == TEST_FLAGS_DISCONNECT) {
>           qemu_chr_fe_disconnect(chr);
>           s->test_flags = TEST_FLAGS_BAD;

Why this? commit 6e9389563 says that this should not trigger for code in 
tests/ , so I wonder why you ran into this checkpatch warning?

  Thomas

[PATCH v2 13/15] tests/qtest: implement stub for VHOST_USER_GET_CONFIG

[Alex Bennée]

We don't implement the full solution because frankly none of the tests
need to at the moment. We may end up re-implementing libvhostuser in
the end.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/vhost-user-test.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index db18e0b664..d546721f5d 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -79,6 +79,8 @@ typedef enum VhostUserRequest {
     VHOST_USER_SET_PROTOCOL_FEATURES = 16,
     VHOST_USER_GET_QUEUE_NUM = 17,
     VHOST_USER_SET_VRING_ENABLE = 18,
+    VHOST_USER_GET_CONFIG = 24,
+    VHOST_USER_SET_CONFIG = 25,
     VHOST_USER_MAX
 } VhostUserRequest;
 
@@ -372,6 +374,17 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         }
         break;
 
+    case VHOST_USER_GET_CONFIG:
+        /*
+         * Treat GET_CONFIG as a NOP and just reply and let the guest
+         * consider we have updated its memory. Tests currently don't
+         * require working configs.
+         */
+        msg.flags |= VHOST_USER_REPLY_MASK;
+        p = (uint8_t *) &msg;
+        qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+        break;
+
     case VHOST_USER_SET_PROTOCOL_FEATURES:
         /*
          * We did set VHOST_USER_F_PROTOCOL_FEATURES so its valid for
-- 
2.30.2

[PATCH v2 14/15] tests/qtest: add a get_features op to vhost-user-test

[Alex Bennée]

As we expand this test for more virtio devices we will need to support
different feature sets. Add a mandatory op field to fetch the list of
features needed for the test itself.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/vhost-user-test.c | 37 +++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index d546721f5d..28b4cf28ec 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -171,10 +171,11 @@ struct vhost_user_ops {
             const char *chr_opts);
 
     /* VHOST-USER commands. */
+    uint64_t (*get_features)(TestServer *s);
     void (*set_features)(TestServer *s, CharBackend *chr,
-            VhostUserMsg *msg);
+                         VhostUserMsg *msg);
     void (*get_protocol_features)(TestServer *s,
-            CharBackend *chr, VhostUserMsg *msg);
+                                  CharBackend *chr, VhostUserMsg *msg);
 };
 
 static const char *init_hugepagefs(void);
@@ -338,20 +339,22 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
 
     switch (msg.request) {
     case VHOST_USER_GET_FEATURES:
+        /* Mandatory for tests to define get_features */
+        g_assert(s->vu_ops->get_features);
+
         /* send back features to qemu */
         msg.flags |= VHOST_USER_REPLY_MASK;
         msg.size = sizeof(m.payload.u64);
-        msg.payload.u64 = 0x1ULL << VHOST_F_LOG_ALL |
-            0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
-        if (s->queues > 1) {
-            msg.payload.u64 |= 0x1ULL << VIRTIO_NET_F_MQ;
-        }
+
         if (s->test_flags >= TEST_FLAGS_BAD) {
             msg.payload.u64 = 0;
             s->test_flags = TEST_FLAGS_END;
+        } else {
+            msg.payload.u64 = s->vu_ops->get_features(s);
         }
-        p = (uint8_t *) &msg;
-        qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+
+        qemu_chr_fe_write_all(chr, (uint8_t *) &msg,
+                              VHOST_USER_HDR_SIZE + msg.size);
         break;
 
     case VHOST_USER_SET_FEATURES:
@@ -990,8 +993,21 @@ static void test_multiqueue(void *obj, void *arg, QGuestAllocator *alloc)
     wait_for_rings_started(s, s->queues * 2);
 }
 
+
+static uint64_t vu_net_get_features(TestServer *s)
+{
+    uint64_t features = 0x1ULL << VHOST_F_LOG_ALL |
+        0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+
+    if (s->queues > 1) {
+        features |= 0x1ULL << VIRTIO_NET_F_MQ;
+    }
+
+    return features;
+}
+
 static void vu_net_set_features(TestServer *s, CharBackend *chr,
-        VhostUserMsg *msg)
+                                VhostUserMsg *msg)
 {
     g_assert(msg->payload.u64 & (0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES));
     if (s->test_flags == TEST_FLAGS_DISCONNECT) {
@@ -1020,6 +1036,7 @@ static struct vhost_user_ops g_vu_net_ops = {
 
     .append_opts = append_vhost_net_opts,
 
+    .get_features = vu_net_get_features,
     .set_features = vu_net_set_features,
     .get_protocol_features = vu_net_get_protocol_features,
 };
-- 
2.30.2

[PATCH v2 15/15] tests/qtest: enable tests for virtio-gpio

[Alex Bennée]

We don't have a virtio-gpio implementation in QEMU and only
support a vhost-user backend. The QEMU side of the code is minimal so
it should be enough to instantiate the device and pass some vhost-user
messages over the control socket. To do this we hook into the existing
vhost-user-test code and just add the bits required for gpio.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
Message-Id: <20220408155704.2777166-1-alex.bennee@linaro.org>

---
v2
  - add more of the missing boilerplate
  - don't request LOG_SHMD
  - use get_features op
  - report VIRTIO_F_VERSION_1
  - more comments
---
 tests/qtest/libqos/virtio-gpio.h |  35 +++++++
 tests/qtest/libqos/virtio-gpio.c | 171 +++++++++++++++++++++++++++++++
 tests/qtest/libqos/virtio.c      |   2 +-
 tests/qtest/vhost-user-test.c    |  66 ++++++++++++
 tests/qtest/libqos/meson.build   |   1 +
 5 files changed, 274 insertions(+), 1 deletion(-)
 create mode 100644 tests/qtest/libqos/virtio-gpio.h
 create mode 100644 tests/qtest/libqos/virtio-gpio.c

diff --git a/tests/qtest/libqos/virtio-gpio.h b/tests/qtest/libqos/virtio-gpio.h
new file mode 100644
index 0000000000..f11d41bd19
--- /dev/null
+++ b/tests/qtest/libqos/virtio-gpio.h
@@ -0,0 +1,35 @@
+/*
+ * virtio-gpio structures
+ *
+ * Copyright (c) 2022 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef TESTS_LIBQOS_VIRTIO_GPIO_H
+#define TESTS_LIBQOS_VIRTIO_GPIO_H
+
+#include "qgraph.h"
+#include "virtio.h"
+#include "virtio-pci.h"
+
+typedef struct QVhostUserGPIO QVhostUserGPIO;
+typedef struct QVhostUserGPIOPCI QVhostUserGPIOPCI;
+typedef struct QVhostUserGPIODevice QVhostUserGPIODevice;
+
+struct QVhostUserGPIO {
+    QVirtioDevice *vdev;
+    QVirtQueue **queues;
+};
+
+struct QVhostUserGPIOPCI {
+    QVirtioPCIDevice pci_vdev;
+    QVhostUserGPIO gpio;
+};
+
+struct QVhostUserGPIODevice {
+    QOSGraphObject obj;
+    QVhostUserGPIO gpio;
+};
+
+#endif
diff --git a/tests/qtest/libqos/virtio-gpio.c b/tests/qtest/libqos/virtio-gpio.c
new file mode 100644
index 0000000000..762aa6695b
--- /dev/null
+++ b/tests/qtest/libqos/virtio-gpio.c
@@ -0,0 +1,171 @@
+/*
+ * virtio-gpio nodes for testing
+ *
+ * Copyright (c) 2022 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "standard-headers/linux/virtio_config.h"
+#include "../libqtest.h"
+#include "qemu/module.h"
+#include "qgraph.h"
+#include "virtio-gpio.h"
+
+static QGuestAllocator *alloc;
+
+static void virtio_gpio_cleanup(QVhostUserGPIO *gpio)
+{
+    QVirtioDevice *vdev = gpio->vdev;
+    int i;
+
+    for (i = 0; i < 2; i++) {
+        qvirtqueue_cleanup(vdev->bus, gpio->queues[i], alloc);
+    }
+    g_free(gpio->queues);
+}
+
+/*
+ * This handles the VirtIO setup from the point of view of the driver
+ * frontend and therefor doesn't present any vhost specific features
+ * and in fact masks of the re-used bit.
+ */
+static void virtio_gpio_setup(QVhostUserGPIO *gpio)
+{
+    QVirtioDevice *vdev = gpio->vdev;
+    uint64_t features;
+    int i;
+
+    features = qvirtio_get_features(vdev);
+    features &= ~QVIRTIO_F_BAD_FEATURE;
+    qvirtio_set_features(vdev, features);
+
+    gpio->queues = g_new(QVirtQueue *, 2);
+    for (i = 0; i < 2; i++) {
+        gpio->queues[i] = qvirtqueue_setup(vdev, alloc, i);
+    }
+    qvirtio_set_driver_ok(vdev);
+}
+
+static void *qvirtio_gpio_get_driver(QVhostUserGPIO *v_gpio,
+                                     const char *interface)
+{
+    if (!g_strcmp0(interface, "vhost-user-gpio")) {
+        return v_gpio;
+    }
+    if (!g_strcmp0(interface, "virtio")) {
+        return v_gpio->vdev;
+    }
+
+    g_assert_not_reached();
+}
+
+static void *qvirtio_gpio_device_get_driver(void *object,
+                                            const char *interface)
+{
+    QVhostUserGPIODevice *v_gpio = object;
+    return qvirtio_gpio_get_driver(&v_gpio->gpio, interface);
+}
+
+/* virtio-gpio (mmio) */
+static void qvirtio_gpio_device_destructor(QOSGraphObject *obj)
+{
+    QVhostUserGPIODevice *gpio_dev = (QVhostUserGPIODevice *) obj;
+    virtio_gpio_cleanup(&gpio_dev->gpio);
+}
+
+static void qvirtio_gpio_device_start_hw(QOSGraphObject *obj)
+{
+    QVhostUserGPIODevice *gpio_dev = (QVhostUserGPIODevice *) obj;
+    virtio_gpio_setup(&gpio_dev->gpio);
+}
+
+static void *virtio_gpio_device_create(void *virtio_dev,
+                                       QGuestAllocator *t_alloc,
+                                       void *addr)
+{
+    QVhostUserGPIODevice *virtio_device = g_new0(QVhostUserGPIODevice, 1);
+    QVhostUserGPIO *interface = &virtio_device->gpio;
+
+    interface->vdev = virtio_dev;
+    alloc = t_alloc;
+
+    virtio_device->obj.get_driver = qvirtio_gpio_device_get_driver;
+    virtio_device->obj.start_hw = qvirtio_gpio_device_start_hw;
+    virtio_device->obj.destructor = qvirtio_gpio_device_destructor;
+
+    return &virtio_device->obj;
+}
+
+/* virtio-gpio-pci */
+static void qvirtio_gpio_pci_destructor(QOSGraphObject *obj)
+{
+    QVhostUserGPIOPCI *gpio_pci = (QVhostUserGPIOPCI *) obj;
+    QOSGraphObject *pci_vobj =  &gpio_pci->pci_vdev.obj;
+
+    virtio_gpio_cleanup(&gpio_pci->gpio);
+    qvirtio_pci_destructor(pci_vobj);
+}
+
+static void qvirtio_gpio_pci_start_hw(QOSGraphObject *obj)
+{
+    QVhostUserGPIOPCI *gpio_pci = (QVhostUserGPIOPCI *) obj;
+    QOSGraphObject *pci_vobj =  &gpio_pci->pci_vdev.obj;
+
+    qvirtio_pci_start_hw(pci_vobj);
+    virtio_gpio_setup(&gpio_pci->gpio);
+}
+
+static void *qvirtio_gpio_pci_get_driver(void *object, const char *interface)
+{
+    QVhostUserGPIOPCI *v_gpio = object;
+
+    if (!g_strcmp0(interface, "pci-device")) {
+        return v_gpio->pci_vdev.pdev;
+    }
+    return qvirtio_gpio_get_driver(&v_gpio->gpio, interface);
+}
+
+static void *virtio_gpio_pci_create(void *pci_bus, QGuestAllocator *t_alloc,
+                                    void *addr)
+{
+    QVhostUserGPIOPCI *virtio_spci = g_new0(QVhostUserGPIOPCI, 1);
+    QVhostUserGPIO *interface = &virtio_spci->gpio;
+    QOSGraphObject *obj = &virtio_spci->pci_vdev.obj;
+
+    virtio_pci_init(&virtio_spci->pci_vdev, pci_bus, addr);
+    interface->vdev = &virtio_spci->pci_vdev.vdev;
+    alloc = t_alloc;
+
+    obj->get_driver = qvirtio_gpio_pci_get_driver;
+    obj->start_hw = qvirtio_gpio_pci_start_hw;
+    obj->destructor = qvirtio_gpio_pci_destructor;
+
+    return obj;
+}
+
+static void virtio_gpio_register_nodes(void)
+{
+    QPCIAddress addr = {
+        .devfn = QPCI_DEVFN(4, 0),
+    };
+
+    QOSGraphEdgeOptions edge_opts = { };
+
+    /* vhost-user-gpio-device */
+    edge_opts.extra_device_opts = "id=gpio0,chardev=chr-vhost-user-test";
+    qos_node_create_driver("vhost-user-gpio-device",
+                            virtio_gpio_device_create);
+    qos_node_consumes("vhost-user-gpio-device", "virtio-bus", &edge_opts);
+    qos_node_produces("vhost-user-gpio-device", "vhost-user-gpio");
+
+    /* virtio-gpio-pci */
+    edge_opts.extra_device_opts = "id=gpio0,addr=04.0,chardev=chr-vhost-user-test";
+    add_qpci_address(&edge_opts, &addr);
+    qos_node_create_driver("vhost-user-gpio-pci", virtio_gpio_pci_create);
+    qos_node_consumes("vhost-user-gpio-pci", "pci-bus", &edge_opts);
+    qos_node_produces("vhost-user-gpio-pci", "vhost-user-gpio");
+}
+
+libqos_init(virtio_gpio_register_nodes);
diff --git a/tests/qtest/libqos/virtio.c b/tests/qtest/libqos/virtio.c
index 09ec09b655..d6fc4f5a29 100644
--- a/tests/qtest/libqos/virtio.c
+++ b/tests/qtest/libqos/virtio.c
@@ -108,7 +108,7 @@ void qvirtio_set_features(QVirtioDevice *d, uint64_t features)
      * This could be a separate function for drivers that want to access
      * configuration space before setting FEATURES_OK, but no existing users
      * need that and it's less code for callers if this is done implicitly.
-    */
+     */
     if (features & (1ull << VIRTIO_F_VERSION_1)) {
         uint8_t status = d->bus->get_status(d) |
                          VIRTIO_CONFIG_S_FEATURES_OK;
diff --git a/tests/qtest/vhost-user-test.c b/tests/qtest/vhost-user-test.c
index 28b4cf28ec..04950e8458 100644
--- a/tests/qtest/vhost-user-test.c
+++ b/tests/qtest/vhost-user-test.c
@@ -32,6 +32,7 @@
 #include "standard-headers/linux/vhost_types.h"
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_net.h"
+#include "standard-headers/linux/virtio_gpio.h"
 
 #ifdef CONFIG_LINUX
 #include <sys/vfs.h>
@@ -53,9 +54,12 @@
 #define VHOST_MAX_VIRTQUEUES    0x100
 
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VIRTIO_F_VERSION_1 32
+
 #define VHOST_USER_PROTOCOL_F_MQ 0
 #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1
 #define VHOST_USER_PROTOCOL_F_CROSS_ENDIAN   6
+#define VHOST_USER_PROTOCOL_F_CONFIG 9
 
 #define VHOST_LOG_PAGE 0x1000
 
@@ -140,6 +144,7 @@ enum {
 
 enum {
     VHOST_USER_NET,
+    VHOST_USER_GPIO,
 };
 
 typedef struct TestServer {
@@ -198,6 +203,19 @@ static void append_vhost_net_opts(TestServer *s, GString *cmd_line,
                            chr_opts, s->chr_name);
 }
 
+/*
+ * For GPIO there are no other magic devices we need to add (like
+ * block or netdev) so all we need to worry about is the vhost-user
+ * chardev socket.
+ */
+static void append_vhost_gpio_opts(TestServer *s, GString *cmd_line,
+                             const char *chr_opts)
+{
+    g_string_append_printf(cmd_line, QEMU_CMD_CHR,
+                           s->chr_name, s->socket_path,
+                           chr_opts);
+}
+
 static void append_mem_opts(TestServer *server, GString *cmd_line,
                             int size, enum test_memfd memfd)
 {
@@ -1085,3 +1103,51 @@ static void register_vhost_user_test(void)
                  test_multiqueue, &opts);
 }
 libqos_init(register_vhost_user_test);
+
+static uint64_t vu_gpio_get_features(TestServer *s)
+{
+    return 0x1ULL << VIRTIO_F_VERSION_1 |
+        0x1ULL << VIRTIO_GPIO_F_IRQ |
+        0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+}
+
+/*
+ * This stub can't handle all the message types but we should reply
+ * that we support VHOST_USER_PROTOCOL_F_CONFIG as gpio would use it
+ * talking to a read vhost-user daemon.
+ */
+static void vu_gpio_get_protocol_features(TestServer *s, CharBackend *chr,
+                                          VhostUserMsg *msg)
+{
+    /* send back features to qemu */
+    msg->flags |= VHOST_USER_REPLY_MASK;
+    msg->size = sizeof(m.payload.u64);
+    msg->payload.u64 = 1ULL << VHOST_USER_PROTOCOL_F_CONFIG;
+
+    qemu_chr_fe_write_all(chr, (uint8_t *)msg, VHOST_USER_HDR_SIZE + msg->size);
+}
+
+static struct vhost_user_ops g_vu_gpio_ops = {
+    .type = VHOST_USER_GPIO,
+
+    .append_opts = append_vhost_gpio_opts,
+
+    .get_features = vu_gpio_get_features,
+    .set_features = vu_net_set_features,
+    .get_protocol_features = vu_gpio_get_protocol_features,
+};
+
+static void register_vhost_gpio_test(void)
+{
+    QOSGraphTestOptions opts = {
+        .before = vhost_user_test_setup,
+        .subprocess = true,
+        .arg = &g_vu_gpio_ops,
+    };
+
+    qemu_add_opts(&qemu_chardev_opts);
+
+    qos_add_test("read-guest-mem/memfile",
+                 "vhost-user-gpio", test_read_guest_mem, &opts);
+}
+libqos_init(register_vhost_gpio_test);
diff --git a/tests/qtest/libqos/meson.build b/tests/qtest/libqos/meson.build
index fd5d6e5ae1..9dc815ddd4 100644
--- a/tests/qtest/libqos/meson.build
+++ b/tests/qtest/libqos/meson.build
@@ -45,6 +45,7 @@ libqos_srcs = files(
         'virtio-scsi.c',
         'virtio-serial.c',
         'virtio-iommu.c',
+        'virtio-gpio.c',
         'generic-pcihost.c',
 
         # qgraph machines:
-- 
2.30.2

Re: [PATCH v2 15/15] tests/qtest: enable tests for virtio-gpio

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3923 bytes --]

On Tue, May 24, 2022 at 04:40:56PM +0100, Alex Bennée wrote:
> We don't have a virtio-gpio implementation in QEMU and only
> support a vhost-user backend. The QEMU side of the code is minimal so
> it should be enough to instantiate the device and pass some vhost-user
> messages over the control socket. To do this we hook into the existing
> vhost-user-test code and just add the bits required for gpio.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Message-Id: <20220408155704.2777166-1-alex.bennee@linaro.org>
> 
> ---
> v2
>   - add more of the missing boilerplate
>   - don't request LOG_SHMD
>   - use get_features op
>   - report VIRTIO_F_VERSION_1
>   - more comments
> ---
>  tests/qtest/libqos/virtio-gpio.h |  35 +++++++
>  tests/qtest/libqos/virtio-gpio.c | 171 +++++++++++++++++++++++++++++++
>  tests/qtest/libqos/virtio.c      |   2 +-
>  tests/qtest/vhost-user-test.c    |  66 ++++++++++++
>  tests/qtest/libqos/meson.build   |   1 +
>  5 files changed, 274 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qtest/libqos/virtio-gpio.h
>  create mode 100644 tests/qtest/libqos/virtio-gpio.c
> 
> diff --git a/tests/qtest/libqos/virtio-gpio.h b/tests/qtest/libqos/virtio-gpio.h
> new file mode 100644
> index 0000000000..f11d41bd19
> --- /dev/null
> +++ b/tests/qtest/libqos/virtio-gpio.h
> @@ -0,0 +1,35 @@
> +/*
> + * virtio-gpio structures
> + *
> + * Copyright (c) 2022 Linaro Ltd
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef TESTS_LIBQOS_VIRTIO_GPIO_H
> +#define TESTS_LIBQOS_VIRTIO_GPIO_H
> +
> +#include "qgraph.h"
> +#include "virtio.h"
> +#include "virtio-pci.h"
> +
> +typedef struct QVhostUserGPIO QVhostUserGPIO;
> +typedef struct QVhostUserGPIOPCI QVhostUserGPIOPCI;
> +typedef struct QVhostUserGPIODevice QVhostUserGPIODevice;
> +
> +struct QVhostUserGPIO {
> +    QVirtioDevice *vdev;
> +    QVirtQueue **queues;
> +};
> +
> +struct QVhostUserGPIOPCI {
> +    QVirtioPCIDevice pci_vdev;
> +    QVhostUserGPIO gpio;
> +};
> +
> +struct QVhostUserGPIODevice {
> +    QOSGraphObject obj;
> +    QVhostUserGPIO gpio;
> +};
> +
> +#endif
> diff --git a/tests/qtest/libqos/virtio-gpio.c b/tests/qtest/libqos/virtio-gpio.c
> new file mode 100644
> index 0000000000..762aa6695b
> --- /dev/null
> +++ b/tests/qtest/libqos/virtio-gpio.c
> @@ -0,0 +1,171 @@
> +/*
> + * virtio-gpio nodes for testing
> + *
> + * Copyright (c) 2022 Linaro Ltd
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "standard-headers/linux/virtio_config.h"
> +#include "../libqtest.h"
> +#include "qemu/module.h"
> +#include "qgraph.h"
> +#include "virtio-gpio.h"
> +
> +static QGuestAllocator *alloc;
> +
> +static void virtio_gpio_cleanup(QVhostUserGPIO *gpio)
> +{
> +    QVirtioDevice *vdev = gpio->vdev;
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        qvirtqueue_cleanup(vdev->bus, gpio->queues[i], alloc);
> +    }
> +    g_free(gpio->queues);
> +}
> +
> +/*
> + * This handles the VirtIO setup from the point of view of the driver
> + * frontend and therefor doesn't present any vhost specific features
> + * and in fact masks of the re-used bit.
> + */
> +static void virtio_gpio_setup(QVhostUserGPIO *gpio)
> +{
> +    QVirtioDevice *vdev = gpio->vdev;
> +    uint64_t features;
> +    int i;
> +
> +    features = qvirtio_get_features(vdev);
> +    features &= ~QVIRTIO_F_BAD_FEATURE;

This looks questionable. qvirtio_get_features() should return VIRTIO
feature bits. Is QVIRTIO_F_BAD_FEATURE masked out here because
qvirtio_get_features() is returning raw vhost-user feature bits instead
and you want to get rid of VHOST_USER_F_PROTOCOL_FEATURES?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 15/15] tests/qtest: enable tests for virtio-gpio

[Alex Bennée]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> [[PGP Signed Part:Undecided]]
> On Tue, May 24, 2022 at 04:40:56PM +0100, Alex Bennée wrote:
>> We don't have a virtio-gpio implementation in QEMU and only
>> support a vhost-user backend. The QEMU side of the code is minimal so
>> it should be enough to instantiate the device and pass some vhost-user
>> messages over the control socket. To do this we hook into the existing
>> vhost-user-test code and just add the bits required for gpio.
>> 
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Eric Auger <eric.auger@redhat.com>
>> Message-Id: <20220408155704.2777166-1-alex.bennee@linaro.org>
>> 
>> ---
>> v2
>>   - add more of the missing boilerplate
>>   - don't request LOG_SHMD
>>   - use get_features op
>>   - report VIRTIO_F_VERSION_1
>>   - more comments
>> ---
>>  tests/qtest/libqos/virtio-gpio.h |  35 +++++++
>>  tests/qtest/libqos/virtio-gpio.c | 171 +++++++++++++++++++++++++++++++
>>  tests/qtest/libqos/virtio.c      |   2 +-
>>  tests/qtest/vhost-user-test.c    |  66 ++++++++++++
>>  tests/qtest/libqos/meson.build   |   1 +
>>  5 files changed, 274 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/qtest/libqos/virtio-gpio.h
>>  create mode 100644 tests/qtest/libqos/virtio-gpio.c
>> 
>> diff --git a/tests/qtest/libqos/virtio-gpio.h b/tests/qtest/libqos/virtio-gpio.h
>> new file mode 100644
>> index 0000000000..f11d41bd19
>> --- /dev/null
>> +++ b/tests/qtest/libqos/virtio-gpio.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * virtio-gpio structures
>> + *
>> + * Copyright (c) 2022 Linaro Ltd
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#ifndef TESTS_LIBQOS_VIRTIO_GPIO_H
>> +#define TESTS_LIBQOS_VIRTIO_GPIO_H
>> +
>> +#include "qgraph.h"
>> +#include "virtio.h"
>> +#include "virtio-pci.h"
>> +
>> +typedef struct QVhostUserGPIO QVhostUserGPIO;
>> +typedef struct QVhostUserGPIOPCI QVhostUserGPIOPCI;
>> +typedef struct QVhostUserGPIODevice QVhostUserGPIODevice;
>> +
>> +struct QVhostUserGPIO {
>> +    QVirtioDevice *vdev;
>> +    QVirtQueue **queues;
>> +};
>> +
>> +struct QVhostUserGPIOPCI {
>> +    QVirtioPCIDevice pci_vdev;
>> +    QVhostUserGPIO gpio;
>> +};
>> +
>> +struct QVhostUserGPIODevice {
>> +    QOSGraphObject obj;
>> +    QVhostUserGPIO gpio;
>> +};
>> +
>> +#endif
>> diff --git a/tests/qtest/libqos/virtio-gpio.c b/tests/qtest/libqos/virtio-gpio.c
>> new file mode 100644
>> index 0000000000..762aa6695b
>> --- /dev/null
>> +++ b/tests/qtest/libqos/virtio-gpio.c
>> @@ -0,0 +1,171 @@
>> +/*
>> + * virtio-gpio nodes for testing
>> + *
>> + * Copyright (c) 2022 Linaro Ltd
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "standard-headers/linux/virtio_config.h"
>> +#include "../libqtest.h"
>> +#include "qemu/module.h"
>> +#include "qgraph.h"
>> +#include "virtio-gpio.h"
>> +
>> +static QGuestAllocator *alloc;
>> +
>> +static void virtio_gpio_cleanup(QVhostUserGPIO *gpio)
>> +{
>> +    QVirtioDevice *vdev = gpio->vdev;
>> +    int i;
>> +
>> +    for (i = 0; i < 2; i++) {
>> +        qvirtqueue_cleanup(vdev->bus, gpio->queues[i], alloc);
>> +    }
>> +    g_free(gpio->queues);
>> +}
>> +
>> +/*
>> + * This handles the VirtIO setup from the point of view of the driver
>> + * frontend and therefor doesn't present any vhost specific features
>> + * and in fact masks of the re-used bit.
>> + */
>> +static void virtio_gpio_setup(QVhostUserGPIO *gpio)
>> +{
>> +    QVirtioDevice *vdev = gpio->vdev;
>> +    uint64_t features;
>> +    int i;
>> +
>> +    features = qvirtio_get_features(vdev);
>> +    features &= ~QVIRTIO_F_BAD_FEATURE;
>
> This looks questionable. qvirtio_get_features() should return VIRTIO
> feature bits. Is QVIRTIO_F_BAD_FEATURE masked out here because
> qvirtio_get_features() is returning raw vhost-user feature bits instead
> and you want to get rid of VHOST_USER_F_PROTOCOL_FEATURES?

Well it's an invalid bit for the driver/frontend<->hw/backend path -
although maybe we should error if we saw it?

>
> [[End of PGP Signed Part]]


-- 
Alex Bennée

Re: [PATCH v2 15/15] tests/qtest: enable tests for virtio-gpio

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5006 bytes --]

On Wed, May 25, 2022 at 11:35:53PM +0100, Alex Bennée wrote:
> 
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > [[PGP Signed Part:Undecided]]
> > On Tue, May 24, 2022 at 04:40:56PM +0100, Alex Bennée wrote:
> >> We don't have a virtio-gpio implementation in QEMU and only
> >> support a vhost-user backend. The QEMU side of the code is minimal so
> >> it should be enough to instantiate the device and pass some vhost-user
> >> messages over the control socket. To do this we hook into the existing
> >> vhost-user-test code and just add the bits required for gpio.
> >> 
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Eric Auger <eric.auger@redhat.com>
> >> Message-Id: <20220408155704.2777166-1-alex.bennee@linaro.org>
> >> 
> >> ---
> >> v2
> >>   - add more of the missing boilerplate
> >>   - don't request LOG_SHMD
> >>   - use get_features op
> >>   - report VIRTIO_F_VERSION_1
> >>   - more comments
> >> ---
> >>  tests/qtest/libqos/virtio-gpio.h |  35 +++++++
> >>  tests/qtest/libqos/virtio-gpio.c | 171 +++++++++++++++++++++++++++++++
> >>  tests/qtest/libqos/virtio.c      |   2 +-
> >>  tests/qtest/vhost-user-test.c    |  66 ++++++++++++
> >>  tests/qtest/libqos/meson.build   |   1 +
> >>  5 files changed, 274 insertions(+), 1 deletion(-)
> >>  create mode 100644 tests/qtest/libqos/virtio-gpio.h
> >>  create mode 100644 tests/qtest/libqos/virtio-gpio.c
> >> 
> >> diff --git a/tests/qtest/libqos/virtio-gpio.h b/tests/qtest/libqos/virtio-gpio.h
> >> new file mode 100644
> >> index 0000000000..f11d41bd19
> >> --- /dev/null
> >> +++ b/tests/qtest/libqos/virtio-gpio.h
> >> @@ -0,0 +1,35 @@
> >> +/*
> >> + * virtio-gpio structures
> >> + *
> >> + * Copyright (c) 2022 Linaro Ltd
> >> + *
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + */
> >> +
> >> +#ifndef TESTS_LIBQOS_VIRTIO_GPIO_H
> >> +#define TESTS_LIBQOS_VIRTIO_GPIO_H
> >> +
> >> +#include "qgraph.h"
> >> +#include "virtio.h"
> >> +#include "virtio-pci.h"
> >> +
> >> +typedef struct QVhostUserGPIO QVhostUserGPIO;
> >> +typedef struct QVhostUserGPIOPCI QVhostUserGPIOPCI;
> >> +typedef struct QVhostUserGPIODevice QVhostUserGPIODevice;
> >> +
> >> +struct QVhostUserGPIO {
> >> +    QVirtioDevice *vdev;
> >> +    QVirtQueue **queues;
> >> +};
> >> +
> >> +struct QVhostUserGPIOPCI {
> >> +    QVirtioPCIDevice pci_vdev;
> >> +    QVhostUserGPIO gpio;
> >> +};
> >> +
> >> +struct QVhostUserGPIODevice {
> >> +    QOSGraphObject obj;
> >> +    QVhostUserGPIO gpio;
> >> +};
> >> +
> >> +#endif
> >> diff --git a/tests/qtest/libqos/virtio-gpio.c b/tests/qtest/libqos/virtio-gpio.c
> >> new file mode 100644
> >> index 0000000000..762aa6695b
> >> --- /dev/null
> >> +++ b/tests/qtest/libqos/virtio-gpio.c
> >> @@ -0,0 +1,171 @@
> >> +/*
> >> + * virtio-gpio nodes for testing
> >> + *
> >> + * Copyright (c) 2022 Linaro Ltd
> >> + *
> >> + * SPDX-License-Identifier: GPL-2.0-or-later
> >> + */
> >> +
> >> +#include "qemu/osdep.h"
> >> +#include "standard-headers/linux/virtio_config.h"
> >> +#include "../libqtest.h"
> >> +#include "qemu/module.h"
> >> +#include "qgraph.h"
> >> +#include "virtio-gpio.h"
> >> +
> >> +static QGuestAllocator *alloc;
> >> +
> >> +static void virtio_gpio_cleanup(QVhostUserGPIO *gpio)
> >> +{
> >> +    QVirtioDevice *vdev = gpio->vdev;
> >> +    int i;
> >> +
> >> +    for (i = 0; i < 2; i++) {
> >> +        qvirtqueue_cleanup(vdev->bus, gpio->queues[i], alloc);
> >> +    }
> >> +    g_free(gpio->queues);
> >> +}
> >> +
> >> +/*
> >> + * This handles the VirtIO setup from the point of view of the driver
> >> + * frontend and therefor doesn't present any vhost specific features
> >> + * and in fact masks of the re-used bit.
> >> + */
> >> +static void virtio_gpio_setup(QVhostUserGPIO *gpio)
> >> +{
> >> +    QVirtioDevice *vdev = gpio->vdev;
> >> +    uint64_t features;
> >> +    int i;
> >> +
> >> +    features = qvirtio_get_features(vdev);
> >> +    features &= ~QVIRTIO_F_BAD_FEATURE;
> >
> > This looks questionable. qvirtio_get_features() should return VIRTIO
> > feature bits. Is QVIRTIO_F_BAD_FEATURE masked out here because
> > qvirtio_get_features() is returning raw vhost-user feature bits instead
> > and you want to get rid of VHOST_USER_F_PROTOCOL_FEATURES?
> 
> Well it's an invalid bit for the driver/frontend<->hw/backend path -
> although maybe we should error if we saw it?

Thinking about it again, there's an argument for keeping "features &=
~QVIRTIO_F_BAD_FEATURE;". It means the code can be reused with
virtio-pci virtio-gpio devices.

I was just afraid that may the vhost-user implementation is exposing
VHOST_USER_F_PROTOCOL_FEATURES and we're working around that by masking
QVIRTIO_F_BAD_FEATURE here. An assertion/error would protect against
that.

Either way is fine by me.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 00/15] virtio-gpio and various virtio cleanups

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote:
> Hi,
> 
> This series ostensibly adds virtio-user-gpio stubs to the build for
> use with an external vhost-user daemon. We've been testing it with our
> rust daemons from:
> 
>   https://github.com/rust-vmm/vhost-device
> 
> Getting the test enabled took some doing most likely because the need
> for CONFIG support exercised additional paths in the code that were
> not used for the simpler virtio-net tests. As a result the series has
> a number of cleanup and documentation patches.
> 
> The final thing that needed fixing was the ensuring that
> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation
> process. This was the hardest thing to track down as we store the
> feature bits in several places variously as:
> 
>   in VirtIODevice as:
>     uint64_t guest_features;
>     uint64_t host_features;
>     uint64_t backend_features;

None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's
unfiltered feature bits should never be passed to VirtIODevice.

> 
>  in vhost_dev as:
>     uint64_t features;
>     uint64_t acked_features;
>     uint64_t backend_features;

I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES
either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw
vhost-user GET_FEATURES.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 00/15] virtio-gpio and various virtio cleanups

[Alex Bennée]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> [[PGP Signed Part:Undecided]]
> On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote:
>> Hi,
>> 
>> This series ostensibly adds virtio-user-gpio stubs to the build for
>> use with an external vhost-user daemon. We've been testing it with our
>> rust daemons from:
>> 
>>   https://github.com/rust-vmm/vhost-device
>> 
>> Getting the test enabled took some doing most likely because the need
>> for CONFIG support exercised additional paths in the code that were
>> not used for the simpler virtio-net tests. As a result the series has
>> a number of cleanup and documentation patches.
>> 
>> The final thing that needed fixing was the ensuring that
>> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation
>> process. This was the hardest thing to track down as we store the
>> feature bits in several places variously as:
>> 
>>   in VirtIODevice as:
>>     uint64_t guest_features;
>>     uint64_t host_features;
>>     uint64_t backend_features;
>
> None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's
> unfiltered feature bits should never be passed to VirtIODevice.
>
>> 
>>  in vhost_dev as:
>>     uint64_t features;
>>     uint64_t acked_features;
>>     uint64_t backend_features;
>
> I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES
> either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw
> vhost-user GET_FEATURES.

So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set
with the VHOST_USER_SET_FEATURES message? Currently it's fed via:

    uint64_t features = vhost_dev->acked_features;

in vhost_dev_set_features() which does apply a few extra bits
(VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding
VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the
vhost-user backend that this should be done? Overload the function?

>
> Stefan
>
> [[End of PGP Signed Part]]


-- 
Alex Bennée

Re: [PATCH v2 00/15] virtio-gpio and various virtio cleanups

[Stefan Hajnoczi]

On Thu, 7 Jul 2022 at 14:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
> > [[PGP Signed Part:Undecided]]
> > On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote:
> >> Hi,
> >>
> >> This series ostensibly adds virtio-user-gpio stubs to the build for
> >> use with an external vhost-user daemon. We've been testing it with our
> >> rust daemons from:
> >>
> >>   https://github.com/rust-vmm/vhost-device
> >>
> >> Getting the test enabled took some doing most likely because the need
> >> for CONFIG support exercised additional paths in the code that were
> >> not used for the simpler virtio-net tests. As a result the series has
> >> a number of cleanup and documentation patches.
> >>
> >> The final thing that needed fixing was the ensuring that
> >> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation
> >> process. This was the hardest thing to track down as we store the
> >> feature bits in several places variously as:
> >>
> >>   in VirtIODevice as:
> >>     uint64_t guest_features;
> >>     uint64_t host_features;
> >>     uint64_t backend_features;
> >
> > None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's
> > unfiltered feature bits should never be passed to VirtIODevice.
> >
> >>
> >>  in vhost_dev as:
> >>     uint64_t features;
> >>     uint64_t acked_features;
> >>     uint64_t backend_features;
> >
> > I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES
> > either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw
> > vhost-user GET_FEATURES.
>
> So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set
> with the VHOST_USER_SET_FEATURES message? Currently it's fed via:
>
>     uint64_t features = vhost_dev->acked_features;
>
> in vhost_dev_set_features() which does apply a few extra bits
> (VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding
> VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the
> vhost-user backend that this should be done? Overload the function?

A modern vhost-user server replies to VHOST_USER_GET_FEATURES with
VHOST_USER_F_PROTOCOL_FEATURES set. That's when the vhost-user client
encounters this bit.

The vhost-user client should then filter out
VHOST_USER_F_PROTOCOL_FEATURES because it belongs to the vhost-user
protocol and isn't a real VIRTIO feature bit. The client uses the
filtered VIRTIO feature bits and it now knows whether the vhost-user
server supports the VHOST_USER_GET_PROTOCOL_FEATURES and
VHOST_USER_SET_PROTOCOL_FEATURES messages.

I think vhost_user_set_features() should set
VHOST_USER_F_PROTOCOL_FEATURES if the server returned it from
VHOST_USER_GET_FEATURES. At the moment vhost_user_backend_init()
stores VHOST_USER_F_PROTOCOL_FEATURES in struct
vhost_dev->backend_features, which only seems to be used by vhost-net
code.

The other vhost-user devices set acked_features = guest_features and
ignore backend_features. As a result I guess they don't set
VHOST_USER_F_PROTOCOL_FEATURES in the VHOST_USER_SET_FEATURES message.
Most vhost-user servers probably don't care and still respond to
VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES
messages (although the vhost-user protocol spec mentions other
protocol differences when VHOST_USER_F_PROTOCOL_FEATURES is not
negotiated).

Does this match what you've found? The code is a maze so I may have
gotten something wrong. In general I think hw/virtio/vhost-user.c
should be responsible for VHOST_USER_F_PROTOCOL_FEATURES and no other
part of the QEMU codebase should ever see the bit since it's a
vhost-user protocol detail and not part of VIRTIO or even a common
part of vhost.

Stefan

Re: [PATCH v2 00/15] virtio-gpio and various virtio cleanups

[Alex Bennée]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Thu, 7 Jul 2022 at 14:42, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>>
>> > [[PGP Signed Part:Undecided]]
>> > On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote:
>> >> Hi,
>> >>
>> >> This series ostensibly adds virtio-user-gpio stubs to the build for
>> >> use with an external vhost-user daemon. We've been testing it with our
>> >> rust daemons from:
>> >>
>> >>   https://github.com/rust-vmm/vhost-device
>> >>
>> >> Getting the test enabled took some doing most likely because the need
>> >> for CONFIG support exercised additional paths in the code that were
>> >> not used for the simpler virtio-net tests. As a result the series has
>> >> a number of cleanup and documentation patches.
>> >>
>> >> The final thing that needed fixing was the ensuring that
>> >> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation
>> >> process. This was the hardest thing to track down as we store the
>> >> feature bits in several places variously as:
>> >>
>> >>   in VirtIODevice as:
>> >>     uint64_t guest_features;
>> >>     uint64_t host_features;
>> >>     uint64_t backend_features;
>> >
>> > None of these know about VHOST_USER_F_PROTOCOL_FEATURES and vhost-user's
>> > unfiltered feature bits should never be passed to VirtIODevice.
>> >
>> >>
>> >>  in vhost_dev as:
>> >>     uint64_t features;
>> >>     uint64_t acked_features;
>> >>     uint64_t backend_features;
>> >
>> > I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES
>> > either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw
>> > vhost-user GET_FEATURES.
>>
>> So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set
>> with the VHOST_USER_SET_FEATURES message? Currently it's fed via:
>>
>>     uint64_t features = vhost_dev->acked_features;
>>
>> in vhost_dev_set_features() which does apply a few extra bits
>> (VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding
>> VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the
>> vhost-user backend that this should be done? Overload the function?
>
> A modern vhost-user server replies to VHOST_USER_GET_FEATURES with
> VHOST_USER_F_PROTOCOL_FEATURES set. That's when the vhost-user client
> encounters this bit.
>
> The vhost-user client should then filter out
> VHOST_USER_F_PROTOCOL_FEATURES because it belongs to the vhost-user
> protocol and isn't a real VIRTIO feature bit. The client uses the
> filtered VIRTIO feature bits and it now knows whether the vhost-user
> server supports the VHOST_USER_GET_PROTOCOL_FEATURES and
> VHOST_USER_SET_PROTOCOL_FEATURES messages.
>
> I think vhost_user_set_features() should set
> VHOST_USER_F_PROTOCOL_FEATURES if the server returned it from
> VHOST_USER_GET_FEATURES. At the moment vhost_user_backend_init()
> stores VHOST_USER_F_PROTOCOL_FEATURES in struct
> vhost_dev->backend_features, which only seems to be used by vhost-net
> code.

I can clean-up the documentation for this. I wonder if the VirtIODevice
backend_features is a duplication that should be removed?

> The other vhost-user devices set acked_features = guest_features and
> ignore backend_features. As a result I guess they don't set
> VHOST_USER_F_PROTOCOL_FEATURES in the VHOST_USER_SET_FEATURES message.
> Most vhost-user servers probably don't care and still respond to
> VHOST_USER_GET_PROTOCOL_FEATURES and VHOST_USER_SET_PROTOCOL_FEATURES
> messages (although the vhost-user protocol spec mentions other
> protocol differences when VHOST_USER_F_PROTOCOL_FEATURES is not
> negotiated).
>
> Does this match what you've found? The code is a maze so I may have
> gotten something wrong.

I think so. As you say it's a bit of a maze and hopefully we can more
clearly document when and where things are and how they should be used.
The various virtio devices have grown organically so there are
inconsistencies that need ironing out. 

> In general I think hw/virtio/vhost-user.c
> should be responsible for VHOST_USER_F_PROTOCOL_FEATURES and no other
> part of the QEMU codebase should ever see the bit since it's a
> vhost-user protocol detail and not part of VIRTIO or even a common
> part of vhost.

OK I'll see what I can cook up.

>
> Stefan


-- 
Alex Bennée

Re: [PATCH v2 00/15] virtio-gpio and various virtio cleanups

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3554 bytes --]

On Thu, Jul 7, 2022, 17:28 Alex Bennée <alex.bennee@linaro.org> wrote:

>
> Stefan Hajnoczi <stefanha@gmail.com> writes:
>
> > On Thu, 7 Jul 2022 at 14:42, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >>
> >> Stefan Hajnoczi <stefanha@redhat.com> writes:
> >>
> >> > [[PGP Signed Part:Undecided]]
> >> > On Tue, May 24, 2022 at 04:40:41PM +0100, Alex Bennée wrote:
> >> >> Hi,
> >> >>
> >> >> This series ostensibly adds virtio-user-gpio stubs to the build for
> >> >> use with an external vhost-user daemon. We've been testing it with
> our
> >> >> rust daemons from:
> >> >>
> >> >>   https://github.com/rust-vmm/vhost-device
> >> >>
> >> >> Getting the test enabled took some doing most likely because the need
> >> >> for CONFIG support exercised additional paths in the code that were
> >> >> not used for the simpler virtio-net tests. As a result the series has
> >> >> a number of cleanup and documentation patches.
> >> >>
> >> >> The final thing that needed fixing was the ensuring that
> >> >> VHOST_USER_F_PROTOCOL_FEATURES didn't get squashed in the negotiation
> >> >> process. This was the hardest thing to track down as we store the
> >> >> feature bits in several places variously as:
> >> >>
> >> >>   in VirtIODevice as:
> >> >>     uint64_t guest_features;
> >> >>     uint64_t host_features;
> >> >>     uint64_t backend_features;
> >> >
> >> > None of these know about VHOST_USER_F_PROTOCOL_FEATURES and
> vhost-user's
> >> > unfiltered feature bits should never be passed to VirtIODevice.
> >> >
> >> >>
> >> >>  in vhost_dev as:
> >> >>     uint64_t features;
> >> >>     uint64_t acked_features;
> >> >>     uint64_t backend_features;
> >> >
> >> > I don't think these should know about VHOST_USER_F_PROTOCOL_FEATURES
> >> > either. AFAIK vhost_dev deals with VIRTIO feature bits, not raw
> >> > vhost-user GET_FEATURES.
> >>
> >> So where does VHOST_USER_F_PROTOCOL_FEATURES get set before it's set
> >> with the VHOST_USER_SET_FEATURES message? Currently it's fed via:
> >>
> >>     uint64_t features = vhost_dev->acked_features;
> >>
> >> in vhost_dev_set_features() which does apply a few extra bits
> >> (VHOST_F_LOG_ALL/VIRTIO_F_IOMMU_PLATFORM). Maybe it should be adding
> >> VHOST_USER_F_PROTOCOL_FEATURES here? How should it be signalled by the
> >> vhost-user backend that this should be done? Overload the function?
> >
> > A modern vhost-user server replies to VHOST_USER_GET_FEATURES with
> > VHOST_USER_F_PROTOCOL_FEATURES set. That's when the vhost-user client
> > encounters this bit.
> >
> > The vhost-user client should then filter out
> > VHOST_USER_F_PROTOCOL_FEATURES because it belongs to the vhost-user
> > protocol and isn't a real VIRTIO feature bit. The client uses the
> > filtered VIRTIO feature bits and it now knows whether the vhost-user
> > server supports the VHOST_USER_GET_PROTOCOL_FEATURES and
> > VHOST_USER_SET_PROTOCOL_FEATURES messages.
> >
> > I think vhost_user_set_features() should set
> > VHOST_USER_F_PROTOCOL_FEATURES if the server returned it from
> > VHOST_USER_GET_FEATURES. At the moment vhost_user_backend_init()
> > stores VHOST_USER_F_PROTOCOL_FEATURES in struct
> > vhost_dev->backend_features, which only seems to be used by vhost-net
> > code.
>
> I can clean-up the documentation for this. I wonder if the VirtIODevice
> backend_features is a duplication that should be removed?
>

I don't know the code well enough to say, but it's possible that it can be
simplified.

Stefan

[-- Attachment #2: Type: text/html, Size: 5032 bytes --]