[RESEND,v6,21/36] multi-process: PCI BAR read/write handling for proxy & remote endpoints

Commit Message

Elena Ufimtseva April 23, 2020, 4:13 a.m. UTC

From: Jagannathan Raman <[e-mail]>

Proxy device object implements handler for PCI BAR writes and reads.
The handler uses BAR_WRITE/BAR_READ message to communicate to the
remote process with the BAR address and value to be written/read.
The remote process implements handler for BAR_WRITE/BAR_READ
message.

Signed-off-by: Jagannathan Raman <[e-mail]>
Signed-off-by: Elena Ufimtseva <[e-mail]>
Signed-off-by: John G Johnson <[e-mail]>
---
 hw/proxy/qemu-proxy.c         | 64 ++++++++++++++++++++++++++
 include/hw/proxy/qemu-proxy.h | 20 ++++++++-
 include/io/mpqemu-link.h      | 12 +++++
 io/mpqemu-link.c              |  6 +++
 remote/remote-main.c          | 84 +++++++++++++++++++++++++++++++++++
 5 files changed, 184 insertions(+), 2 deletions(-)

Comments

Stefan Hajnoczi May 12, 2020, 2:19 p.m. UTC | #1

On Wed, Apr 22, 2020 at 09:13:56PM -0700, [e-mail] wrote:
> +uint64_t proxy_default_bar_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    ProxyMemoryRegion *pmr = opaque;
> +    uint64_t val;
> +
> +    send_bar_access_msg(pmr->dev, &pmr->mr, false, addr, &val, size,
> +                        pmr->memory);
> +
> +     return val;

Indentation is should be 4 spaces.

> @@ -43,4 +54,9 @@ typedef struct PCIProxyDevClass {
>      char *command;
>  } PCIProxyDevClass;
>  
> +void proxy_default_bar_write(void *opaque, hwaddr addr, uint64_t val,
> +                             unsigned size);
> +
> +uint64_t proxy_default_bar_read(void *opaque, hwaddr addr, unsigned size);

"default" is probably not appropriate here. This isn't a base class'
default implementation that child classes override.

> @@ -114,6 +115,77 @@ exit:
>      notify_proxy(wait, ret);
>  }
>  
> +/* TODO: confirm memtx attrs. */
> +static void process_bar_write(MPQemuMsg *msg, Error **errp)
> +{
> +    bar_access_msg_t *bar_access = &msg->data1.bar_access;
> +    AddressSpace *as =
> +        bar_access->memory ? &address_space_memory : &address_space_io;
> +    MemTxResult res;
> +
> +    assert(is_power_of_2(bar_access->size) &&
> +           bar_access->size <= sizeof(uint64_t));

Please return an error instead of asserting. At the moment it doesn't
matter much but this will be necessary so that a process hosting remote
devices for multiple guests cannot be killed by one malicious guest.

> +
> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
> +                           (uint8_t *)&bar_access->val, bar_access->size,

Missing uint64_t val to void* conversion code.

For example, if bar_access->size is 1 then this fails on big-endian
hosts because &bar_access->val is the address of the most-significant
byte in uint64_t val:

  uint64_t val = 1;

Now we have the following memory layout on big-endian hosts:

  val = 00 00 00 00  00 00 00 01
        ^-------------- &val

We actually wanted a 1-byte write of 0x01 and not 0x00.

Patch

diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c
index 87cf39c672..7fd0a312a5 100644
--- a/hw/proxy/qemu-proxy.c
+++ b/hw/proxy/qemu-proxy.c
@@ -169,3 +169,67 @@  static void pci_proxy_dev_register_types(void)
 
 type_init(pci_proxy_dev_register_types)
 
+static void send_bar_access_msg(PCIProxyDev *dev, MemoryRegion *mr,
+                                bool write, hwaddr addr, uint64_t *val,
+                                unsigned size, bool memory)
+{
+    MPQemuLinkState *mpqemu_link = dev->mpqemu_link;
+    MPQemuMsg msg;
+    int wait;
+
+    memset(&msg, 0, sizeof(MPQemuMsg));
+
+    msg.bytestream = 0;
+    msg.size = sizeof(msg.data1);
+    msg.data1.bar_access.addr = mr->addr + addr;
+    msg.data1.bar_access.size = size;
+    msg.data1.bar_access.memory = memory;
+
+    if (write) {
+        msg.cmd = BAR_WRITE;
+        msg.data1.bar_access.val = *val;
+    } else {
+        wait = GET_REMOTE_WAIT;
+
+        msg.cmd = BAR_READ;
+        msg.num_fds = 1;
+        msg.fds[0] = wait;
+    }
+
+    mpqemu_msg_send(&msg, mpqemu_link->dev);
+
+    if (!write) {
+        *val = wait_for_remote(wait);
+        PUT_REMOTE_WAIT(wait);
+    }
+}
+
+void proxy_default_bar_write(void *opaque, hwaddr addr, uint64_t val,
+                             unsigned size)
+{
+    ProxyMemoryRegion *pmr = opaque;
+
+    send_bar_access_msg(pmr->dev, &pmr->mr, true, addr, &val, size,
+                        pmr->memory);
+}
+
+uint64_t proxy_default_bar_read(void *opaque, hwaddr addr, unsigned size)
+{
+    ProxyMemoryRegion *pmr = opaque;
+    uint64_t val;
+
+    send_bar_access_msg(pmr->dev, &pmr->mr, false, addr, &val, size,
+                        pmr->memory);
+
+     return val;
+}
+
+const MemoryRegionOps proxy_default_ops = {
+    .read = proxy_default_bar_read,
+    .write = proxy_default_bar_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
diff --git a/include/hw/proxy/qemu-proxy.h b/include/hw/proxy/qemu-proxy.h
index d7eaf26f29..9e4127eccb 100644
--- a/include/hw/proxy/qemu-proxy.h
+++ b/include/hw/proxy/qemu-proxy.h
@@ -26,14 +26,25 @@ 
 #define PCI_PROXY_DEV_GET_CLASS(obj) \
             OBJECT_GET_CLASS(PCIProxyDevClass, (obj), TYPE_PCI_PROXY_DEV)
 
-typedef struct PCIProxyDev {
+typedef struct PCIProxyDev PCIProxyDev;
+
+typedef struct ProxyMemoryRegion {
+    PCIProxyDev *dev;
+    MemoryRegion mr;
+    bool memory;
+    bool present;
+    uint8_t type;
+} ProxyMemoryRegion;
+
+struct PCIProxyDev {
     PCIDevice parent_dev;
 
     MPQemuLinkState *mpqemu_link;
 
     int socket;
 
-} PCIProxyDev;
+    ProxyMemoryRegion region[PCI_NUM_REGIONS];
+};
 
 typedef struct PCIProxyDevClass {
     PCIDeviceClass parent_class;
@@ -43,4 +54,9 @@  typedef struct PCIProxyDevClass {
     char *command;
 } PCIProxyDevClass;
 
+void proxy_default_bar_write(void *opaque, hwaddr addr, uint64_t val,
+                             unsigned size);
+
+uint64_t proxy_default_bar_read(void *opaque, hwaddr addr, unsigned size);
+
 #endif /* QEMU_PROXY_H */
diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
index 7228a1915e..41cf092f9e 100644
--- a/include/io/mpqemu-link.h
+++ b/include/io/mpqemu-link.h
@@ -31,6 +31,8 @@ 
 /**
  * mpqemu_cmd_t:
  * SYNC_SYSMEM      Shares QEMU's RAM with remote device's RAM
+ * BAR_WRITE        Writes to PCI BAR region
+ * BAR_READ         Reads from PCI BAR region
  *
  * proc_cmd_t enum type to specify the command to be executed on the remote
  * device.
@@ -41,6 +43,8 @@  typedef enum {
     CONNECT_DEV,
     PCI_CONFIG_WRITE,
     PCI_CONFIG_READ,
+    BAR_WRITE,
+    BAR_READ,
     MAX,
 } mpqemu_cmd_t;
 
@@ -56,6 +60,13 @@  typedef struct {
     ram_addr_t offsets[REMOTE_MAX_FDS];
 } sync_sysmem_msg_t;
 
+typedef struct {
+    hwaddr addr;
+    uint64_t val;
+    unsigned size;
+    bool memory;
+} bar_access_msg_t;
+
 /**
  * MPQemuMsg:
  * @cmd: The remote command
@@ -78,6 +89,7 @@  typedef struct {
     union {
         uint64_t u64;
         sync_sysmem_msg_t sync_sysmem;
+        bar_access_msg_t bar_access;
     } data1;
 
     int fds[REMOTE_MAX_FDS];
diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
index ef4a07b81a..5cb93fc47b 100644
--- a/io/mpqemu-link.c
+++ b/io/mpqemu-link.c
@@ -387,6 +387,12 @@  bool mpqemu_msg_valid(MPQemuMsg *msg)
             return false;
         }
         break;
+    case BAR_WRITE:
+    case BAR_READ:
+        if (msg->size != sizeof(msg->data1)) {
+            return false;
+        }
+        break;
     default:
         break;
     }
diff --git a/remote/remote-main.c b/remote/remote-main.c
index 834574e172..0990509f7a 100644
--- a/remote/remote-main.c
+++ b/remote/remote-main.c
@@ -34,6 +34,7 @@ 
 #include "block/block.h"
 #include "exec/ramlist.h"
 #include "remote/remote-common.h"
+#include "exec/memattrs.h"
 
 static void process_msg(GIOCondition cond, MPQemuLinkState *link,
                         MPQemuChannel *chan);
@@ -114,6 +115,77 @@  exit:
     notify_proxy(wait, ret);
 }
 
+/* TODO: confirm memtx attrs. */
+static void process_bar_write(MPQemuMsg *msg, Error **errp)
+{
+    bar_access_msg_t *bar_access = &msg->data1.bar_access;
+    AddressSpace *as =
+        bar_access->memory ? &address_space_memory : &address_space_io;
+    MemTxResult res;
+
+    assert(is_power_of_2(bar_access->size) &&
+           bar_access->size <= sizeof(uint64_t));
+
+    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
+                           (uint8_t *)&bar_access->val, bar_access->size,
+                           true);
+
+    if (res != MEMTX_OK) {
+        error_setg(errp, "Could not perform address space write operation,"
+                   " inaccessible address: %lx in pid %d, %s.",
+                   bar_access->addr, getpid(), __progname);
+    }
+}
+
+static void process_bar_read(MPQemuMsg *msg, Error **errp)
+{
+    bar_access_msg_t *bar_access = &msg->data1.bar_access;
+    AddressSpace *as;
+    int wait = msg->fds[0];
+    MemTxResult res;
+    uint64_t val = 0;
+
+    as = bar_access->memory ? &address_space_memory : &address_space_io;
+
+    assert(is_power_of_2(bar_access->size) &&
+           bar_access->size <= sizeof(uint64_t));
+
+    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
+                           (uint8_t *)&val, bar_access->size, false);
+
+    if (res != MEMTX_OK) {
+        error_setg(errp, "Could not perform address space read operation,"
+                   " inaccessible address: %lx in pid %d, %s.",
+                   bar_access->addr, getpid(), __progname);
+        val = (uint64_t)-1;
+        goto fail;
+    }
+
+    switch (bar_access->size) {
+    case 8:
+        /* Nothing to do as val is already 8 bytes long */
+        break;
+    case 4:
+        val = *((uint32_t *)&val);
+        break;
+    case 2:
+        val = *((uint16_t *)&val);
+        break;
+    case 1:
+        val = *((uint8_t *)&val);
+        break;
+    default:
+        error_setg(errp, "Invalid PCI BAR read size in pid %d, %s",
+                   getpid(), __progname);
+        return;
+    }
+
+fail:
+    notify_proxy(wait, val);
+
+    PUT_REMOTE_WAIT(wait);
+}
+
 static void process_msg(GIOCondition cond, MPQemuLinkState *link,
                         MPQemuChannel *chan)
 {
@@ -147,6 +219,18 @@  static void process_msg(GIOCondition cond, MPQemuLinkState *link,
     case PCI_CONFIG_READ:
         process_config_read(LINK_TO_DEV(link), msg);
         break;
+    case BAR_WRITE:
+        process_bar_write(msg, &err);
+        if (err) {
+            goto finalize_loop;
+        }
+        break;
+    case BAR_READ:
+        process_bar_read(msg, &err);
+        if (err) {
+            goto finalize_loop;
+        }
+        break;
     default:
         error_setg(&err, "Unknown command in %s", print_pid_exec(pid_exec));
         goto finalize_loop;