<div dir="ltr"><div dir="ltr"><div><div dir="ltr" class="gmail_signature"><div dir="ltr"><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 31, 2023 at 6:03 PM Alex Bennée <<a href="mailto:alex.bennee@linaro.org">alex.bennee@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
Albert Esteve <<a href="mailto:aesteve@redhat.com" target="_blank">aesteve@redhat.com</a>> writes:<br>
<br>
> Sorry to bring up this post, it's been a while since you posted.<br>
> But I have been testing the patch the last couple of days.<br>
><br>
> On Mon, Jul 10, 2023 at 9:58 PM Michael S. Tsirkin <<a href="mailto:mst@redhat.com" target="_blank">mst@redhat.com</a>> wrote:<br>
><br>
> On Mon, Jul 10, 2023 at 04:35:12PM +0100, Alex Bennée wrote:<br>
> > To use the generic device the user will need to provide the config<br>
> > region size via the command line. We also add a notifier so the guest<br>
> > can be pinged if the remote daemon updates the config.<br>
> > <br>
> > With these changes:<br>
> > <br>
> > -device vhost-user-device-pci,virtio-id=41,num_vqs=2,config_size=8<br>
> > <br>
> > is equivalent to:<br>
> > <br>
> > -device vhost-user-gpio-pci<br>
> > <br>
> > Signed-off-by: Alex Bennée <<a href="mailto:alex.bennee@linaro.org" target="_blank">alex.bennee@linaro.org</a>><br>
><br>
> This one I think it's best to defer until we get a better<br>
> handle on how we want the configuration to look.<br>
><br>
> > ---<br>
> > include/hw/virtio/vhost-user-device.h | 1 +<br>
> > hw/virtio/vhost-user-device.c | 58 ++++++++++++++++++++++++++-<br>
> > 2 files changed, 58 insertions(+), 1 deletion(-)<br>
> > <br>
> > diff --git a/include/hw/virtio/vhost-user-device.h b/include/hw/virtio/vhost-user-device.h<br>
> > index 9105011e25..3ddf88a146 100644<br>
> > --- a/include/hw/virtio/vhost-user-device.h<br>
> > +++ b/include/hw/virtio/vhost-user-device.h<br>
> > @@ -22,6 +22,7 @@ struct VHostUserBase {<br>
> > CharBackend chardev;<br>
> > uint16_t virtio_id;<br>
> > uint32_t num_vqs;<br>
> > + uint32_t config_size;<br>
> > /* State tracking */<br>
> > VhostUserState vhost_user;<br>
> > struct vhost_virtqueue *vhost_vq;<br>
> > diff --git a/hw/virtio/vhost-user-device.c b/hw/virtio/vhost-user-device.c<br>
> > index b0239fa033..2b028cae08 100644<br>
> > --- a/hw/virtio/vhost-user-device.c<br>
> > +++ b/hw/virtio/vhost-user-device.c<br>
> > @@ -117,6 +117,42 @@ static uint64_t vub_get_features(VirtIODevice *vdev,<br>
> > return vub->vhost_dev.features & ~(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);<br>
> > }<br>
> > <br>
> > +/*<br>
> > + * To handle VirtIO config we need to know the size of the config<br>
> > + * space. We don't cache the config but re-fetch it from the guest<br>
> > + * every time in case something has changed.<br>
> > + */<br>
> > +static void vub_get_config(VirtIODevice *vdev, uint8_t *config)<br>
> > +{<br>
> > + VHostUserBase *vub = VHOST_USER_BASE(vdev);<br>
> > + Error *local_err = NULL;<br>
> > +<br>
> > + /*<br>
> > + * There will have been a warning during vhost_dev_init, but lets<br>
> > + * assert here as nothing will go right now.<br>
> > + */<br>
> > + g_assert(vub->config_size && vub->vhost_user.supports_config == true);<br>
> > +<br>
> > + if (vhost_dev_get_config(&vub->vhost_dev, config,<br>
> > + vub->config_size, &local_err)) {<br>
> > + error_report_err(local_err);<br>
> > + }<br>
> > +}<br>
> > +<br>
> > +/*<br>
> > + * When the daemon signals an update to the config we just need to<br>
> > + * signal the guest as we re-read the config on demand above.<br>
> > + */<br>
> > +static int vub_config_notifier(struct vhost_dev *dev)<br>
> > +{<br>
> > + virtio_notify_config(dev->vdev);<br>
> > + return 0;<br>
> > +}<br>
> > +<br>
> > +const VhostDevConfigOps vub_config_ops = {<br>
> > + .vhost_dev_config_notifier = vub_config_notifier,<br>
> > +};<br>
> > +<br>
> > static void vub_handle_output(VirtIODevice *vdev, VirtQueue *vq)<br>
> > {<br>
> > /*<br>
> > @@ -141,12 +177,21 @@ static int vub_connect(DeviceState *dev)<br>
> > {<br>
> > VirtIODevice *vdev = VIRTIO_DEVICE(dev);<br>
> > VHostUserBase *vub = VHOST_USER_BASE(vdev);<br>
> > + struct vhost_dev *vhost_dev = &vub->vhost_dev;<br>
> > <br>
> > if (vub->connected) {<br>
> > return 0;<br>
> > }<br>
> > vub->connected = true;<br>
> > <br>
> > + /*<br>
> > + * If we support VHOST_USER_GET_CONFIG we must enable the notifier<br>
> > + * so we can ping the guest when it updates.<br>
> > + */<br>
> > + if (vub->vhost_user.supports_config) {<br>
> > + vhost_dev_set_config_notifier(vhost_dev, &vub_config_ops);<br>
> > + }<br>
> > +<br>
> > /* restore vhost state */<br>
> > if (virtio_device_started(vdev, vdev->status)) {<br>
> > vub_start(vdev);<br>
> > @@ -214,11 +259,20 @@ static void vub_device_realize(DeviceState *dev, Error **errp)<br>
> > vub->num_vqs = 1; /* reasonable default? */<br>
> > }<br>
> > <br>
> > + /*<br>
> > + * We can't handle config requests unless we know the size of the<br>
> > + * config region, specialisations of the vhost-user-device will be<br>
> > + * able to set this.<br>
> > + */<br>
> > + if (vub->config_size) {<br>
> > + vub->vhost_user.supports_config = true;<br>
> > + }<br>
><br>
> Shouldn't the `supports_config = true' be set before we call vhost_dev_init() a few lines above?<br>
> Otherwise, we end up checking the `supports_config` attribute from within `vhost_user_backend_init()` (in vhost_user<br>
> source file)<br>
> before the VhostUserState is set, causing this warning to pop if the backend supports the CONFIG feature:<br>
> ```<br>
> qemu-system-x86_64: warning: vhost-user backend supports VHOST_USER_PROTOCOL_F_CONFIG but QEMU does<br>
> not.<br>
> ```<br>
<br>
I allude to that in the comments for vub_get_config() further up.<br></blockquote><div><br></div><div>Ah, true. Sorry I missed it. Still not sure that allowing the warning is a good idea.</div><div>Either the warning is not relevant anymore or the logic is not correct.</div><div>In my case the connection was breaking because the driver was receiving</div><div>a wrong configuration, even though the backend was sending correct data.</div><div>Qemu disables the F_CONFIG bit from the backend features when the</div><div>warning is printed, and I assume that was causing the issue that the</div><div>config fields were all 0'd when the driver asked for it.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
However the more I look at this the more confused I am about the<br>
original intention of the flag I added (*blush*). I think we need to<br>
handle the following cases:<br>
<br>
- the virtio device has no config space<br>
- the virtio device has config space, emulated inside qemu<br>
- the virtio vhost device has config space, emulated inside qemu<br>
- the virtio vhost device has config space, handled by vhost<br>
<br>
for the final case the qemu internals need to be able to handle the<br>
signalling of updates to the config by the vhost device by way of the<br>
notifier.<br>
<br>
In the case of a "standalone" vhost-user daemon we won't even know if<br>
there is a config space until we have connected to it and queried its<br>
size via protocol messages.<br>
<br>
Maybe we need two fields?<br>
<br>
- supports_remote_config (device is capable of handling remote config)<br>
- config_location one of { NONE, LOCAL, REMOTE }<br>
<br></blockquote><div><br></div><div>Mmh, when setting the config location, if remote config is not supported and the location</div><div>is remote, we could force the flag to NONE, and then we only need the `config_location`.</div><div>It already tells us both if supported, and where to find it. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> <br>
> > +<br>
> > if (!vhost_user_init(&vub->vhost_user, &vub->chardev, errp)) {<br>
> > return;<br>
> > }<br>
> > <br>
> > - virtio_init(vdev, vub->virtio_id, 0);<br>
> > + virtio_init(vdev, vub->virtio_id, vub->config_size);<br>
> > <br>
> > /*<br>
> > * Disable guest notifiers, by default all notifications will be via the<br>
> > @@ -268,6 +322,7 @@ static void vub_class_init(ObjectClass *klass, void *data)<br>
> > vdc->realize = vub_device_realize;<br>
> > vdc->unrealize = vub_device_unrealize;<br>
> > vdc->get_features = vub_get_features;<br>
> > + vdc->get_config = vub_get_config;<br>
> > vdc->set_status = vub_set_status;<br>
> > }<br>
> > <br>
> > @@ -295,6 +350,7 @@ static Property vud_properties[] = {<br>
> > DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),<br>
> > DEFINE_PROP_UINT16("virtio-id", VHostUserBase, virtio_id, 0),<br>
> > DEFINE_PROP_UINT32("num_vqs", VHostUserBase, num_vqs, 1),<br>
> > + DEFINE_PROP_UINT32("config_size", VHostUserBase, config_size, 0),<br>
> > DEFINE_PROP_END_OF_LIST(),<br>
> > };<br>
> > <br>
> > -- <br>
> > 2.39.2<br>
<br>
<br>
-- <br>
Alex Bennée<br>
Virtualisation Tech Lead @ Linaro<br>
<br>
</blockquote></div></div>