On Wed, Jan 05, 2022 at 08:58:55AM +0800, Longpeng(Mike) wrote:
> From: Longpeng <longpe...@huawei.com>
>
> Implements the .realize interface.
>
> Signed-off-by: Longpeng <longpe...@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.
> +{
> + 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).
> +
> + 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.
> + 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.
signature.asc
Description: PGP signature
