[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space |
Date: |
Tue, 24 Oct 2017 15:00:37 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Tue, Oct 24, 2017 at 12:52:28AM +0000, Liu, Changpeng wrote:
>
>
> > -----Original Message-----
> > From: Stefan Hajnoczi [mailto:address@hidden
> > Sent: Tuesday, October 24, 2017 1:26 AM
> > To: Liu, Changpeng <address@hidden>
> > Cc: Michael S. Tsirkin <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 Mon, Oct 23, 2017 at 04:47:00AM +0000, Liu, Changpeng wrote:
> > >
> > >
> > > > -----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.
> >
> > 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
- [Qemu-devel] [PATCH v4 0/4] *** Introduce a new vhost-user-blk host device to Qemu ***, Changpeng Liu, 2017/10/19
- [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Changpeng Liu, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Paolo Bonzini, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Stefan Hajnoczi, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Michael S. Tsirkin, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Stefan Hajnoczi, 2017/10/20
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Liu, Changpeng, 2017/10/23
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Stefan Hajnoczi, 2017/10/23
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Liu, Changpeng, 2017/10/23
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space,
Stefan Hajnoczi <=
Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Michael S. Tsirkin, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Paolo Bonzini, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Michael S. Tsirkin, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Paolo Bonzini, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Michael S. Tsirkin, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Liu, Changpeng, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Michael S. Tsirkin, 2017/10/19
- Re: [Qemu-devel] [PATCH v4 1/4] vhost-user: add new vhost user messages to support virtio config space, Liu, Changpeng, 2017/10/19
[Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device, Changpeng Liu, 2017/10/19