[RESEND,v6,20/36] multi-process: Forward PCI config space acceses to the remote process

Commit Message

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

From: Elena Ufimtseva <[e-mail]>

The Proxy Object sends the PCI config space accesses as messages
to the remote process over the communication channel

Signed-off-by: Elena Ufimtseva <[e-mail]>
Signed-off-by: Jagannathan Raman <[e-mail]>
Signed-off-by: John G Johnson <[e-mail]>
---
 hw/proxy/qemu-proxy.c    | 61 ++++++++++++++++++++++++++++++++++++++++
 include/io/mpqemu-link.h |  8 ++++++
 io/mpqemu-link.c         |  6 ++++
 remote/remote-main.c     | 32 +++++++++++++++++++++
 4 files changed, 107 insertions(+)

Comments

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

On Wed, Apr 22, 2020 at 09:13:55PM -0700, [e-mail] wrote:
> +static int config_op_send(PCIProxyDev *dev, uint32_t addr, uint32_t *val, int l,
> +                          unsigned int op)
> +{
> +    MPQemuMsg msg;
> +    struct conf_data_msg conf_data;
> +    int wait;
> +
> +    memset(&msg, 0, sizeof(MPQemuMsg));
> +    conf_data.addr = addr;
> +    conf_data.val = (op == PCI_CONFIG_WRITE) ? *val : 0;
> +    conf_data.l = l;
> +
> +    msg.data2 = (uint8_t *)&conf_data;
> +    if (!msg.data2) {
> +        return -ENOMEM;

This can never happen since conf_data is on the stack.

> +    }
> +
> +    msg.size = sizeof(conf_data);
> +    msg.cmd = op;
> +    msg.bytestream = 1;
> +
> +    if (op == PCI_CONFIG_WRITE) {
> +        msg.num_fds = 0;
> +    } else {
> +        /* TODO: Dont create fd each time for send. */
> +        wait = GET_REMOTE_WAIT;
> +        msg.num_fds = 1;
> +        msg.fds[0] = wait;
> +    }
> +
> +    mpqemu_msg_send(&msg, dev->mpqemu_link->dev);
> +
> +    if (op == PCI_CONFIG_READ) {

Are you sure it's correct for writes to be posted instead of waiting for
completion?

> +        *val = (uint32_t)wait_for_remote(wait);
> +        PUT_REMOTE_WAIT(wait);
> +    }
> +
> +    return 0;
> +}
> +
> +static uint32_t pci_proxy_read_config(PCIDevice *d, uint32_t addr, int len)
> +{
> +    uint32_t val;
> +
> +    (void)pci_default_read_config(d, addr, len);

Please add a comment explaining why this local read is necessary.

> +
> +    config_op_send(PCI_PROXY_DEV(d), addr, &val, len, PCI_CONFIG_READ);
> +
> +    return val;
> +}
> +
> +static void pci_proxy_write_config(PCIDevice *d, uint32_t addr, uint32_t val,
> +                                   int l)
> +{
> +    pci_default_write_config(d, addr, val, l);

Please add a comment explaining why this local write is necessary.

> +
> +    config_op_send(PCI_PROXY_DEV(d), addr, &val, l, PCI_CONFIG_WRITE);
> +}
...
> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> index f780b65181..ef4a07b81a 100644
> --- a/io/mpqemu-link.c
> +++ b/io/mpqemu-link.c
> @@ -381,6 +381,12 @@ bool mpqemu_msg_valid(MPQemuMsg *msg)
>              return false;
>          }
>          break;
> +    case PCI_CONFIG_WRITE:
> +    case PCI_CONFIG_READ:
> +        if (msg->size != sizeof(struct conf_data_msg)) {
> +            return false;
> +        }

conf_data_msg.l is not validated.

> +        break;
>      default:
>          break;
>      }
> diff --git a/remote/remote-main.c b/remote/remote-main.c
> index f541baae6a..834574e172 100644
> --- a/remote/remote-main.c
> +++ b/remote/remote-main.c
> @@ -53,6 +53,32 @@ gchar *print_pid_exec(gchar *str)
>  
>  #define LINK_TO_DEV(link) ((PCIDevice *)link->opaque)
>  
> +static void process_config_write(PCIDevice *dev, MPQemuMsg *msg)
> +{
> +    struct conf_data_msg *conf = (struct conf_data_msg *)msg->data2;
> +
> +    qemu_mutex_lock_iothread();

The qemu_mutex_lock_iothread() can be dropped once this is integrated in
to QEMU's event loop.

> +    pci_default_write_config(dev, conf->addr, conf->val, conf->l);

It is not safe to call this function with arbitrary addr input values.

> +    qemu_mutex_unlock_iothread();
> +}
> +
> +static void process_config_read(PCIDevice *dev, MPQemuMsg *msg)
> +{
> +    struct conf_data_msg *conf = (struct conf_data_msg *)msg->data2;
> +    uint32_t val;
> +    int wait;
> +
> +    wait = msg->fds[0];
> +
> +    qemu_mutex_lock_iothread();
> +    val = pci_default_read_config(dev, conf->addr, conf->l);

It is not safe to call this function with arbitrary addr input values.

Patch

diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c
index 9b5e429a88..87cf39c672 100644
--- a/hw/proxy/qemu-proxy.c
+++ b/hw/proxy/qemu-proxy.c
@@ -14,6 +14,65 @@ 
 #include "hw/proxy/qemu-proxy.h"
 #include "hw/pci/pci.h"
 
+static int config_op_send(PCIProxyDev *dev, uint32_t addr, uint32_t *val, int l,
+                          unsigned int op)
+{
+    MPQemuMsg msg;
+    struct conf_data_msg conf_data;
+    int wait;
+
+    memset(&msg, 0, sizeof(MPQemuMsg));
+    conf_data.addr = addr;
+    conf_data.val = (op == PCI_CONFIG_WRITE) ? *val : 0;
+    conf_data.l = l;
+
+    msg.data2 = (uint8_t *)&conf_data;
+    if (!msg.data2) {
+        return -ENOMEM;
+    }
+
+    msg.size = sizeof(conf_data);
+    msg.cmd = op;
+    msg.bytestream = 1;
+
+    if (op == PCI_CONFIG_WRITE) {
+        msg.num_fds = 0;
+    } else {
+        /* TODO: Dont create fd each time for send. */
+        wait = GET_REMOTE_WAIT;
+        msg.num_fds = 1;
+        msg.fds[0] = wait;
+    }
+
+    mpqemu_msg_send(&msg, dev->mpqemu_link->dev);
+
+    if (op == PCI_CONFIG_READ) {
+        *val = (uint32_t)wait_for_remote(wait);
+        PUT_REMOTE_WAIT(wait);
+    }
+
+    return 0;
+}
+
+static uint32_t pci_proxy_read_config(PCIDevice *d, uint32_t addr, int len)
+{
+    uint32_t val;
+
+    (void)pci_default_read_config(d, addr, len);
+
+    config_op_send(PCI_PROXY_DEV(d), addr, &val, len, PCI_CONFIG_READ);
+
+    return val;
+}
+
+static void pci_proxy_write_config(PCIDevice *d, uint32_t addr, uint32_t val,
+                                   int l)
+{
+    pci_default_write_config(d, addr, val, l);
+
+    config_op_send(PCI_PROXY_DEV(d), addr, &val, l, PCI_CONFIG_WRITE);
+}
+
 static void proxy_set_socket(Object *obj, const char *str, Error **errp)
 {
     PCIProxyDev *pdev = PCI_PROXY_DEV(obj);
@@ -86,6 +145,8 @@  static void pci_proxy_dev_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->realize = pci_proxy_dev_realize;
+    k->config_read = pci_proxy_read_config;
+    k->config_write = pci_proxy_write_config;
 }
 
 static const TypeInfo pci_proxy_dev_type_info = {
diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
index ebae9afc45..7228a1915e 100644
--- a/include/io/mpqemu-link.h
+++ b/include/io/mpqemu-link.h
@@ -39,9 +39,17 @@  typedef enum {
     INIT = 0,
     SYNC_SYSMEM,
     CONNECT_DEV,
+    PCI_CONFIG_WRITE,
+    PCI_CONFIG_READ,
     MAX,
 } mpqemu_cmd_t;
 
+struct conf_data_msg {
+    uint32_t addr;
+    uint32_t val;
+    int l;
+};
+
 typedef struct {
     hwaddr gpas[REMOTE_MAX_FDS];
     uint64_t sizes[REMOTE_MAX_FDS];
diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
index f780b65181..ef4a07b81a 100644
--- a/io/mpqemu-link.c
+++ b/io/mpqemu-link.c
@@ -381,6 +381,12 @@  bool mpqemu_msg_valid(MPQemuMsg *msg)
             return false;
         }
         break;
+    case PCI_CONFIG_WRITE:
+    case PCI_CONFIG_READ:
+        if (msg->size != sizeof(struct conf_data_msg)) {
+            return false;
+        }
+        break;
     default:
         break;
     }
diff --git a/remote/remote-main.c b/remote/remote-main.c
index f541baae6a..834574e172 100644
--- a/remote/remote-main.c
+++ b/remote/remote-main.c
@@ -53,6 +53,32 @@  gchar *print_pid_exec(gchar *str)
 
 #define LINK_TO_DEV(link) ((PCIDevice *)link->opaque)
 
+static void process_config_write(PCIDevice *dev, MPQemuMsg *msg)
+{
+    struct conf_data_msg *conf = (struct conf_data_msg *)msg->data2;
+
+    qemu_mutex_lock_iothread();
+    pci_default_write_config(dev, conf->addr, conf->val, conf->l);
+    qemu_mutex_unlock_iothread();
+}
+
+static void process_config_read(PCIDevice *dev, MPQemuMsg *msg)
+{
+    struct conf_data_msg *conf = (struct conf_data_msg *)msg->data2;
+    uint32_t val;
+    int wait;
+
+    wait = msg->fds[0];
+
+    qemu_mutex_lock_iothread();
+    val = pci_default_read_config(dev, conf->addr, conf->l);
+    qemu_mutex_unlock_iothread();
+
+    notify_proxy(wait, val);
+
+    PUT_REMOTE_WAIT(wait);
+}
+
 static gpointer dev_thread(gpointer data)
 {
     MPQemuLinkState *link = data;
@@ -115,6 +141,12 @@  static void process_msg(GIOCondition cond, MPQemuLinkState *link,
     case CONNECT_DEV:
         process_connect_dev_msg(msg);
         break;
+    case PCI_CONFIG_WRITE:
+        process_config_write(LINK_TO_DEV(link), msg);
+        break;
+    case PCI_CONFIG_READ:
+        process_config_read(LINK_TO_DEV(link), msg);
+        break;
     default:
         error_setg(&err, "Unknown command in %s", print_pid_exec(pid_exec));
         goto finalize_loop;