From: Thanos Makatos <thanos.makatos@nutanix.com>
To: John Johnson <john.g.johnson@oracle.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: RE: [RFC v4 12/21] vfio-user: region read/write
Date: Wed, 26 Jan 2022 21:57:06 +0000 [thread overview]
Message-ID: <DM8PR02MB8005A695D0FF504C07DFC3758B209@DM8PR02MB8005.namprd02.prod.outlook.com> (raw)
In-Reply-To: <0fbe8c0935af73fc12eff1f6c919387a9ecad5fd.1641584317.git.john.g.johnson@oracle.com>
> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+thanos.makatos=nutanix.com@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.ufimtseva@oracle.com>
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> Signed-off-by: Jagannathan Raman <jag.raman@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
>
next prev parent reply other threads:[~2022-01-26 21:59 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-12 0:43 [RFC v4 00/21] vfio-user client John Johnson
2022-01-12 0:43 ` [RFC v4 01/21] vfio-user: introduce vfio-user protocol specification John Johnson
2022-02-14 13:10 ` Thanos Makatos
2022-03-09 22:34 ` Alex Williamson
2022-03-10 10:20 ` John Levon
2022-03-14 6:04 ` John Johnson
2022-03-15 21:43 ` Thanos Makatos
2022-03-15 22:28 ` Alex Williamson
2022-07-22 6:23 ` John Johnson
2022-01-12 0:43 ` [RFC v4 02/21] vfio-user: add VFIO base abstract class John Johnson
2022-01-12 0:43 ` [RFC v4 03/21] vfio-user: add container IO ops vector John Johnson
2022-01-12 0:43 ` [RFC v4 04/21] vfio-user: add region cache John Johnson
2022-03-09 23:40 ` Alex Williamson
2022-01-12 0:43 ` [RFC v4 05/21] vfio-user: add device IO ops vector John Johnson
2022-01-12 0:43 ` [RFC v4 06/21] vfio-user: Define type vfio_user_pci_dev_info John Johnson
2022-01-12 0:43 ` [RFC v4 07/21] vfio-user: connect vfio proxy to remote server John Johnson
2022-01-12 0:43 ` [RFC v4 08/21] vfio-user: define socket receive functions John Johnson
2022-02-03 21:53 ` Thanos Makatos
2022-02-04 12:42 ` Thanos Makatos
2022-02-07 7:07 ` John Johnson
2022-02-15 13:35 ` Thanos Makatos
2022-02-15 14:50 ` Thanos Makatos
2022-02-16 2:09 ` John Johnson
2022-02-16 9:31 ` Thanos Makatos
2022-01-12 0:43 ` [RFC v4 09/21] vfio-user: define socket send functions John Johnson
2022-01-26 10:17 ` Thanos Makatos
2022-02-07 7:09 ` John Johnson
2022-01-12 0:43 ` [RFC v4 10/21] vfio-user: get device info John Johnson
2022-01-12 0:43 ` [RFC v4 11/21] vfio-user: get region info John Johnson
2022-01-12 0:43 ` [RFC v4 12/21] vfio-user: region read/write John Johnson
2022-01-26 21:57 ` Thanos Makatos [this message]
2022-01-12 0:43 ` [RFC v4 13/21] vfio-user: pci_user_realize PCI setup John Johnson
2022-01-12 0:43 ` [RFC v4 14/21] vfio-user: get and set IRQs John Johnson
2022-01-12 0:43 ` [RFC v4 15/21] vfio-user: proxy container connect/disconnect John Johnson
2022-01-12 0:43 ` [RFC v4 16/21] vfio-user: dma map/unmap operations John Johnson
2022-01-12 0:43 ` [RFC v4 17/21] vfio-user: secure DMA support John Johnson
2022-01-12 0:43 ` [RFC v4 18/21] vfio-user: dma read/write operations John Johnson
2022-01-12 0:43 ` [RFC v4 19/21] vfio-user: pci reset John Johnson
2022-01-12 0:43 ` [RFC v4 20/21] vfio-user: migration support John Johnson
2022-02-11 13:31 ` Thanos Makatos
2022-02-14 18:50 ` John Johnson
2022-02-15 14:53 ` Thanos Makatos
2022-01-12 0:43 ` [RFC v4 21/21] Only set qemu file error if saving state so the file exists John Johnson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM8PR02MB8005A695D0FF504C07DFC3758B209@DM8PR02MB8005.namprd02.prod.outlook.com \
--to=thanos.makatos@nutanix.com \
--cc=john.g.johnson@oracle.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.