Re: What is the correct way to handle the VirtIO config space in vhost-user?

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4822 bytes --]

On Fri, Feb 25, 2022 at 05:32:43PM +0000, Alex Bennée wrote:
> 
> [Apologies to CC list for repost due to fat fingering the mailing list address]
> 
> Hi Michael,
> 
> I was updating my vhost-user-rpmb implementation when I realised it
> wasn't handling config space access properly. However when I went to
> implement it I realised there seems to be two ways of doing this. For
> example in vhost-user-gpu we have:
> 
>   static void
>   vhost_user_gpu_get_config(VirtIODevice *vdev, uint8_t *config_data)
>   {
>       VhostUserGPU *g = VHOST_USER_GPU(vdev);
>       VirtIOGPUBase *b = VIRTIO_GPU_BASE(vdev);
>       struct virtio_gpu_config *vgconfig =
>           (struct virtio_gpu_config *)config_data;
>       Error *local_err = NULL;
>       int ret;
> 
>       memset(config_data, 0, sizeof(struct virtio_gpu_config));
> 
>       ret = vhost_dev_get_config(&g->vhost->dev,
>                                  config_data, sizeof(struct virtio_gpu_config),
>                                  &local_err);
>       if (ret) {
>           error_report_err(local_err);
>           return;
>       }
> 
>       /* those fields are managed by qemu */
>       vgconfig->num_scanouts = b->virtio_config.num_scanouts;
>       vgconfig->events_read = b->virtio_config.events_read;
>       vgconfig->events_clear = b->virtio_config.events_clear;
>   }
> 
> which is setup with .get_config and .set_config functions that poke the
> appropriate vhost communication. However to do this needs an instance
> init to create a vhost just so it can jump the g->vhost->dev indirection:
> 
>   static void
>   vhost_user_gpu_instance_init(Object *obj)
>   {
>       VhostUserGPU *g = VHOST_USER_GPU(obj);
> 
>       g->vhost = VHOST_USER_BACKEND(object_new(TYPE_VHOST_USER_BACKEND));
>       object_property_add_alias(obj, "chardev",
>                                 OBJECT(g->vhost), "chardev");
>   }
> 
> (aside: this continues my QOM confusion about when things should be in a
> class or instance init, up until this point I hadn't needed it in my
> stub).

Class init is a one-time per-class initializer function. It is mostly
used for setting up callbacks/overridden methods from the base class.

Instance init is like an object constructor in object-oriented
programming.

> However when grepping around I found some vhost-user devices do it
> differently, for example vhost-user-blk has:
> 
>   static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
>   {
>       int ret;
>       struct virtio_blk_config blkcfg;
>       VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
>       Error *local_err = NULL;
> 
>       ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
>                                  sizeof(struct virtio_blk_config),
>                                  &local_err);
>       if (ret < 0) {
>           error_report_err(local_err);
>           return ret;
>       }
> 
>       /* valid for resize only */
>       if (blkcfg.capacity != s->blkcfg.capacity) {
>           s->blkcfg.capacity = blkcfg.capacity;
>           memcpy(dev->vdev->config, &s->blkcfg, sizeof(struct virtio_blk_config));
>           virtio_notify_config(dev->vdev);
>       }
> 
>       return 0;
>   }

This is not a .get_config() method, it's a VIRTIO configuration change
notification handler. The vhost-user-blk device server ("slave") sends
this notification to notify the driver that configuration space contents
have been updated (e.g. the disk was resized). QEMU fetches the new
config space contents from the device and then forwards the notification
to the guest.

The .get_config() method for vhost-user-blk.c is:

  static void vhost_user_blk_update_config(VirtIODevice *vdev, uint8_t *config)
  {
      VHostUserBlk *s = VHOST_USER_BLK(vdev);
  
      /* Our num_queues overrides the device backend */
      virtio_stw_p(vdev, &s->blkcfg.num_queues, s->num_queues);
  
      memcpy(config, &s->blkcfg, sizeof(struct virtio_blk_config));
  }

vhost_user_blk_update_config() is simple, it copies out s->blkcfg.

> Although this seems to miss the ability to "set" a config - although
> that seems confusing anyway, surely the guest only ever reads the config
> space?

VIRTIO allows the driver to write to the config space. This is used to
toggle the disk write cache on the virtio-blk device, for example.

> So my question is which approach is the correct one? Is one a legacy
> approach or is it "depends on what you are doing"?

Yes, it depends on whether the device sends configuration space change
notifications or not. If not, a traditional .get_config() like
vhost-user-gpu can be used. If yes, then caching the configuration space
contents like vhost-user-blk is convenient.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]