[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: What is the correct way to handle the VirtIO config space in vhost-u
From: |
Stefan Hajnoczi |
Subject: |
Re: What is the correct way to handle the VirtIO config space in vhost-user? |
Date: |
Tue, 8 Mar 2022 10:31:51 +0000 |
On Mon, Mar 07, 2022 at 12:09:59PM +0000, Alex Bennée wrote:
>
> 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.
I see. Currently QEMU only allows 2 cases:
1. No VHOST_USER_PROTOCOL_F_CONFIG
2. VHOST_USER_PROTOCOL_F_CONFIG with config change notifications
implemented by VirtIODevice
You are adding another (valid) case:
3. VHOST_USER_PROTOCOL_F_CONFIG without config change notifications
implemented by VirtIODevice
Some device types never use config change notifications so I think
you're correct and QEMU's check is too restrictive.
Stefan
signature.asc
Description: PGP signature