[Virtio-fs] [PATCH v3 10/20] hw/virtio: add config support to vhost-user-device

[Albert Esteve]

Sorry to bring up this post, it's been a while since you posted.
But I have been testing the patch the last couple of days.

On Mon, Jul 10, 2023 at 9:58 PM Michael S. Tsirkin <mst at redhat.com> wrote:

> On Mon, Jul 10, 2023 at 04:35:12PM +0100, Alex Bennée wrote:
> > To use the generic device the user will need to provide the config
> > region size via the command line. We also add a notifier so the guest
> > can be pinged if the remote daemon updates the config.
> >
> > With these changes:
> >
> >   -device vhost-user-device-pci,virtio-id=41,num_vqs=2,config_size=8
> >
> > is equivalent to:
> >
> >   -device vhost-user-gpio-pci
> >
> > Signed-off-by: Alex Bennée <alex.bennee at linaro.org>
>
>
> This one I think it's best to defer until we get a better
> handle on how we want the configuration to look.
>
>
> > ---
> >  include/hw/virtio/vhost-user-device.h |  1 +
> >  hw/virtio/vhost-user-device.c         | 58 ++++++++++++++++++++++++++-
> >  2 files changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/virtio/vhost-user-device.h
> b/include/hw/virtio/vhost-user-device.h
> > index 9105011e25..3ddf88a146 100644
> > --- a/include/hw/virtio/vhost-user-device.h
> > +++ b/include/hw/virtio/vhost-user-device.h
> > @@ -22,6 +22,7 @@ struct VHostUserBase {
> >      CharBackend chardev;
> >      uint16_t virtio_id;
> >      uint32_t num_vqs;
> > +    uint32_t config_size;
> >      /* State tracking */
> >      VhostUserState vhost_user;
> >      struct vhost_virtqueue *vhost_vq;
> > diff --git a/hw/virtio/vhost-user-device.c
> b/hw/virtio/vhost-user-device.c
> > index b0239fa033..2b028cae08 100644
> > --- a/hw/virtio/vhost-user-device.c
> > +++ b/hw/virtio/vhost-user-device.c
> > @@ -117,6 +117,42 @@ static uint64_t vub_get_features(VirtIODevice *vdev,
> >      return vub->vhost_dev.features & ~(1ULL <<
> VHOST_USER_F_PROTOCOL_FEATURES);
> >  }
> >
> > +/*
> > + * To handle VirtIO config we need to know the size of the config
> > + * space. We don't cache the config but re-fetch it from the guest
> > + * every time in case something has changed.
> > + */
> > +static void vub_get_config(VirtIODevice *vdev, uint8_t *config)
> > +{
> > +    VHostUserBase *vub = VHOST_USER_BASE(vdev);
> > +    Error *local_err = NULL;
> > +
> > +    /*
> > +     * There will have been a warning during vhost_dev_init, but lets
> > +     * assert here as nothing will go right now.
> > +     */
> > +    g_assert(vub->config_size && vub->vhost_user.supports_config ==
> true);
> > +
> > +    if (vhost_dev_get_config(&vub->vhost_dev, config,
> > +                             vub->config_size, &local_err)) {
> > +        error_report_err(local_err);
> > +    }
> > +}
> > +
> > +/*
> > + * When the daemon signals an update to the config we just need to
> > + * signal the guest as we re-read the config on demand above.
> > + */
> > +static int vub_config_notifier(struct vhost_dev *dev)
> > +{
> > +    virtio_notify_config(dev->vdev);
> > +    return 0;
> > +}
> > +
> > +const VhostDevConfigOps vub_config_ops = {
> > +    .vhost_dev_config_notifier = vub_config_notifier,
> > +};
> > +
> >  static void vub_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> >  {
> >      /*
> > @@ -141,12 +177,21 @@ static int vub_connect(DeviceState *dev)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      VHostUserBase *vub = VHOST_USER_BASE(vdev);
> > +    struct vhost_dev *vhost_dev = &vub->vhost_dev;
> >
> >      if (vub->connected) {
> >          return 0;
> >      }
> >      vub->connected = true;
> >
> > +    /*
> > +     * If we support VHOST_USER_GET_CONFIG we must enable the notifier
> > +     * so we can ping the guest when it updates.
> > +     */
> > +    if (vub->vhost_user.supports_config) {
> > +        vhost_dev_set_config_notifier(vhost_dev, &vub_config_ops);
> > +    }
> > +
> >      /* restore vhost state */
> >      if (virtio_device_started(vdev, vdev->status)) {
> >          vub_start(vdev);
> > @@ -214,11 +259,20 @@ static void vub_device_realize(DeviceState *dev,
> Error **errp)
> >          vub->num_vqs = 1; /* reasonable default? */
> >      }
> >
> > +    /*
> > +     * We can't handle config requests unless we know the size of the
> > +     * config region, specialisations of the vhost-user-device will be
> > +     * able to set this.
> > +     */
> > +    if (vub->config_size) {
> > +        vub->vhost_user.supports_config = true;
> > +    }
>

Shouldn't the `supports_config = true' be set before we
call vhost_dev_init() a few lines above?
Otherwise, we end up checking the `supports_config` attribute from within
`vhost_user_backend_init()` (in vhost_user source file)
before the VhostUserState is set, causing this warning to pop if the
backend supports the CONFIG feature:
```
qemu-system-x86_64: warning: vhost-user backend supports
VHOST_USER_PROTOCOL_F_CONFIG but QEMU does not.
```


> > +
> >      if (!vhost_user_init(&vub->vhost_user, &vub->chardev, errp)) {
> >          return;
> >      }
> >
> > -    virtio_init(vdev, vub->virtio_id, 0);
> > +    virtio_init(vdev, vub->virtio_id, vub->config_size);
> >
> >      /*
> >       * Disable guest notifiers, by default all notifications will be
> via the
> > @@ -268,6 +322,7 @@ static void vub_class_init(ObjectClass *klass, void
> *data)
> >      vdc->realize = vub_device_realize;
> >      vdc->unrealize = vub_device_unrealize;
> >      vdc->get_features = vub_get_features;
> > +    vdc->get_config = vub_get_config;
> >      vdc->set_status = vub_set_status;
> >  }
> >
> > @@ -295,6 +350,7 @@ static Property vud_properties[] = {
> >      DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
> >      DEFINE_PROP_UINT16("virtio-id", VHostUserBase, virtio_id, 0),
> >      DEFINE_PROP_UINT32("num_vqs", VHostUserBase, num_vqs, 1),
> > +    DEFINE_PROP_UINT32("config_size", VHostUserBase, config_size, 0),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > --
> > 2.39.2
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/virtio-fs/attachments/20230831/26fd45c1/attachment.htm>