On Mon, Mar 07, 2022 at 12:09:59PM +0000, Alex Bennée wrote: > > Stefan Hajnoczi <[email protected]> writes: > > > [[PGP Signed Part:Undecided]] > > On Fri, Mar 04, 2022 at 04:49:30PM +0000, Alex Bennée wrote: > >> > >> Stefan Hajnoczi <[email protected]> writes: > >> > >> > [[PGP Signed Part:Undecided]] > >> > On Mon, Feb 28, 2022 at 04:16:43PM +0000, Alex Bennée wrote: > >> >> > >> >> Stefan Hajnoczi <[email protected]> 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
