qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5 15/50] multi-process: PCI BAR read/write handling for prox


From: Jag Raman
Subject: Re: [PATCH v5 15/50] multi-process: PCI BAR read/write handling for proxy & remote endpoints
Date: Wed, 4 Mar 2020 14:05:14 -0500
User-agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0



On 3/4/2020 5:47 AM, Dr. David Alan Gilbert wrote:
* Jagannathan Raman (address@hidden) wrote:
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 <address@hidden>
Signed-off-by: Elena Ufimtseva <address@hidden>
Signed-off-by: John G Johnson <address@hidden>
---
  hw/proxy/qemu-proxy.c         | 65 ++++++++++++++++++++++++++++++++++++++
  include/hw/proxy/qemu-proxy.h | 22 +++++++++++--
  include/io/mpqemu-link.h      | 12 +++++++
  remote/remote-main.c          | 73 +++++++++++++++++++++++++++++++++++++++++++
  4 files changed, 170 insertions(+), 2 deletions(-)

diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c
index d792e86..b17d9bb 100644
--- a/hw/proxy/qemu-proxy.c
+++ b/hw/proxy/qemu-proxy.c
@@ -262,3 +262,68 @@ static void pci_proxy_dev_realize(PCIDevice *device, Error 
**errp)
      dev->get_proxy_sock = get_proxy_sock;
      dev->init_proxy = init_proxy;
  }
+
+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->com);
+
+    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 29fa2e9..44e370e 100644
--- a/include/hw/proxy/qemu-proxy.h
+++ b/include/hw/proxy/qemu-proxy.h
@@ -22,7 +22,19 @@
  #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;
+
+extern const MemoryRegionOps proxy_default_ops;
+
+struct PCIProxyDev {
      PCIDevice parent_dev;
MPQemuLinkState *mpqemu_link;
@@ -41,7 +53,8 @@ typedef struct PCIProxyDev {
      void (*init_proxy) (PCIDevice *dev, char *command, char *exec_name,
                          bool need_spawn, Error **errp);
-} PCIProxyDev;
+    ProxyMemoryRegion region[PCI_NUM_REGIONS];
+};
typedef struct PCIProxyDevClass {
      PCIDeviceClass parent_class;
@@ -51,4 +64,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 5a2be48..1a7738e 100644
--- a/include/io/mpqemu-link.h
+++ b/include/io/mpqemu-link.h
@@ -38,6 +38,8 @@
   * PCI_CONFIG_READ        PCI configuration space read
   * PCI_CONFIG_WRITE       PCI configuration space write
   * 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.
@@ -47,6 +49,8 @@ typedef enum {
      PCI_CONFIG_READ,
      PCI_CONFIG_WRITE,
      SYNC_SYSMEM,
+    BAR_WRITE,
+    BAR_READ,
      MAX,
  } mpqemu_cmd_t;
@@ -70,6 +74,13 @@ typedef struct {
  } sync_sysmem_msg_t;
typedef struct {
+    hwaddr addr;
+    uint64_t val;
+    unsigned size;
+    bool memory;
+} bar_access_msg_t;
+
+typedef struct {
      mpqemu_cmd_t cmd;
      int bytestream;
      size_t size;
@@ -77,6 +88,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/remote/remote-main.c b/remote/remote-main.c
index 7b4cf2f..acd8daf 100644
--- a/remote/remote-main.c
+++ b/remote/remote-main.c
@@ -33,6 +33,7 @@
  #include "sysemu/sysemu.h"
  #include "block/block.h"
  #include "exec/ramlist.h"
+#include "exec/memattrs.h"
static MPQemuLinkState *mpqemu_link;
  PCIDevice *remote_pci_dev;
@@ -63,6 +64,66 @@ static void process_config_read(MPQemuMsg *msg)
      PUT_REMOTE_WAIT(wait);
  }
+/* 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;
+
+    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.", bar_access->addr);
+    }
+}
+
+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(bar_access->size <= sizeof(uint64_t));

Note you don't have that check on the write function above.
Do you actually want something like:
    assert(is_power_of_2(bar_access->size) && bar_access->size <= 
sizeof(uint64_t));

Thanks, Dave! I think we should add this check to the write function as
well.


+    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.", bar_access->addr);
+        val = (uint64_t)-1;
+        goto fail;
+    }
+
+    switch (bar_access->size) {

No case 8 ?

You're correct; we need case 8 in this case.

We don't need to do an explicit typecast for case 8, as the variable
"val" is already 8 bytes long. However, if we don't have case 8, then
all 8-byte reads would get picked up as "default" and report an error.

We initially didn't have the "default" case. But checkpatch complained
about switch-case without default, and this bug got introduced
unwittingly.

Thank you!
--
Jag


Dave

+    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");
+        return;
+    }
+
+fail:
+    notify_proxy(wait, val);
+
+    PUT_REMOTE_WAIT(wait);
+}
+
  static void process_msg(GIOCondition cond, MPQemuChannel *chan)
  {
      MPQemuMsg *msg = NULL;
@@ -88,6 +149,18 @@ static void process_msg(GIOCondition cond, MPQemuChannel 
*chan)
      case PCI_CONFIG_READ:
          process_config_read(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");
          goto finalize_loop;
--
1.8.3.1

--
Dr. David Alan Gilbert / address@hidden / Manchester, UK




reply via email to

[Prev in Thread] Current Thread [Next in Thread]