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

["Erik Schilling" <erik.schilling@linaro.org>]

On Thu Aug 31, 2023 at 5:47 PM CEST, Alex Bennée wrote:
>
> Albert Esteve <aesteve@redhat.com> writes:
>
> > 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@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@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.
> > ```
>
> I allude to that in the comments for vub_get_config() further up.
> However the more I look at this the more confused I am about the
> original intention of the flag I added (*blush*). I think we need to
> handle the following cases:
>
>  - the virtio device has no config space
>  - the virtio device has config space, emulated inside qemu
>  - the virtio vhost device has config space, emulated inside qemu
>  - the virtio vhost device has config space, handled by vhost
>
> for the final case the qemu internals need to be able to handle the
> signalling of updates to the config by the vhost device by way of the
> notifier.
>
> In the case of a "standalone" vhost-user daemon we won't even know if
> there is a config space until we have connected to it and queried its
> size via protocol messages.
>
> Maybe we need two fields?
>
>   - supports_remote_config (device is capable of handling remote config)
>   - config_location one of { NONE, LOCAL, REMOTE }

Hm... Shouldn't the F_CONFIG flag already indicate
`supports_remote_config` (you mean "backend-handled" config, right?) and
the negotiation of the flag would clarify the location?

Of course, there would be a disambiguity between case 3 and 4. But
wouldn't that be decidable by the configured QEMU device? The new
standalone device could default to just forwarding the backend config
while the existing devices could continue providing their proxies where
required?

- Erik

>   
> >  
> >  > +
> >  >      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