On Thu, Aug 31, 2023 at 6:03 PM Alex Bennée <alex.ben...@linaro.org> wrote:
>
> Albert Esteve <aest...@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 <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.
> > ```
>
> 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
>
>
