<div dir="ltr"><div dir="ltr"><div><div dir="ltr" class="gmail_signature"><div dir="ltr">Sorry to bring up this post, it's been a while since you posted.<br></div></div></div><div>But I have been testing the patch the last couple of days.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 10, 2023 at 9:58 PM Michael S. Tsirkin <<a href="mailto:mst@redhat.com">mst@redhat.com</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">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>
<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>
> ---<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></blockquote><div><br></div><div>Shouldn't the `supports_config = true' be set before we call vhost_dev_init() a few lines above?</div><div>Otherwise, we end up checking the `supports_config` attribute from within `vhost_user_backend_init()` (in vhost_user source file)</div><div>before the VhostUserState is set, causing this warning to pop if the backend supports the CONFIG feature:</div><div>```<br></div><div>qemu-system-x86_64: warning: vhost-user backend supports VHOST_USER_PROTOCOL_F_CONFIG but QEMU does not.<br></div><div>```</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">
> +<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>
</blockquote></div></div>