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

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

[Alex Bennée]

[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).

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;
  }

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?

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

Ultimately I guess this points to the need for a bit more API
documentation to make it clear when certain methods should be used.

-- 
Alex Bennée

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

[Stefan Hajnoczi]

[-- 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 --]

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

[Alex Bennée]

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?

> 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?

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


-- 
Alex Bennée

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

[Peter Maydell]

On Mon, 28 Feb 2022 at 16:32, Alex Bennée <alex.bennee@linaro.org> wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> > On Fri, Feb 25, 2022 at 05:32:43PM +0000, Alex Bennée wrote:
> >> (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).

Do you have examples of inconsistency? (I'm sure there are some,
we're inconsistent about almost everything...)

In principle though I think the class-init vs instance-init split
is mostly fairly clear: if something changes per-instance then
it needs to be in the object state structure and set up in
instance_init. If it is a property of all instances of the
object then it goes in the class struct and is set up in the
class init function. As Stefan says, mostly that is setting up
the equivalent of method pointers and static class data.
It's more straightforward than instance-init vs realize, at
least :-)

-- PMM

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

[Stefan Hajnoczi]

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

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.

> > 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

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

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

[Stefan Hajnoczi]

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

On Mon, Feb 28, 2022 at 04:44:47PM +0000, Peter Maydell wrote:
> On Mon, 28 Feb 2022 at 16:32, Alex Bennée <alex.bennee@linaro.org> wrote:
> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> > > On Fri, Feb 25, 2022 at 05:32:43PM +0000, Alex Bennée wrote:
> > >> (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).
> 
> Do you have examples of inconsistency? (I'm sure there are some,
> we're inconsistent about almost everything...)

Phew, at least we're inconsistent about being inconsistent. If we were
inconsistent about absolutely everything that just wouldn't do!

Stefan

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

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

[Alex Bennée]

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

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

[Stefan Hajnoczi]

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

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.

Stefan

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

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

[Alex Bennée]

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

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

[Stefan Hajnoczi]

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

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

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