[Virtio-fs] [PATCH v3 10/20] hw/virtio: add config support to vhost-user-device
Alex Bennée
alex.bennee at linaro.org
Thu Aug 31 15:47:55 UTC 2023
Albert Esteve <aesteve at 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 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.
> ```
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 }
>
> > +
> > 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
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
More information about the Virtio-fs
mailing list