On Mon, Jan 17, 2022 at 12:34:50PM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote: > > > > -----Original Message----- > > From: Stefan Hajnoczi [mailto:stefanha@redhat.com] > > Sent: Thursday, January 6, 2022 7:34 PM > > To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > > > > Cc: mst@redhat.com; jasowang@redhat.com; sgarzare@redhat.com; > > cohuck@redhat.com; pbonzini@redhat.com; Gonglei (Arei) > > ; Yechuan ; Huangzhichao > > ; qemu-devel@nongnu.org > > Subject: Re: [RFC 05/10] vdpa-dev: implement the realize interface > > > > On Thu, Jan 06, 2022 at 03:02:37AM +0000, Longpeng (Mike, Cloud Infrastructure > > Service Product Dept.) wrote: > > > > > > > > > > -----Original Message----- > > > > From: Stefan Hajnoczi [mailto:stefanha@redhat.com] > > > > Sent: Wednesday, January 5, 2022 6:18 PM > > > > To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) > > > > > > > > Cc: mst@redhat.com; jasowang@redhat.com; sgarzare@redhat.com; > > > > cohuck@redhat.com; pbonzini@redhat.com; Gonglei (Arei) > > > > ; Yechuan ; Huangzhichao > > > > ; qemu-devel@nongnu.org > > > > Subject: Re: [RFC 05/10] vdpa-dev: implement the realize interface > > > > > > > > On Wed, Jan 05, 2022 at 08:58:55AM +0800, Longpeng(Mike) wrote: > > > > > From: Longpeng > > > > > > > > > > Implements the .realize interface. > > > > > > > > > > Signed-off-by: Longpeng > > > > > --- > > > > > hw/virtio/vdpa-dev.c | 114 +++++++++++++++++++++++++++++++++++ > > > > > include/hw/virtio/vdpa-dev.h | 8 +++ > > > > > 2 files changed, 122 insertions(+) > > > > > > > > > > diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c > > > > > index 790117fb3b..2d534d837a 100644 > > > > > --- a/hw/virtio/vdpa-dev.c > > > > > +++ b/hw/virtio/vdpa-dev.c > > > > > @@ -15,9 +15,122 @@ > > > > > #include "sysemu/sysemu.h" > > > > > #include "sysemu/runstate.h" > > > > > > > > > > +static void > > > > > +vhost_vdpa_device_dummy_handle_output(VirtIODevice *vdev, VirtQueue > > *vq) > > > > > +{ > > > > > + /* Nothing to do */ > > > > > +} > > > > > + > > > > > +static int vdpa_dev_get_info_by_fd(int fd, uint64_t cmd, Error **errp) > > > > > > > > This looks similar to the helper function in a previous patch but this > > > > time the return value type is int instead of uint32_t. Please make the > > > > types consistent. > > > > > > > > > > OK. > > > > > > > > +{ > > > > > + int val; > > > > > + > > > > > + if (ioctl(fd, cmd, &val) < 0) { > > > > > + error_setg(errp, "vhost-vdpa-device: cmd 0x%lx failed: %s", > > > > > + cmd, strerror(errno)); > > > > > + return -1; > > > > > + } > > > > > + > > > > > + return val; > > > > > +} > > > > > + > > > > > +static inline int vdpa_dev_get_queue_size(int fd, Error **errp) > > > > > +{ > > > > > + return vdpa_dev_get_info_by_fd(fd, VHOST_VDPA_GET_VRING_NUM, errp); > > > > > +} > > > > > + > > > > > +static inline int vdpa_dev_get_vqs_num(int fd, Error **errp) > > > > > +{ > > > > > + return vdpa_dev_get_info_by_fd(fd, VHOST_VDPA_GET_VQS_NUM, errp); > > > > > +} > > > > > + > > > > > +static inline int vdpa_dev_get_config_size(int fd, Error **errp) > > > > > +{ > > > > > + return vdpa_dev_get_info_by_fd(fd, VHOST_VDPA_GET_CONFIG_SIZE, > > errp); > > > > > +} > > > > > + > > > > > static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp) > > > > > { > > > > > + VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > > > > + VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev); > > > > > + uint32_t device_id; > > > > > + int max_queue_size; > > > > > + int fd; > > > > > + int i, ret; > > > > > + > > > > > + fd = qemu_open(s->vdpa_dev, O_RDWR, errp); > > > > > + if (fd == -1) { > > > > > + return; > > > > > + } > > > > > + s->vdpa.device_fd = fd; > > > > > > > > This is the field I suggest exposing as a QOM property so it can be set > > > > from the proxy object (e.g. when the PCI proxy opens the vdpa device > > > > before our .realize() function is called). > > > > > > > > > > OK. > > > > > > > > + > > > > > + max_queue_size = vdpa_dev_get_queue_size(fd, errp); > > > > > + if (*errp) { > > > > > + goto out; > > > > > + } > > > > > + > > > > > + if (s->queue_size > max_queue_size) { > > > > > + error_setg(errp, "vhost-vdpa-device: invalid queue_size: %d > > > > (max:%d)", > > > > > + s->queue_size, max_queue_size); > > > > > + goto out; > > > > > + } else if (!s->queue_size) { > > > > > + s->queue_size = max_queue_size; > > > > > + } > > > > > + > > > > > + ret = vdpa_dev_get_vqs_num(fd, errp); > > > > > + if (*errp) { > > > > > + goto out; > > > > > + } > > > > > + > > > > > + s->dev.nvqs = ret; > > > > > > > > There is no input validation because we trust the kernel vDPA return > > > > values. That seems okay for now but if there is a vhost-user version of > > > > this in the future then input validation will be necessary to achieve > > > > isolation between QEMU and the vhost-user processes. I suggest including > > > > input validation code right away because it's harder to audit the code > > > > and fix missing input validation later on. > > > > > > > > > > Make sense! > > > > > > Should we only need to validate the upper boundary (e.g. > > > Careful, ret is currently an int so negative values would bypass the < > > VIRTIO_QUEUE_MAX check. > > > > > > > > > > + s->dev.vqs = g_new0(struct vhost_virtqueue, s->dev.nvqs); > > > > > + s->dev.vq_index = 0; > > > > > + s->dev.vq_index_end = s->dev.nvqs; > > > > > + s->dev.backend_features = 0; > > > > > + s->started = false; > > > > > + > > > > > + ret = vhost_dev_init(&s->dev, &s->vdpa, VHOST_BACKEND_TYPE_VDPA, 0, > > > > NULL); > > > > > + if (ret < 0) { > > > > > + error_setg(errp, "vhost-vdpa-device: vhost initialization > > > > failed: %s", > > > > > + strerror(-ret)); > > > > > + goto out; > > > > > + } > > > > > + > > > > > + ret = s->dev.vhost_ops->vhost_get_device_id(&s->dev, &device_id); > > > > > > > > The vhost_*() API abstracts the ioctl calls but this source file and the > > > > PCI proxy have ioctl calls. I wonder if it's possible to move the ioctls > > > > calls into the vhost_*() API? That would be cleaner and also make it > > > > easier to add vhost-user vDPA support in the future. > > > > > > We need these ioctls calls because we need invoke them before the vhost-dev > > > object is initialized. > > > > It may be possible to clean this up by changing how vhost_dev_init() > > works but I haven't investigated. The issue is that the vhost_dev_init() > > API requires information from the caller that has to be fetched from the > > vDPA device. This forces the caller to communicate directly with the > > vDPA device before calling vhost_dev_init(). It may be possible to move > > this setup code inside vhost_dev_init() (and vhost_ops callbacks). > > > > Hmm, this is still not clear to me, so let's continue to discuss this > in v2 if you think it's necessary. Okay. Stefan