diff mbox series

[v8,13/20] multi-process: PCI BAR read/write handling for proxy & remote endpoints

Message ID 3588624b278c97cb3c9d1eeda109ad36af39effc.1596217462.git.jag.raman@oracle.com (mailing list archive)
State New, archived
Headers show
Series Initial support for multi-process qemu | expand

Commit Message

Jag Raman July 31, 2020, 6:20 p.m. UTC 
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 <jag.raman@oracle.com>
Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
---
 hw/i386/remote-msg.c     | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
 hw/pci/proxy.c           | 61 +++++++++++++++++++++++++++++++++
 include/hw/pci/proxy.h   | 16 +++++++--
 include/io/mpqemu-link.h | 10 ++++++
 io/mpqemu-link.c         |  6 ++++
 5 files changed, 179 insertions(+), 2 deletions(-)

Comments

Stefan Hajnoczi Aug. 11, 2020, 2:04 p.m. UTC | #1 
On Fri, Jul 31, 2020 at 02:20:20PM -0400, Jagannathan Raman wrote:
> +static void process_bar_write(QIOChannel *ioc, MPQemuMsg *msg, Error **errp)
> +{
> +    BarAccessMsg *bar_access = &msg->data1.bar_access;
> +    AddressSpace *as =
> +        bar_access->memory ? &address_space_memory : &address_space_io;
> +    MPQemuMsg ret = { 0 };
> +    MemTxResult res;
> +    uint64_t val;
> +    Error *local_err = NULL;
> +
> +    if (!is_power_of_2(bar_access->size) ||
> +       (bar_access->size > sizeof(uint64_t))) {
> +        ret.data1.u64 = UINT64_MAX;
> +        goto fail;
> +    }
> +
> +    val = cpu_to_le64(bar_access->val);
> +
> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
> +                           (void *)&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.",
> +                   bar_access->addr, getpid());
> +        ret.data1.u64 = -1;
> +    }
> +
> +fail:
> +    ret.cmd = RET_MSG;
> +    ret.size = sizeof(ret.data1);
> +
> +    mpqemu_msg_send(&ret, ioc, &local_err);
> +    if (local_err) {
> +        error_setg(errp, "Error while sending message to proxy "
> +                   "in remote process pid=%d", getpid());

There is an assertion failure if res != MEMTX_OK because errp was
already set. error_setg() must not be called on an Error pointer that
has already been set.

It is simplest to do:

  mpqemu_msg_send(&ret, ioc, (errp && *errp) ? NULL : &local_err);

> +    }
> +}
> +
> +static void process_bar_read(QIOChannel *ioc, MPQemuMsg *msg, Error **errp)
> +{
> +    BarAccessMsg *bar_access = &msg->data1.bar_access;
> +    MPQemuMsg ret = { 0 };
> +    AddressSpace *as;
> +    MemTxResult res;
> +    uint64_t val = 0;
> +    Error *local_err = NULL;
> +
> +    as = bar_access->memory ? &address_space_memory : &address_space_io;
> +
> +    if (!is_power_of_2(bar_access->size) ||
> +       (bar_access->size > sizeof(uint64_t))) {
> +        val = UINT64_MAX;
> +        goto fail;
> +    }
> +
> +    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
> +                           (void *)&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.",
> +                   bar_access->addr, getpid());
> +        val = UINT64_MAX;
> +        goto fail;
> +    }
> +
> +fail:
> +    ret.cmd = RET_MSG;
> +    ret.data1.u64 = le64_to_cpu(val);
> +    ret.size = sizeof(ret.data1);
> +
> +    mpqemu_msg_send(&ret, ioc, &local_err);
> +    if (local_err) {
> +        error_setg(errp, "Error while sending message to proxy "

Same here.

> +static void send_bar_access_msg(PCIProxyDev *pdev, MemoryRegion *mr,
> +                                bool write, hwaddr addr, uint64_t *val,
> +                                unsigned size, bool memory)
> +{
> +    MPQemuMsg msg = { 0 };
> +    long ret = -EINVAL;

long is not guaranteed to be 64-bit. This function supports 64-bit
accesses to BARs so uint64_t is needed here.

> +    Error *local_err = NULL;
> +
> +    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 {
> +        msg.cmd = BAR_READ;
> +    }
> +
> +    ret = mpqemu_msg_send_and_await_reply(&msg, pdev->ioc, &local_err);
> +    if (local_err) {
> +        error_report("Failed to send BAR command to the remote process.");

Leaks local_err. Please report the error message from local_err and then
free it.

> +const MemoryRegionOps proxy_mr_ops = {
> +    .read = proxy_bar_read,
> +    .write = proxy_bar_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,

Should this be .max_access_size = 8?

> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> index 5d04b81..82b8465 100644
> --- a/io/mpqemu-link.c
> +++ b/io/mpqemu-link.c
> @@ -269,6 +269,12 @@ bool mpqemu_msg_valid(MPQemuMsg *msg)
>              return false;
>          }
>          break;
> +    case BAR_WRITE:
> +    case BAR_READ:
> +        if ((msg->size != sizeof(msg->data1)) || (msg->num_fds != 0)) {

What about bytestream? It would be cleanest not to send bytestream over
the wire. Since it is sent today the receiver can be confused if it has
the wrong value and some mpqemu_msg_valid() cases validate bytestream.
It is not validate here for some reason.
diff mbox series

Patch

diff --git a/hw/i386/remote-msg.c b/hw/i386/remote-msg.c
index c5b8651..7ccdd63 100644
--- a/hw/i386/remote-msg.c
+++ b/hw/i386/remote-msg.c
@@ -16,11 +16,14 @@ 
 #include "qapi/error.h"
 #include "sysemu/runstate.h"
 #include "hw/pci/pci.h"
+#include "exec/memattrs.h"
 
 static void process_config_write(QIOChannel *ioc, PCIDevice *dev,
                                  MPQemuMsg *msg);
 static void process_config_read(QIOChannel *ioc, PCIDevice *dev,
                                 MPQemuMsg *msg);
+static void process_bar_write(QIOChannel *ioc, MPQemuMsg *msg, Error **errp);
+static void process_bar_read(QIOChannel *ioc, MPQemuMsg *msg, Error **errp);
 
 gboolean mpqemu_process_msg(QIOChannel *ioc, GIOCondition cond,
                             gpointer opaque)
@@ -58,6 +61,12 @@  gboolean mpqemu_process_msg(QIOChannel *ioc, GIOCondition cond,
     case PCI_CONFIG_READ:
         process_config_read(ioc, pci_dev, &msg);
         break;
+    case BAR_WRITE:
+        process_bar_write(ioc, &msg, &local_err);
+        break;
+    case BAR_READ:
+        process_bar_read(ioc, &msg, &local_err);
+        break;
     default:
         error_setg(&local_err,
                    "Unknown command (%d) received for device %s (pid=%d)",
@@ -127,3 +136,82 @@  static void process_config_read(QIOChannel *ioc, PCIDevice *dev,
     }
 
 }
+
+static void process_bar_write(QIOChannel *ioc, MPQemuMsg *msg, Error **errp)
+{
+    BarAccessMsg *bar_access = &msg->data1.bar_access;
+    AddressSpace *as =
+        bar_access->memory ? &address_space_memory : &address_space_io;
+    MPQemuMsg ret = { 0 };
+    MemTxResult res;
+    uint64_t val;
+    Error *local_err = NULL;
+
+    if (!is_power_of_2(bar_access->size) ||
+       (bar_access->size > sizeof(uint64_t))) {
+        ret.data1.u64 = UINT64_MAX;
+        goto fail;
+    }
+
+    val = cpu_to_le64(bar_access->val);
+
+    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
+                           (void *)&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.",
+                   bar_access->addr, getpid());
+        ret.data1.u64 = -1;
+    }
+
+fail:
+    ret.cmd = RET_MSG;
+    ret.size = sizeof(ret.data1);
+
+    mpqemu_msg_send(&ret, ioc, &local_err);
+    if (local_err) {
+        error_setg(errp, "Error while sending message to proxy "
+                   "in remote process pid=%d", getpid());
+    }
+}
+
+static void process_bar_read(QIOChannel *ioc, MPQemuMsg *msg, Error **errp)
+{
+    BarAccessMsg *bar_access = &msg->data1.bar_access;
+    MPQemuMsg ret = { 0 };
+    AddressSpace *as;
+    MemTxResult res;
+    uint64_t val = 0;
+    Error *local_err = NULL;
+
+    as = bar_access->memory ? &address_space_memory : &address_space_io;
+
+    if (!is_power_of_2(bar_access->size) ||
+       (bar_access->size > sizeof(uint64_t))) {
+        val = UINT64_MAX;
+        goto fail;
+    }
+
+    res = address_space_rw(as, bar_access->addr, MEMTXATTRS_UNSPECIFIED,
+                           (void *)&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.",
+                   bar_access->addr, getpid());
+        val = UINT64_MAX;
+        goto fail;
+    }
+
+fail:
+    ret.cmd = RET_MSG;
+    ret.data1.u64 = le64_to_cpu(val);
+    ret.size = sizeof(ret.data1);
+
+    mpqemu_msg_send(&ret, ioc, &local_err);
+    if (local_err) {
+        error_setg(errp, "Error while sending message to proxy "
+                   "in remote process pid=%d", getpid());
+    }
+}
diff --git a/hw/pci/proxy.c b/hw/pci/proxy.c
index 945cc35..179f0c7 100644
--- a/hw/pci/proxy.c
+++ b/hw/pci/proxy.c
@@ -133,3 +133,64 @@  static void pci_proxy_dev_register_types(void)
 }
 
 type_init(pci_proxy_dev_register_types)
+
+static void send_bar_access_msg(PCIProxyDev *pdev, MemoryRegion *mr,
+                                bool write, hwaddr addr, uint64_t *val,
+                                unsigned size, bool memory)
+{
+    MPQemuMsg msg = { 0 };
+    long ret = -EINVAL;
+    Error *local_err = NULL;
+
+    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 {
+        msg.cmd = BAR_READ;
+    }
+
+    ret = mpqemu_msg_send_and_await_reply(&msg, pdev->ioc, &local_err);
+    if (local_err) {
+        error_report("Failed to send BAR command to the remote process.");
+    }
+
+    if (!write) {
+        *val = ret;
+    }
+}
+
+static void proxy_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);
+}
+
+static uint64_t proxy_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_mr_ops = {
+    .read = proxy_bar_read,
+    .write = proxy_bar_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
diff --git a/include/hw/pci/proxy.h b/include/hw/pci/proxy.h
index 0a66ddc..9d57483 100644
--- a/include/hw/pci/proxy.h
+++ b/include/hw/pci/proxy.h
@@ -17,10 +17,22 @@ 
 #define PCI_PROXY_DEV(obj) \
             OBJECT_CHECK(PCIProxyDev, (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;
     char *fd;
     QIOChannel *ioc;
-} PCIProxyDev;
+
+    ProxyMemoryRegion region[PCI_NUM_REGIONS];
+};
 
 #endif /* PROXY_H */
diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
index 9bd754e..ee3b44f 100644
--- a/include/io/mpqemu-link.h
+++ b/include/io/mpqemu-link.h
@@ -36,6 +36,8 @@  typedef enum {
     RET_MSG,
     PCI_CONFIG_WRITE,
     PCI_CONFIG_READ,
+    BAR_WRITE,
+    BAR_READ,
     MAX = INT_MAX,
 } MPQemuCmd;
 
@@ -51,6 +53,13 @@  typedef struct {
     int l;
 } ConfDataMsg;
 
+typedef struct {
+    hwaddr addr;
+    uint64_t val;
+    unsigned size;
+    bool memory;
+} BarAccessMsg;
+
 /**
  * Maximum size of data2 field in the message to be transmitted.
  */
@@ -78,6 +87,7 @@  typedef struct {
     union {
         uint64_t u64;
         SyncSysmemMsg sync_sysmem;
+        BarAccessMsg bar_access;
     } data1;
 
     int fds[REMOTE_MAX_FDS];
diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
index 5d04b81..82b8465 100644
--- a/io/mpqemu-link.c
+++ b/io/mpqemu-link.c
@@ -269,6 +269,12 @@  bool mpqemu_msg_valid(MPQemuMsg *msg)
             return false;
         }
         break;
+    case BAR_WRITE:
+    case BAR_READ:
+        if ((msg->size != sizeof(msg->data1)) || (msg->num_fds != 0)) {
+            return false;
+        }
+        break;
     default:
         break;
     }