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 Fri, Mar 04, 2022 at 04:49:30PM +0000, Alex Bennée wrote:
>> 
>> 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.
>
> Can you describe what you have in mind? The issue isn't clear to me.

I had to patch out that config_ops check to get the get_config over
vhost to work. Otherwise QEMU keeps complaining:

  qemu-system-aarch64: VHOST_USER_PROTOCOL_F_CONFIG not supported

because it itself has squashed the feature in the vhost protocol
negotiation.


>
> Stefan
>
> [[End of PGP Signed Part]]


-- 
Alex Bennée