> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+thanos.makatos=nutanix....@nongnu.org> On Behalf Of John
> Johnson
> Sent: 12 January 2022 00:44
> To: qemu-devel@nongnu.org
> Subject: [RFC v4 12/21] vfio-user: region read/write
>
> Add support for posted writes on remote devices
>
> Signed-off-by: Elena Ufimtseva <elena.ufimts...@oracle.com>
> Signed-off-by: John G Johnson <john.g.john...@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.ra...@oracle.com>
> ---
> hw/vfio/pci.h | 1 +
> hw/vfio/user-protocol.h | 12 +++++
> hw/vfio/user.h | 1 +
> include/hw/vfio/vfio-common.h | 7 +--
> hw/vfio/common.c | 10 +++-
> hw/vfio/pci.c | 9 +++-
> hw/vfio/user.c | 109
> ++++++++++++++++++++++++++++++++++++++++++
> 7 files changed, 143 insertions(+), 6 deletions(-)
>
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index ec9f345..643ff75 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -194,6 +194,7 @@ struct VFIOUserPCIDevice {
> VFIOPCIDevice device;
> char *sock_name;
> bool send_queued; /* all sends are queued */
> + bool no_post; /* all regions write are sync */
> };
>
> /* Use uin32_t for vendor & device so PCI_ANY_ID expands and cannot match
> hw */
> diff --git a/hw/vfio/user-protocol.h b/hw/vfio/user-protocol.h
> index caa523a..b1ea55f 100644
> --- a/hw/vfio/user-protocol.h
> +++ b/hw/vfio/user-protocol.h
> @@ -120,4 +120,16 @@ typedef struct {
> uint64_t offset;
> } VFIOUserRegionInfo;
>
> +/*
> + * VFIO_USER_REGION_READ
> + * VFIO_USER_REGION_WRITE
> + */
> +typedef struct {
> + VFIOUserHdr hdr;
> + uint64_t offset;
> + uint32_t region;
> + uint32_t count;
> + char data[];
> +} VFIOUserRegionRW;
> +
> #endif /* VFIO_USER_PROTOCOL_H */
> diff --git a/hw/vfio/user.h b/hw/vfio/user.h
> index 19edd84..f2098f2 100644
> --- a/hw/vfio/user.h
> +++ b/hw/vfio/user.h
> @@ -75,6 +75,7 @@ typedef struct VFIOProxy {
> /* VFIOProxy flags */
> #define VFIO_PROXY_CLIENT 0x1
> #define VFIO_PROXY_FORCE_QUEUED 0x4
> +#define VFIO_PROXY_NO_POST 0x8
>
> VFIOProxy *vfio_user_connect_dev(SocketAddress *addr, Error **errp);
> void vfio_user_disconnect(VFIOProxy *proxy);
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 2552557..4118b8a 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -57,6 +57,7 @@ typedef struct VFIORegion {
> VFIOMmap *mmaps;
> uint8_t nr; /* cache the region number for debug */
> int fd; /* fd to mmap() region */
> + bool post_wr; /* writes can be posted */
> } VFIORegion;
>
> typedef struct VFIOMigration {
> @@ -180,7 +181,7 @@ struct VFIODevIO {
> int (*region_read)(VFIODevice *vdev, uint8_t nr, off_t off, uint32_t
> size,
> void *data);
> int (*region_write)(VFIODevice *vdev, uint8_t nr, off_t off, uint32_t
> size,
> - void *data);
> + void *data, bool post);
> };
>
> #define VDEV_GET_INFO(vdev, info) \
> @@ -193,8 +194,8 @@ struct VFIODevIO {
> ((vdev)->io_ops->set_irqs((vdev), (irqs)))
> #define VDEV_REGION_READ(vdev, nr, off, size, data) \
> ((vdev)->io_ops->region_read((vdev), (nr), (off), (size), (data)))
> -#define VDEV_REGION_WRITE(vdev, nr, off, size, data) \
> - ((vdev)->io_ops->region_write((vdev), (nr), (off), (size), (data)))
> +#define VDEV_REGION_WRITE(vdev, nr, off, size, data, post) \
> + ((vdev)->io_ops->region_write((vdev), (nr), (off), (size), (data),
> (post)))
>
> struct VFIOContIO {
> int (*dma_map)(VFIOContainer *container,
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index a50bf4b..83cc5ec 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -213,6 +213,7 @@ void vfio_region_write(void *opaque, hwaddr addr,
> uint32_t dword;
> uint64_t qword;
> } buf;
> + bool post = region->post_wr;
> int ret;
>
> switch (size) {
> @@ -233,7 +234,11 @@ void vfio_region_write(void *opaque, hwaddr addr,
> break;
> }
>
> - ret = VDEV_REGION_WRITE(vbasedev, region->nr, addr, size, &buf);
> + /* read-after-write hazard if guest can directly access region */
> + if (region->nr_mmaps) {
> + post = false;
> + }
> + ret = VDEV_REGION_WRITE(vbasedev, region->nr, addr, size, &buf, post);
> if (ret != size) {
> const char *err = ret < 0 ? strerror(-ret) : "short write";
>
> @@ -1555,6 +1560,7 @@ int vfio_region_setup(Object *obj, VFIODevice
> *vbasedev, VFIORegion *region,
> region->size = info->size;
> region->fd_offset = info->offset;
> region->nr = index;
> + region->post_wr = false;
> if (vbasedev->regfds != NULL) {
> region->fd = vbasedev->regfds[index];
> } else {
> @@ -2689,7 +2695,7 @@ static int vfio_io_region_read(VFIODevice *vbasedev,
> uint8_t index, off_t off,
> }
>
> static int vfio_io_region_write(VFIODevice *vbasedev, uint8_t index, off_t
> off,
> - uint32_t size, void *data)
> + uint32_t size, void *data, bool post)
> {
> struct vfio_region_info *info = vbasedev->regions[index];
> int ret;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 6f85853..a4fd5e2 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -51,7 +51,7 @@
> (size), (data))
> #define VDEV_CONFIG_WRITE(vbasedev, off, size, data) \
> VDEV_REGION_WRITE((vbasedev), VFIO_PCI_CONFIG_REGION_INDEX, (off),
> \
> - (size), (data))
> + (size), (data), false)
>
> #define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug"
>
> @@ -1658,6 +1658,9 @@ static void vfio_bar_prepare(VFIOPCIDevice *vdev,
> int nr)
> bar->type = pci_bar & (bar->ioport ? ~PCI_BASE_ADDRESS_IO_MASK :
> ~PCI_BASE_ADDRESS_MEM_MASK);
> bar->size = bar->region.size;
> +
> + /* IO regions are sync, memory can be async */
> + bar->region.post_wr = (bar->ioport == 0);
> }
>
> static void vfio_bars_prepare(VFIOPCIDevice *vdev)
> @@ -3444,6 +3447,9 @@ static void vfio_user_pci_realize(PCIDevice *pdev,
> Error **errp)
> if (udev->send_queued) {
> proxy->flags |= VFIO_PROXY_FORCE_QUEUED;
> }
> + if (udev->no_post) {
> + proxy->flags |= VFIO_PROXY_NO_POST;
> + }
>
> vfio_user_validate_version(vbasedev, &err);
> if (err != NULL) {
> @@ -3503,6 +3509,7 @@ static void vfio_user_instance_finalize(Object *obj)
> static Property vfio_user_pci_dev_properties[] = {
> DEFINE_PROP_STRING("socket", VFIOUserPCIDevice, sock_name),
> DEFINE_PROP_BOOL("x-send-queued", VFIOUserPCIDevice, send_queued,
> false),
> + DEFINE_PROP_BOOL("x-no-posted-writes", VFIOUserPCIDevice, no_post,
IMO renaming this to "x-posted-writes" and defaulting it to true would be
easier to understand, but that's a matter of personal preference.
Could we have another option that would check the error code of the response
for writes, at least for debugging purposes? IIUC in real HW writes are posted
it isn't possible to check whether HW was happy with the write, but now we're
talking to emulated HW.
I suppose this new option would have to depend on writes not being posted.
> false),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/vfio/user.c b/hw/vfio/user.c
> index 1b0c9aa..09132a0 100644
> --- a/hw/vfio/user.c
> +++ b/hw/vfio/user.c
> @@ -50,6 +50,8 @@ static void vfio_user_cb(void *opaque);
>
> static void vfio_user_request(void *opaque);
> static int vfio_user_send_queued(VFIOProxy *proxy, VFIOUserMsg *msg);
> +static void vfio_user_send_async(VFIOProxy *proxy, VFIOUserHdr *hdr,
> + VFIOUserFDs *fds);
> static void vfio_user_send_wait(VFIOProxy *proxy, VFIOUserHdr *hdr,
> VFIOUserFDs *fds, int rsize, bool nobql);
> static void vfio_user_request_msg(VFIOUserHdr *hdr, uint16_t cmd,
> @@ -534,6 +536,33 @@ static int vfio_user_send_queued(VFIOProxy *proxy,
> VFIOUserMsg *msg)
> return 0;
> }
>
> +/*
> + * async send - msg can be queued, but will be freed when sent
> + */
> +static void vfio_user_send_async(VFIOProxy *proxy, VFIOUserHdr *hdr,
> + VFIOUserFDs *fds)
> +{
> + VFIOUserMsg *msg;
> + int ret;
> +
> + if (!(hdr->flags & (VFIO_USER_NO_REPLY | VFIO_USER_REPLY))) {
> + error_printf("vfio_user_send_async on sync message\n");
> + return;
> + }
> +
> + QEMU_LOCK_GUARD(&proxy->lock);
> +
> + msg = vfio_user_getmsg(proxy, hdr, fds);
> + msg->id = hdr->id;
> + msg->rsize = 0;
> + msg->type = VFIO_MSG_ASYNC;
> +
> + ret = vfio_user_send_queued(proxy, msg);
> + if (ret < 0) {
> + vfio_user_recycle(proxy, msg);
> + }
> +}
> +
> static void vfio_user_send_wait(VFIOProxy *proxy, VFIOUserHdr *hdr,
> VFIOUserFDs *fds, int rsize, bool nobql)
> {
> @@ -959,6 +988,70 @@ static int vfio_user_get_region_info(VFIOProxy *proxy,
> return 0;
> }
>
> +static int vfio_user_region_read(VFIOProxy *proxy, uint8_t index, off_t
> offset,
> + uint32_t count, void *data)
> +{
> + g_autofree VFIOUserRegionRW *msgp = NULL;
> + int size = sizeof(*msgp) + count;
> +
> + if (count > max_xfer_size) {
> + return -EINVAL;
> + }
> +
> + 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_wait(proxy, &msgp->hdr, NULL, size, false);
> + 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;
> +}
> +
> +static int vfio_user_region_write(VFIOProxy *proxy, uint8_t index, off_t
> offset,
> + uint32_t count, void *data, bool post)
> +{
> + VFIOUserRegionRW *msgp = NULL;
> + int flags = post ? VFIO_USER_NO_REPLY : 0;
> + int size = sizeof(*msgp) + count;
> + int ret;
> +
> + if (count > max_xfer_size) {
> + return -EINVAL;
> + }
> +
> + msgp = g_malloc0(size);
> + vfio_user_request_msg(&msgp->hdr, VFIO_USER_REGION_WRITE, size,
> flags);
> + msgp->offset = offset;
> + msgp->region = index;
> + msgp->count = count;
> + memcpy(&msgp->data, data, count);
> +
> + /* async send will free msg after it's sent */
> + if (post && !(proxy->flags & VFIO_PROXY_NO_POST)) {
> + vfio_user_send_async(proxy, &msgp->hdr, NULL);
> + return count;
> + }
> +
> + vfio_user_send_wait(proxy, &msgp->hdr, NULL, 0, false);
> + if (msgp->hdr.flags & VFIO_USER_ERROR) {
> + ret = -msgp->hdr.error_reply;
> + } else {
> + ret = count;
> + }
> +
> + g_free(msgp);
> + return ret;
> +}
> +
>
> /*
> * Socket-based io_ops
> @@ -1005,8 +1098,24 @@ static int vfio_user_io_get_region_info(VFIODevice
> *vbasedev,
> return 0;
> }
>
> +static int vfio_user_io_region_read(VFIODevice *vbasedev, uint8_t index,
> + off_t off, uint32_t size, void *data)
> +{
> + return vfio_user_region_read(vbasedev->proxy, index, off, size, data);
> +}
> +
> +static int vfio_user_io_region_write(VFIODevice *vbasedev, uint8_t index,
> + off_t off, unsigned size, void *data,
> + bool post)
> +{
> + return vfio_user_region_write(vbasedev->proxy, index, off, size, data,
> + post);
> +}
> +
> VFIODevIO vfio_dev_io_sock = {
> .get_info = vfio_user_io_get_info,
> .get_region_info = vfio_user_io_get_region_info,
> + .region_read = vfio_user_io_region_read,
> + .region_write = vfio_user_io_region_write,
> };
>
> --
> 1.8.3.1
>
