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

["Alex Bennée" <alex.bennee@linaro.org>]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> [[PGP Signed Part:Undecided]]
> On Mon, Feb 28, 2022 at 04:16:43PM +0000, Alex Bennée wrote:
>> 
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>> 
>> > [[PGP Signed Part:Undecided]]
>> > 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]
>> >> 
>> <snip>
>> >> 
>> >> (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.
>> 
>> I phrased my statement poorly. What I meant to say is I sometimes find
>> QEMUs approach to using class over instance initialisation inconsistent.
>> I think I understand the "policy" as use class init until there is a
>> case where you can't (e.g. having individual control of each instance of
>> a device).
>> 
>> > 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).
>> 
>> So this should come in the initial vhost-user set of handshake messages
>> if the VHOST_USER_PROTOCOL_F_CONFIG is negotiated between the master and
>> slave? I guess without this protocol feature vhost-user can't support
>> writeable config spaces?
>
> The VHOST_USER_PROTOCOL_F_CONFIG vhost-user protocol feature bit
> enables:
> 1. VHOST_USER_GET_CONFIG - reading configuration space
> 2. VHOST_USER_SET_CONFIG - writing configuration space
> 3. VHOST_USER_SLAVE_CONFIG_CHANGE_MSG - change notifications
>
> If the vhost-user server is supposed to participate in configuration
> space accesses/notifications, then it needs to implement
> VHOST_USER_PROTOCOL_F_CONFIG.
>
> QEMU's vhost-user-blk assumes the vhost-user server supports
> VHOST_USER_PROTOCOL_F_CONFIG. It's an optional vhost-user protocol
> feature but the virtio-blk device relies on configuration space
> (otherwise QEMU's --device vhost-user-blk wouldn't know the capacity of
> the disk). vhost_user_blk_realize_connect() sends VHOST_USER_GET_CONFIG
> to fetch the configuration space contents when the device is
> instantiated.
>
> Some vhost-user device types don't need VHOST_USER_PROTOCOL_F_CONFIG. In
> that case QEMU's --device vhost-user-FOO implements .get/set_config()
> itself. virtio-net is an example where this is the case.

I wonder when the last time this was tested was because since 1c3e5a2617
(vhost-user: back SET/GET_CONFIG requests with a protocol feature) the
check in vhost_user_backend_init is:

   if (!dev->config_ops || !dev->config_ops->vhost_dev_config_notifier) {
       /* Don't acknowledge CONFIG feature if device doesn't support it */
       dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
   } else if (!(protocol_features &
               (1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) {
       error_setg(errp, "Device expects VHOST_USER_PROTOCOL_F_CONFIG "
                  "but backend does not support it.");
       return -EINVAL;
   }

which means I don't think it ever asks the vhost-user backend.

>
>> > 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.
>> 
>> Is there any feature flag for this in the VirtIO spec? I had a look and
>> couldn't see an obvious common one. Does it basically come down to the
>> verbiage in the Device configure layout section for any given device?
>
> The contents of the configuration space are device-specific, so there is
> no generic feature flag. Many devices don't update the configuration
> space and therefore don't send change notifications. The details are
> documented for each device type (e.g. "if the driver negotiated the
> VIRTIO_CONSOLE_F_SIZE feature, a configuration change notification
> indicates that the updated size can be read from the configuration
> fields").
>
> I just noticed that VIRTIO does not specify that the virtio-blk capacity
> field can change. The spec is incomplete and I will send a patch for
> that.
>
> Stefan
>
> [[End of PGP Signed Part]]


-- 
Alex Bennée