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 <m...@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.ben...@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
>
>
>
