Re: [PATCH RFC v2 08/16] vfio-user: get region info

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3879 bytes --]

On Mon, Aug 16, 2021 at 09:42:41AM -0700, Elena Ufimtseva wrote:
> @@ -1514,6 +1515,16 @@ bool vfio_get_info_dma_avail(struct vfio_iommu_type1_info *info,
>      return true;
>  }
>  
> +static int vfio_get_region_info_remfd(VFIODevice *vbasedev, int index)
> +{
> +    struct vfio_region_info *info;
> +
> +    if (vbasedev->regions == NULL || vbasedev->regions[index] == NULL) {
> +        vfio_get_region_info(vbasedev, index, &info);
> +    }

Maybe this will be called from other places in the future, but the
vfio_region_setup() caller below already invoked vfio_get_region_info()
so I'm not sure it's necessary to do this again?

Perhaps add an int *remfd argument to vfio_get_region_info(). That way
vfio_get_region_info_remfd() isn't necessary.

> @@ -2410,6 +2442,24 @@ int vfio_get_region_info(VFIODevice *vbasedev, int index,
>                           struct vfio_region_info **info)
>  {
>      size_t argsz = sizeof(struct vfio_region_info);
> +    int fd = -1;
> +    int ret;
> +
> +    /* create region cache */
> +    if (vbasedev->regions == NULL) {
> +        vbasedev->regions = g_new0(struct vfio_region_info *,
> +                                   vbasedev->num_regions);
> +        if (vbasedev->proxy != NULL) {
> +            vbasedev->regfds = g_new0(int, vbasedev->num_regions);
> +        }
> +    }
> +    /* check cache */
> +    if (vbasedev->regions[index] != NULL) {
> +        *info = g_malloc0(vbasedev->regions[index]->argsz);
> +        memcpy(*info, vbasedev->regions[index],
> +               vbasedev->regions[index]->argsz);
> +        return 0;
> +    }

Why is it necessary to introduce a cache? Is it to avoid passing the
same fd multiple times?

>  
>      *info = g_malloc0(argsz);
>  
> @@ -2417,7 +2467,17 @@ int vfio_get_region_info(VFIODevice *vbasedev, int index,
>  retry:
>      (*info)->argsz = argsz;
>  
> -    if (ioctl(vbasedev->fd, VFIO_DEVICE_GET_REGION_INFO, *info)) {
> +    if (vbasedev->proxy != NULL) {
> +        VFIOUserFDs fds = { 0, 1, &fd};
> +
> +        ret = vfio_user_get_region_info(vbasedev, index, *info, &fds);
> +    } else {
> +        ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_REGION_INFO, *info);
> +        if (ret < 0) {
> +            ret = -errno;
> +        }
> +    }
> +    if (ret != 0) {
>          g_free(*info);
>          *info = NULL;
>          return -errno;
> @@ -2426,10 +2486,22 @@ retry:
>      if ((*info)->argsz > argsz) {
>          argsz = (*info)->argsz;
>          *info = g_realloc(*info, argsz);
> +        if (fd != -1) {
> +            close(fd);
> +            fd = -1;
> +        }
>  
>          goto retry;
>      }
>  
> +    /* fill cache */
> +    vbasedev->regions[index] = g_malloc0(argsz);
> +    memcpy(vbasedev->regions[index], *info, argsz);
> +    *vbasedev->regions[index] = **info;

The previous line already copied the contents of *info. What is the
purpose of this assignment?

> +    if (vbasedev->regfds != NULL) {
> +        vbasedev->regfds[index] = fd;
> +    }
> +
>      return 0;
>  }
>  
> diff --git a/hw/vfio/user.c b/hw/vfio/user.c
> index b584b8e0f2..91b51f37df 100644
> --- a/hw/vfio/user.c
> +++ b/hw/vfio/user.c
> @@ -734,3 +734,36 @@ int vfio_user_get_info(VFIODevice *vbasedev)
>      vbasedev->reset_works = !!(msg.flags & VFIO_DEVICE_FLAGS_RESET);
>      return 0;
>  }
> +
> +int vfio_user_get_region_info(VFIODevice *vbasedev, int index,
> +                              struct vfio_region_info *info, VFIOUserFDs *fds)
> +{
> +    g_autofree VFIOUserRegionInfo *msgp = NULL;
> +    int size;

Please use uint32_t size instead of int size to prevent possible
signedness issues:
- VFIOUserRegionInfo->argsz is uint32_t.
- sizeof(VFIOUserHdr) is size_t.
- The vfio_user_request_msg() size argument is uint32_t.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]