Re: [RFC 04/10] vdpa-dev: implement the instance_init/class_init interface

[Stefan Hajnoczi]

On Wed, Jan 05, 2022 at 08:58:54AM +0800, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> Implements the .instance_init and the .class_init interface.
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
>  hw/virtio/vdpa-dev-pci.c     | 80 +++++++++++++++++++++++++++++++++++-
>  hw/virtio/vdpa-dev.c         | 68 +++++++++++++++++++++++++++++-
>  include/hw/virtio/vdpa-dev.h |  2 +
>  3 files changed, 146 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/vdpa-dev-pci.c b/hw/virtio/vdpa-dev-pci.c
> index a5a7b528a9..0af54a26d4 100644
> --- a/hw/virtio/vdpa-dev-pci.c
> +++ b/hw/virtio/vdpa-dev-pci.c
> @@ -23,14 +23,90 @@ struct VhostVdpaDevicePCI {
>      VhostVdpaDevice vdev;
>  };
>  
> +static uint32_t
> +vdpa_dev_pci_get_info(const char *name, uint64_t cmd, Error **errp)

vdpa_dev_pci_get_u32() might be a clearer name.

> +{
> +    int device_fd;
> +    uint32_t val;
> +    int ret;
> +
> +    device_fd = qemu_open(name, O_RDWR, errp);
> +    if (device_fd == -1) {
> +        return (uint32_t)-1;
> +    }
> +
> +    ret = ioctl(device_fd, cmd, &val);
> +    if (ret < 0) {
> +        error_setg(errp, "vhost-vdpa-device-pci: cmd 0x%lx failed: %s",
> +                   cmd, strerror(errno));
> +        goto out;
> +    }
> +
> +out:
> +    close(device_fd);

Race conditions are possible if the device node is replaced between
calls. Why not open the file once and reuse the fd across ioctl calls?

Both VhostVdpaDevicePCI and VhostVdpaDevice need the fd but
VhostVdpaDevicePCI needs it first. I suggest passing ownership of the fd
to the VhostVdpaDevice. One way of doing this is using QOM properties so
that VhostVdpaDevice can use the given fd instead of reopening the file.
(If fd is -1 then VhostVdpaDevice can try to open the file. That is
necessary when VhostVdpaDevice is used directly with virtio-mmio because
there is no proxy object.)

signature.asc
Description: PGP signature