> On Sep 7, 2021, at 7:31 AM, Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> 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.
>
I think they could be combined, but the region capabilities are
retrieved with separate calls to vfio_get_region_info_cap(), so I followed
that precedent.
>> @@ -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?
>
Yes. For polled regions, the server returns an FD with each
message, so there would be an FD leak if I didn’t check that the region
hadn’t already returned one. Since I had to cache the FD anyway, I
cached the whole struct.
>>
>> *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?
>
That might be a mis-merge. The struct assignment was a bug
fixed several revs ago with the memcpy() call.
>> + 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.
OK
JJ
