[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC 05/10] vdpa-dev: implement the realize interface
From: |
Stefan Hajnoczi |
Subject: |
Re: [RFC 05/10] vdpa-dev: implement the realize interface |
Date: |
Thu, 6 Jan 2022 11:34:23 +0000 |
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.)
> > <longpeng2@huawei.com>
> > Cc: mst@redhat.com; jasowang@redhat.com; sgarzare@redhat.com;
> > cohuck@redhat.com; pbonzini@redhat.com; Gonglei (Arei)
> > <arei.gonglei@huawei.com>; Yechuan <yechuan@huawei.com>; Huangzhichao
> > <huangzhichao@huawei.com>; 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 <longpeng2@huawei.com>
> > >
> > > Implements the .realize interface.
> > >
> > > Signed-off-by: Longpeng <longpeng2@huawei.com>
> > > ---
> > > 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. <VIRTIO_QUEUE_MAX)?
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).
Stefan
signature.asc
Description: PGP signature
- RE: [RFC 06/10] vdpa-dev: implement the unrealize interface, (continued)
[RFC 04/10] vdpa-dev: implement the instance_init/class_init interface, Longpeng(Mike), 2022/01/04
[RFC 05/10] vdpa-dev: implement the realize interface, Longpeng(Mike), 2022/01/04
Re: [RFC 00/10] add generic vDPA device support, Stefan Hajnoczi, 2022/01/05