Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space

[Liu, Changpeng]

> -----Original Message-----
> From: Stefan Hajnoczi [mailto:address@hidden
> Sent: Friday, October 20, 2017 6:01 PM
> To: Michael S. Tsirkin <address@hidden>
> Cc: Liu, Changpeng <address@hidden>; address@hidden;
> address@hidden; address@hidden; address@hidden;
> Harris, James R <address@hidden>
> Subject: Re: [PATCH v4 1/4] vhost-user: add new vhost user messages to support
> virtio config space
> 
> On Thu, Oct 19, 2017 at 06:36:00PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Oct 19, 2017 at 04:09:35PM +0200, Stefan Hajnoczi wrote:
> > > On Thu, Oct 19, 2017 at 01:24:07PM +0800, Changpeng Liu wrote:
> > > > @@ -922,6 +931,91 @@ static void vhost_user_set_iotlb_callback(struct
> vhost_dev *dev, int enabled)
> > > >      /* No-op as the receive channel is not dedicated to IOTLB 
> > > > messages. */
> > > >  }
> > > >
> > > > +static int vhost_user_get_config(struct vhost_dev *dev, uint8_t 
> > > > *config,
> > > > +                                 size_t config_len)
> > > > +{
> > > > +    VhostUserMsg msg = {
> > > > +        .request = VHOST_USER_GET_CONFIG,
> > > > +        .flags = VHOST_USER_VERSION,
> > > > +        .size = config_len,
> > > > +    };
> > > > +
> > > > +    if (config_len == 0 || config_len > VHOST_USER_PAYLOAD_SIZE) {
> > >
> > > config_len should be limited to 256 bytes:
> > >
> > >   if (config_len == 0 || config_len > sizeof(msg.payload.config) {
> >
> > I would just limit it to a reasonable value, acceptable to
> > both master and slave, not fail if it's bigger.
> >
> >
> > > > +        error_report("bad config length");
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    if (vhost_user_read(dev, &msg) < 0) {
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    if (msg.request != VHOST_USER_GET_CONFIG) {
> > > > +        error_report("Received unexpected msg type. Expected %d
> received %d",
> > > > +                     VHOST_USER_GET_CONFIG, msg.request);
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    if (msg.size != config_len) {
> > > > +        error_report("Received bad msg size.");
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    memcpy(config, &msg.payload.config, config_len);
> > >
> > > There is some complexity here: different virtio devices use different
> > > amounts of config space.  Devices may append new fields to the config
> > > space to support new features.
> > >
> > > Therefore I think the simplest protocol is to always fetch the full
> > > 256-byte configuration space.  This way the vhost-user slave process can
> > > implement feature bits that the master process does not know about.
> > >
> > > In other words, I don't think the master process knows how much of the
> > > config space is used so it should always request 256 bytes.
> >
> > Each device knows the max config space size.
> >
> >     vdev->config_len = config_size;
> 
> I see you're referring to the field that is set in:
> 
>   void virtio_init(VirtIODevice *vdev, const char *name,
>                    uint16_t device_id, size_t config_size)
> 
> How does this work for vhost-user where different slave programs may
> offer different config sizes?
Each Qemu vhost controller e.g: vhost-user-scsi-pci and vhost-user-blk-pci 
should has different char devices, 
so vhost-slave knows those messages are from vhost-scsi or vhost-blk, of 
course, each UNIX domain socket
should be assigned by users with types: vhsot-scsi or vhost-blk.  
> 
> The QEMU master process does not know the correct size ahead of time.
> The size depends on the vhost-user slave process.  This is why I suggest
> using the full 256 bytes that the VIRTIO spec defines.