Re: [PATCH v1 09/10] vhost-vdpa: introduce vhost-vdpa backend

[Laurent Vivier <lvivier@redhat.com>]

On 22/06/2020 17:37, Cindy Lu wrote:
> Currently we have 2 types of vhost backends in QEMU: vhost kernel and
> vhost-user. The above patch provides a generic device for vDPA purpose,
> this vDPA device exposes to user space a non-vendor-specific configuration
> interface for setting up a vhost HW accelerator, this patch set introduces
> a third vhost backend called vhost-vdpa based on the vDPA interface.
> 
> Vhost-vdpa usage:
> 
> qemu-system-x86_64 -cpu host -enable-kvm \
>     ......
>     -netdev type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-id,id=vhost-vdpa0 \
>     -device virtio-net-pci,netdev=vhost-vdpa0,page-per-vq=on \
> 
> Signed-off-by: Lingshan zhu <lingshan.zhu@intel.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  configure                         |  21 ++
>  hw/net/vhost_net.c                |  19 +-
>  hw/net/virtio-net.c               |  19 +-
>  hw/virtio/Makefile.objs           |   1 +
>  hw/virtio/vhost-backend.c         |  22 +-
>  hw/virtio/vhost-vdpa.c            | 406 ++++++++++++++++++++++++++++++
>  hw/virtio/vhost.c                 |  42 +++-
>  include/hw/virtio/vhost-backend.h |   6 +-
>  include/hw/virtio/vhost-vdpa.h    |  26 ++
>  include/hw/virtio/vhost.h         |   6 +
>  qemu-options.hx                   |  12 +
>  11 files changed, 555 insertions(+), 25 deletions(-)
>  create mode 100644 hw/virtio/vhost-vdpa.c
>  create mode 100644 include/hw/virtio/vhost-vdpa.h
> 
...
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 660e9e8588..84e5b1a833 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -14,7 +14,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/main-loop.h"
>  #include "standard-headers/linux/vhost_types.h"
> -
> +#include "hw/virtio/vhost-vdpa.h"
>  #ifdef CONFIG_VHOST_KERNEL
>  #include <linux/vhost.h>
>  #include <sys/ioctl.h>
> @@ -22,10 +22,19 @@
>  static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
>                               void *arg)
>  {
> -    int fd = (uintptr_t) dev->opaque;
> -
> -    assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL);
> +    int fd = -1;

You don't need to initialize fd before the switch() because all cases
will set a value to it or assert.

> +    switch (dev->vhost_ops->backend_type) {
> +    case VHOST_BACKEND_TYPE_KERNEL:
> +        fd  = (uintptr_t)dev->opaque;
> +        break;
> +    case VHOST_BACKEND_TYPE_VDPA:
> +        fd = ((struct vhost_vdpa *)dev->opaque)->device_fd;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
>  
> +    assert(fd != -1);

Perhaps this assert is not needed:
Unitialized value will be catched by "default:", and there was no such
kind of check on "(uintptr_t)dev->opaque" before.

Thanks,
Laurent