Re: [PATCH RFC v2 09/16] vfio-user: region read/write

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Mon, Aug 16, 2021 at 09:42:42AM -0700, Elena Ufimtseva wrote:
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 7d667b0533..a8b1ea9358 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -215,6 +215,7 @@ void vfio_region_write(void *opaque, hwaddr addr,
>          uint32_t dword;
>          uint64_t qword;
>      } buf;
> +    int ret;
>  
>      switch (size) {
>      case 1:
> @@ -234,7 +235,12 @@ void vfio_region_write(void *opaque, hwaddr addr,
>          break;
>      }
>  
> -    if (pwrite(vbasedev->fd, &buf, size, region->fd_offset + addr) != size) {
> +    if (vbasedev->proxy != NULL) {
> +        ret = vfio_user_region_write(vbasedev, region->nr, addr, size, &data);
> +    } else {
> +        ret = pwrite(vbasedev->fd, &buf, size, region->fd_offset + addr);
> +    }

The vfio-user spec says everything is little-endian. Why does
vfio_user_region_write() take the host-endian uint64_t data value
instead of the little-endian buf value?

> +    if (ret != size) {
>          error_report("%s(%s:region%d+0x%"HWADDR_PRIx", 0x%"PRIx64
>                       ",%d) failed: %m",
>                       __func__, vbasedev->name, region->nr,
> @@ -266,8 +272,14 @@ uint64_t vfio_region_read(void *opaque,
>          uint64_t qword;
>      } buf;
>      uint64_t data = 0;
> +    int ret;
>  
> -    if (pread(vbasedev->fd, &buf, size, region->fd_offset + addr) != size) {
> +    if (vbasedev->proxy != NULL) {
> +        ret = vfio_user_region_read(vbasedev, region->nr, addr, size, &buf);
> +    } else {
> +        ret = pread(vbasedev->fd, &buf, size, region->fd_offset + addr);
> +    }
> +    if (ret != size) {
>          error_report("%s(%s:region%d+0x%"HWADDR_PRIx", %d) failed: %m",
>                       __func__, vbasedev->name, region->nr,
>                       addr, size);
> diff --git a/hw/vfio/user.c b/hw/vfio/user.c
> index 91b51f37df..83235b2411 100644
> --- a/hw/vfio/user.c
> +++ b/hw/vfio/user.c
> @@ -767,3 +767,46 @@ int vfio_user_get_region_info(VFIODevice *vbasedev, int index,
>      memcpy(info, &msgp->argsz, info->argsz);
>      return 0;
>  }
> +
> +int vfio_user_region_read(VFIODevice *vbasedev, uint32_t index, uint64_t offset,
> +                                 uint32_t count, void *data)
> +{
> +    g_autofree VFIOUserRegionRW *msgp = NULL;
> +    int size = sizeof(*msgp) + count;
> +
> +    msgp = g_malloc0(size);
> +    vfio_user_request_msg(&msgp->hdr, VFIO_USER_REGION_READ, sizeof(*msgp), 0);
> +    msgp->offset = offset;
> +    msgp->region = index;
> +    msgp->count = count;
> +
> +    vfio_user_send_recv(vbasedev->proxy, &msgp->hdr, NULL, size, 0);
> +    if (msgp->hdr.flags & VFIO_USER_ERROR) {
> +        return -msgp->hdr.error_reply;
> +    } else if (msgp->count > count) {
> +        return -E2BIG;
> +    } else {
> +        memcpy(data, &msgp->data, msgp->count);
> +    }
> +
> +    return msgp->count;
> +}
> +
> +int vfio_user_region_write(VFIODevice *vbasedev, uint32_t index,
> +                           uint64_t offset, uint32_t count, void *data)
> +{
> +    g_autofree VFIOUserRegionRW *msgp = NULL;
> +    int size = sizeof(*msgp) + count;
> +
> +    msgp = g_malloc0(size);
> +    vfio_user_request_msg(&msgp->hdr, VFIO_USER_REGION_WRITE, size,
> +                          VFIO_USER_NO_REPLY);
> +    msgp->offset = offset;
> +    msgp->region = index;
> +    msgp->count = count;
> +    memcpy(&msgp->data, data, count);
> +
> +    vfio_user_send(vbasedev->proxy, &msgp->hdr, NULL);

Are VFIO region writes posted writes (VFIO_USER_NO_REPLY)? This can be a
problem if the device driver performs a write to the region followed by
another access (e.g. to an mmap region) and expected the write to
complete before the second access takes place.

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