Re: [PATCH RESEND v6 19/36] multi-process: Connect Proxy Object with device in the remote process

[Stefan Hajnoczi]

On Wed, Apr 22, 2020 at 09:13:54PM -0700, address@hidden wrote:
> From: Jagannathan Raman <address@hidden>
> 
> Send a message to the remote process to connect PCI device with the
> corresponding Proxy object in QEMU

The CONNECT_DEV message is no longer necessary with a 1 socket per
device architecture. Connecting to a specific UNIX domain socket (e.g.
vm001/lsi-scsi-1.sock) already identifies which device the proxy wishes
to talk to.

Each device should have an mpqemu link that accepts client connections.
You can either do that in the main loop or you can use IOThread to run
dedicated per-device threads.

> 
> Signed-off-by: Elena Ufimtseva <address@hidden>
> Signed-off-by: John G Johnson <address@hidden>
> Signed-off-by: Jagannathan Raman <address@hidden>
> ---
>  hw/proxy/qemu-proxy.c    | 34 +++++++++++++++++++++++++++++++
>  include/io/mpqemu-link.h |  5 +++++
>  io/mpqemu-link.c         |  3 +++
>  remote/remote-main.c     | 43 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 85 insertions(+)
> 
> diff --git a/hw/proxy/qemu-proxy.c b/hw/proxy/qemu-proxy.c
> index 40bf56fd37..9b5e429a88 100644
> --- a/hw/proxy/qemu-proxy.c
> +++ b/hw/proxy/qemu-proxy.c
> @@ -17,11 +17,45 @@
>  static void proxy_set_socket(Object *obj, const char *str, Error **errp)
>  {
>      PCIProxyDev *pdev = PCI_PROXY_DEV(obj);
> +    DeviceState *dev = DEVICE(obj);
> +    MPQemuMsg msg = { 0 };
> +    int wait, fd[2];
>  
>      pdev->socket = atoi(str);
>  
>      mpqemu_init_channel(pdev->mpqemu_link, &pdev->mpqemu_link->com,
>                          pdev->socket);
> +
> +    if (socketpair(AF_UNIX, SOCK_STREAM, 0, fd)) {
> +        error_setg(errp, "Failed to create socket for device channel");
> +        return;
> +    }

This extra connection can be removed. The reasons for having it have
gone away now that there is just 1 device per socket.

> +
> +    wait = GET_REMOTE_WAIT;
> +
> +    msg.cmd = CONNECT_DEV;
> +    msg.bytestream = 1;
> +    msg.data2 = (uint8_t *)g_strdup(dev->id);
> +    msg.size = sizeof(msg.data2);

The g_strdup() is unnecessary, dev->id can be used directly.

Should msg.size be strlen(dev->id) instead of sizeof(msg.data2)?

> +    msg.num_fds = 2;
> +    msg.fds[0] = wait;
> +    msg.fds[1] = fd[1];
> +
> +    mpqemu_msg_send(&msg, pdev->mpqemu_link->com);
> +
> +    if (wait_for_remote(wait)) {
> +        error_setg(errp, "Failed to connect device to the remote");
> +        close(fd[0]);
> +    } else {
> +        mpqemu_init_channel(pdev->mpqemu_link, &pdev->mpqemu_link->dev,
> +                            fd[0]);
> +    }
> +
> +    PUT_REMOTE_WAIT(wait);
> +
> +    close(fd[1]);
> +
> +    g_free(msg.data2);
>  }
>  
>  static void proxy_init(Object *obj)
> diff --git a/include/io/mpqemu-link.h b/include/io/mpqemu-link.h
> index 73cc59b874..ebae9afc45 100644
> --- a/include/io/mpqemu-link.h
> +++ b/include/io/mpqemu-link.h
> @@ -38,6 +38,7 @@
>  typedef enum {
>      INIT = 0,
>      SYNC_SYSMEM,
> +    CONNECT_DEV,
>      MAX,
>  } mpqemu_cmd_t;
>  
> @@ -120,8 +121,12 @@ struct MPQemuLinkState {
>      GMainLoop *loop;
>  
>      MPQemuChannel *com;
> +    MPQemuChannel *dev;
>  
>      mpqemu_link_callback callback;
> +
> +    void *opaque;
> +    QemuThread thread;
>  };
>  
>  MPQemuLinkState *mpqemu_link_create(void);
> diff --git a/io/mpqemu-link.c b/io/mpqemu-link.c
> index 3f81cef96e..f780b65181 100644
> --- a/io/mpqemu-link.c
> +++ b/io/mpqemu-link.c
> @@ -46,6 +46,9 @@ MPQemuLinkState *mpqemu_link_create(void)
>      MPQemuLinkState *link = MPQEMU_LINK(object_new(TYPE_MPQEMU_LINK));
>  
>      link->com = NULL;
> +    link->dev = NULL;
> +
> +    link->opaque = NULL;
>  
>      return link;
>  }
> diff --git a/remote/remote-main.c b/remote/remote-main.c
> index dbd6ad2529..f541baae6a 100644
> --- a/remote/remote-main.c
> +++ b/remote/remote-main.c
> @@ -35,6 +35,9 @@
>  #include "exec/ramlist.h"
>  #include "remote/remote-common.h"
>  
> +static void process_msg(GIOCondition cond, MPQemuLinkState *link,
> +                        MPQemuChannel *chan);
> +
>  static MPQemuLinkState *mpqemu_link;
>  
>  gchar *print_pid_exec(gchar *str)
> @@ -48,6 +51,43 @@ gchar *print_pid_exec(gchar *str)
>      return str;
>  }
>  
> +#define LINK_TO_DEV(link) ((PCIDevice *)link->opaque)
> +
> +static gpointer dev_thread(gpointer data)
> +{
> +    MPQemuLinkState *link = data;
> +
> +    mpqemu_start_coms(link, link->dev);
> +
> +    return NULL;
> +}
> +
> +static void process_connect_dev_msg(MPQemuMsg *msg)
> +{
> +    char *devid = (char *)msg->data2;

Input validation is missing for this message. We may not have data2 or
it may not have a NUL-terminator.

> +    MPQemuLinkState *link = NULL;
> +    DeviceState *dev = NULL;
> +    int wait = msg->fds[0];

msg->num_fds wasn't checked.

> +    int ret = 0;
> +
> +    dev = qdev_find_recursive(sysbus_get_default(), devid);
> +    if (!dev) {
> +        ret = 0xff;
> +        goto exit;
> +    }
> +
> +    link = mpqemu_link_create();
> +    link->opaque = (void *)PCI_DEVICE(dev);

Missing check to see if dev is a PCIDevice subclass.

signature.asc
Description: PGP signature