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.
Ah, true. Sorry I missed it. Still not sure that allowing the warning is a good idea.
Either the warning is not relevant anymore or the logic is not correct.
In my case the connection was breaking because the driver was receiving
a wrong configuration, even though the backend was sending correct data.
Qemu disables the F_CONFIG bit from the backend features when the
warning is printed, and I assume that was causing the issue that the
config fields were all 0'd when the driver asked for it.
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 }
Mmh, when setting the config location, if remote config is not supported and the location
is remote, we could force the flag to NONE, and then we only need the `config_location`.
It already tells us both if supported, and where to find it.
>
> > +
> > 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