On Tue, Oct 24, 2017 at 12:52:28AM +0000, Liu, Changpeng wrote: > > > > -----Original Message----- > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com] > > Sent: Tuesday, October 24, 2017 1:26 AM > > To: Liu, Changpeng <changpeng....@intel.com> > > Cc: Michael S. Tsirkin <m...@redhat.com>; qemu-devel@nongnu.org; > > pbonz...@redhat.com; marcandre.lur...@redhat.com; fel...@nutanix.com; > > Harris, James R <james.r.har...@intel.com> > > Subject: Re: [PATCH v4 1/4] vhost-user: add new vhost user messages to > > support > > virtio config space > > > > On Mon, Oct 23, 2017 at 04:47:00AM +0000, Liu, Changpeng wrote: > > > > > > > > > > -----Original Message----- > > > > From: Stefan Hajnoczi [mailto:stefa...@gmail.com] > > > > Sent: Friday, October 20, 2017 6:01 PM > > > > To: Michael S. Tsirkin <m...@redhat.com> > > > > Cc: Liu, Changpeng <changpeng....@intel.com>; qemu-devel@nongnu.org; > > > > pbonz...@redhat.com; marcandre.lur...@redhat.com; fel...@nutanix.com; > > > > Harris, James R <james.r.har...@intel.com> > > > > 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. > > > > We're talking about different things. Here is an example illustrating > > my question: > > > > vhost-user-blk slave A only knows about struct virtio_blk_config fields > > up to wce (VIRTIO 1.0). See > > http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1- > > 2070004. > > > > vhost-user-blk slave B implements struct virtio_blk_config with the new > > num_queues field. See > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/u > > api/linux/virtio_blk.h#n56. > > > > Slaves A and B use different struct virtio_blk_config sizes! > > > > Which config size should the vhost-master use? There is currently no > > way to query the size from the slave. > > > > What should slave programs do when the master requests configuration > > space data that is the wrong size? > > > > I think the simplest answer is that the master always uses 256 bytes. > > Slaves also keep the full 256 bytes stored but their device > > implementation may access fewer bytes. > Yes, clear now. How about the following configuration: > struct vhost_dev_config { > unsigned int offset; > unsigned int size; > uint8_t config[256]; > }; > The master always uses 256 Bytes, but with additional 2 parameters to locate > detailed configuration fields, I think this combined > Michael and your comments about this patch.
Yes, that's probably a good solution. Stefan
