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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- 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 --]