Re: [Qemu-devel] [PATCH v6 2/6] Qemu-Xen-vTPM: Xen frontend driver infrastructure

[Stefano Stabellini]

On Mon, 4 May 2015, Quan Xu wrote:
> This patch adds infrastructure for xen front drivers living in qemu,
> so drivers don't need to implement common stuff on their own.  It's
> mostly xenbus management stuff: some functions to access XenStore,
> setting up XenStore watches, callbacks on device discovery and state
> changes, and handle event channel between the virtual machines.
> 
> Call xen_fe_register() function to register XenDevOps, and make sure,
> XenDevOps's flags is DEVOPS_FLAG_FE, which is flag bit to point out
> the XenDevOps is Xen frontend.
> 
> Create a new file xen_pvdev.c for some common part of xen frontend
> and backend, such as xendevs queue and xenstore update functions.
> 
> Signed-off-by: Quan Xu <address@hidden>

Better than the early versions, thanks.

However the patch is too big and it is too difficult to read as is.
Could you please split it in two: a patch that creates xen_pvdev.c and
moves a few functions from xen_backend.c to it and a second patch that
introduces xen_frontend.c.


>  hw/display/xenfb.c           |   4 +-
>  hw/xen/Makefile.objs         |   2 +-
>  hw/xen/xen_backend.c         | 353 -------------------------------
>  hw/xen/xen_frontend.c        | 345 +++++++++++++++++++++++++++++++
>  hw/xen/xen_pvdev.c           | 481 
> +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/xen/xen_backend.h |  22 +-
>  6 files changed, 850 insertions(+), 357 deletions(-)
>  create mode 100644 hw/xen/xen_frontend.c
>  create mode 100644 hw/xen/xen_pvdev.c
> 
> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
> index 5e324ef..10751df 100644
> --- a/hw/display/xenfb.c
> +++ b/hw/display/xenfb.c
> @@ -988,8 +988,8 @@ void xen_init_display(int domid)
>  wait_more:
>      i++;
>      main_loop_wait(true);
> -    xfb = xen_be_find_xendev("vfb", domid, 0);
> -    xin = xen_be_find_xendev("vkbd", domid, 0);
> +    xfb = xen_find_xendev("vfb", domid, 0);
> +    xin = xen_find_xendev("vkbd", domid, 0);
>      if (!xfb || !xin) {
>          if (i < 256) {
>              usleep(10000);
> diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs
> index a0ca0aa..95eb9d0 100644
> --- a/hw/xen/Makefile.objs
> +++ b/hw/xen/Makefile.objs
> @@ -1,5 +1,5 @@
>  # xen backend driver support
> -common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o
> +common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o 
> xen_frontend.o xen_pvdev.o
>  
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o
>  obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o 
> xen_pt_msi.o
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index b2cb22b..844f918 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -44,86 +44,11 @@
>  /* ------------------------------------------------------------- */
>  
>  /* public */
> -XenXC xen_xc = XC_HANDLER_INITIAL_VALUE;
> -struct xs_handle *xenstore = NULL;
>  const char *xen_protocol;
>  
>  /* private */
> -static QTAILQ_HEAD(XenDeviceHead, XenDevice) xendevs = 
> QTAILQ_HEAD_INITIALIZER(xendevs);
>  static int debug = 0;
>  
> -/* ------------------------------------------------------------- */
> -
> -int xenstore_write_str(const char *base, const char *node, const char *val)
> -{
> -    char abspath[XEN_BUFSIZE];
> -
> -    snprintf(abspath, sizeof(abspath), "%s/%s", base, node);
> -    if (!xs_write(xenstore, 0, abspath, val, strlen(val))) {
> -        return -1;
> -    }
> -    return 0;
> -}
> -
> -char *xenstore_read_str(const char *base, const char *node)
> -{
> -    char abspath[XEN_BUFSIZE];
> -    unsigned int len;
> -    char *str, *ret = NULL;
> -
> -    snprintf(abspath, sizeof(abspath), "%s/%s", base, node);
> -    str = xs_read(xenstore, 0, abspath, &len);
> -    if (str != NULL) {
> -        /* move to qemu-allocated memory to make sure
> -         * callers can savely g_free() stuff. */
> -        ret = g_strdup(str);
> -        free(str);
> -    }
> -    return ret;
> -}
> -
> -int xenstore_write_int(const char *base, const char *node, int ival)
> -{
> -    char val[12];
> -
> -    snprintf(val, sizeof(val), "%d", ival);
> -    return xenstore_write_str(base, node, val);
> -}
> -
> -int xenstore_write_int64(const char *base, const char *node, int64_t ival)
> -{
> -    char val[21];
> -
> -    snprintf(val, sizeof(val), "%"PRId64, ival);
> -    return xenstore_write_str(base, node, val);
> -}
> -
> -int xenstore_read_int(const char *base, const char *node, int *ival)
> -{
> -    char *val;
> -    int rc = -1;
> -
> -    val = xenstore_read_str(base, node);
> -    if (val && 1 == sscanf(val, "%d", ival)) {
> -        rc = 0;
> -    }
> -    g_free(val);
> -    return rc;
> -}
> -
> -int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
> -{
> -    char *val;
> -    int rc = -1;
> -
> -    val = xenstore_read_str(base, node);
> -    if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
> -        rc = 0;
> -    }
> -    g_free(val);
> -    return rc;
> -}
> -
>  int xenstore_write_be_str(struct XenDevice *xendev, const char *node, const 
> char *val)
>  {
>      return xenstore_write_str(xendev->be, node, val);
> @@ -195,183 +120,6 @@ int xen_be_set_state(struct XenDevice *xendev, enum 
> xenbus_state state)
>  }
>  
>  /* ------------------------------------------------------------- */
> -
> -struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev)
> -{
> -    struct XenDevice *xendev;
> -
> -    QTAILQ_FOREACH(xendev, &xendevs, next) {
> -        if (xendev->dom != dom) {
> -            continue;
> -        }
> -        if (xendev->dev != dev) {
> -            continue;
> -        }
> -        if (strcmp(xendev->type, type) != 0) {
> -            continue;
> -        }
> -        return xendev;
> -    }
> -    return NULL;
> -}
> -
> -/*
> - * get xen backend device, allocate a new one if it doesn't exist.
> - */
> -static struct XenDevice *xen_be_get_xendev(const char *type, int dom, int 
> dev,
> -                                           struct XenDevOps *ops)
> -{
> -    struct XenDevice *xendev;
> -
> -    xendev = xen_be_find_xendev(type, dom, dev);
> -    if (xendev) {
> -        return xendev;
> -    }
> -
> -    /* init new xendev */
> -    xendev = g_malloc0(ops->size);
> -    xendev->type  = type;
> -    xendev->dom   = dom;
> -    xendev->dev   = dev;
> -    xendev->ops   = ops;
> -
> -    snprintf(xendev->be, sizeof(xendev->be), "backend/%s/%d/%d",
> -             xendev->type, xendev->dom, xendev->dev);
> -    snprintf(xendev->name, sizeof(xendev->name), "%s-%d",
> -             xendev->type, xendev->dev);
> -
> -    xendev->debug      = debug;
> -    xendev->local_port = -1;
> -
> -    xendev->evtchndev = xen_xc_evtchn_open(NULL, 0);
> -    if (xendev->evtchndev == XC_HANDLER_INITIAL_VALUE) {
> -        xen_be_printf(NULL, 0, "can't open evtchn device\n");
> -        g_free(xendev);
> -        return NULL;
> -    }
> -    fcntl(xc_evtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
> -
> -    if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
> -        xendev->gnttabdev = xen_xc_gnttab_open(NULL, 0);
> -        if (xendev->gnttabdev == XC_HANDLER_INITIAL_VALUE) {
> -            xen_be_printf(NULL, 0, "can't open gnttab device\n");
> -            xc_evtchn_close(xendev->evtchndev);
> -            g_free(xendev);
> -            return NULL;
> -        }
> -    } else {
> -        xendev->gnttabdev = XC_HANDLER_INITIAL_VALUE;
> -    }
> -
> -    QTAILQ_INSERT_TAIL(&xendevs, xendev, next);
> -
> -    if (xendev->ops->alloc) {
> -        xendev->ops->alloc(xendev);
> -    }
> -
> -    return xendev;
> -}
> -
> -/*
> - * release xen backend device.
> - */
> -static struct XenDevice *xen_be_del_xendev(int dom, int dev)
> -{
> -    struct XenDevice *xendev, *xnext;
> -
> -    /*
> -     * This is pretty much like QTAILQ_FOREACH(xendev, &xendevs, next) but
> -     * we save the next pointer in xnext because we might free xendev.
> -     */
> -    xnext = xendevs.tqh_first;
> -    while (xnext) {
> -        xendev = xnext;
> -        xnext = xendev->next.tqe_next;
> -
> -        if (xendev->dom != dom) {
> -            continue;
> -        }
> -        if (xendev->dev != dev && dev != -1) {
> -            continue;
> -        }
> -
> -        if (xendev->ops->free) {
> -            xendev->ops->free(xendev);
> -        }
> -
> -        if (xendev->fe) {
> -            char token[XEN_BUFSIZE];
> -            snprintf(token, sizeof(token), "fe:%p", xendev);
> -            xs_unwatch(xenstore, xendev->fe, token);
> -            g_free(xendev->fe);
> -        }
> -
> -        if (xendev->evtchndev != XC_HANDLER_INITIAL_VALUE) {
> -            xc_evtchn_close(xendev->evtchndev);
> -        }
> -        if (xendev->gnttabdev != XC_HANDLER_INITIAL_VALUE) {
> -            xc_gnttab_close(xendev->gnttabdev);
> -        }
> -
> -        QTAILQ_REMOVE(&xendevs, xendev, next);
> -        g_free(xendev);
> -    }
> -    return NULL;
> -}
> -
> -/*
> - * Sync internal data structures on xenstore updates.
> - * Node specifies the changed field.  node = NULL means
> - * update all fields (used for initialization).
> - */
> -static void xen_be_backend_changed(struct XenDevice *xendev, const char 
> *node)
> -{
> -    if (node == NULL  ||  strcmp(node, "online") == 0) {
> -        if (xenstore_read_be_int(xendev, "online", &xendev->online) == -1) {
> -            xendev->online = 0;
> -        }
> -    }
> -
> -    if (node) {
> -        xen_be_printf(xendev, 2, "backend update: %s\n", node);
> -        if (xendev->ops->backend_changed) {
> -            xendev->ops->backend_changed(xendev, node);
> -        }
> -    }
> -}
> -
> -static void xen_be_frontend_changed(struct XenDevice *xendev, const char 
> *node)
> -{
> -    int fe_state;
> -
> -    if (node == NULL  ||  strcmp(node, "state") == 0) {
> -        if (xenstore_read_fe_int(xendev, "state", &fe_state) == -1) {
> -            fe_state = XenbusStateUnknown;
> -        }
> -        if (xendev->fe_state != fe_state) {
> -            xen_be_printf(xendev, 1, "frontend state: %s -> %s\n",
> -                          xenbus_strstate(xendev->fe_state),
> -                          xenbus_strstate(fe_state));
> -        }
> -        xendev->fe_state = fe_state;
> -    }
> -    if (node == NULL  ||  strcmp(node, "protocol") == 0) {
> -        g_free(xendev->protocol);
> -        xendev->protocol = xenstore_read_fe_str(xendev, "protocol");
> -        if (xendev->protocol) {
> -            xen_be_printf(xendev, 1, "frontend protocol: %s\n", 
> xendev->protocol);
> -        }
> -    }
> -
> -    if (node) {
> -        xen_be_printf(xendev, 2, "frontend update: %s\n", node);
> -        if (xendev->ops->frontend_changed) {
> -            xendev->ops->frontend_changed(xendev, node);
> -        }
> -    }
> -}
> -
> -/* ------------------------------------------------------------- */
>  /* Check for possible state transitions and perform them.        */
>  
>  /*
> @@ -611,81 +359,6 @@ static int xenstore_scan(const char *type, int dom, 
> struct XenDevOps *ops)
>      return 0;
>  }
>  
> -static void xenstore_update_be(char *watch, char *type, int dom,
> -                               struct XenDevOps *ops)
> -{
> -    struct XenDevice *xendev;
> -    char path[XEN_BUFSIZE], *bepath;
> -    unsigned int len, dev;
> -
> -    len = snprintf(path, sizeof(path), "backend/%s/%d", type, dom);
> -    if (strncmp(path, watch, len) != 0) {
> -        return;
> -    }
> -    if (sscanf(watch+len, "/%u/%255s", &dev, path) != 2) {
> -        strcpy(path, "");
> -        if (sscanf(watch+len, "/%u", &dev) != 1) {
> -            dev = -1;
> -        }
> -    }
> -    if (dev == -1) {
> -        return;
> -    }
> -
> -    xendev = xen_be_get_xendev(type, dom, dev, ops);
> -    if (xendev != NULL) {
> -        bepath = xs_read(xenstore, 0, xendev->be, &len);
> -        if (bepath == NULL) {
> -            xen_be_del_xendev(dom, dev);
> -        } else {
> -            free(bepath);
> -            xen_be_backend_changed(xendev, path);
> -            xen_be_check_state(xendev);
> -        }
> -    }
> -}
> -
> -static void xenstore_update_fe(char *watch, struct XenDevice *xendev)
> -{
> -    char *node;
> -    unsigned int len;
> -
> -    len = strlen(xendev->fe);
> -    if (strncmp(xendev->fe, watch, len) != 0) {
> -        return;
> -    }
> -    if (watch[len] != '/') {
> -        return;
> -    }
> -    node = watch + len + 1;
> -
> -    xen_be_frontend_changed(xendev, node);
> -    xen_be_check_state(xendev);
> -}
> -
> -static void xenstore_update(void *unused)
> -{
> -    char **vec = NULL;
> -    intptr_t type, ops, ptr;
> -    unsigned int dom, count;
> -
> -    vec = xs_read_watch(xenstore, &count);
> -    if (vec == NULL) {
> -        goto cleanup;
> -    }
> -
> -    if (sscanf(vec[XS_WATCH_TOKEN], "be:%" PRIxPTR ":%d:%" PRIxPTR,
> -               &type, &dom, &ops) == 3) {
> -        xenstore_update_be(vec[XS_WATCH_PATH], (void*)type, dom, (void*)ops);
> -    }
> -    if (sscanf(vec[XS_WATCH_TOKEN], "fe:%" PRIxPTR, &ptr) == 1) {
> -        xenstore_update_fe(vec[XS_WATCH_PATH], (void*)ptr);
> -    }
> -
> -cleanup:
> -    free(vec);
> -}
> -
>  static void xen_be_evtchn_event(void *opaque)
>  {
>      struct XenDevice *xendev = opaque;
> @@ -706,32 +379,6 @@ static void xen_be_evtchn_event(void *opaque)
>  
>  /* -------------------------------------------------------------------- */
>  
> -int xen_be_init(void)
> -{
> -    xenstore = xs_daemon_open();
> -    if (!xenstore) {
> -        xen_be_printf(NULL, 0, "can't connect to xenstored\n");
> -        return -1;
> -    }
> -
> -    if (qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update, NULL, 
> NULL) < 0) {
> -        goto err;
> -    }
> -
> -    if (xen_xc == XC_HANDLER_INITIAL_VALUE) {
> -        /* Check if xen_init() have been called */
> -        goto err;
> -    }
> -    return 0;
> -
> -err:
> -    qemu_set_fd_handler(xs_fileno(xenstore), NULL, NULL, NULL);
> -    xs_daemon_close(xenstore);
> -    xenstore = NULL;
> -
> -    return -1;
> -}
> -
>  int xen_be_register(const char *type, struct XenDevOps *ops)
>  {
>      return xenstore_scan(type, xen_domid, ops);
> diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c
> new file mode 100644
> index 0000000..55af45a
> --- /dev/null
> +++ b/hw/xen/xen_frontend.c
> @@ -0,0 +1,345 @@
> +/*
> + * Xen frontend driver infrastructure
> + *
> + *  Copyright (c) 2015 Intel Corporation
> + *  Authors:
> + *    Quan Xu <address@hidden>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/mman.h>
> +#include <sys/signal.h>
> +
> +#include "hw/hw.h"
> +#include "sysemu/char.h"
> +#include "qemu/log.h"
> +#include "hw/xen/xen_backend.h"
> +#include <xen/grant_table.h>
> +
> +int xenstore_dev;
> +
> +/* private */
> +static int debug;
> +
> +static void xen_fe_evtchn_event(void *opaque)
> +{
> +    struct XenDevice *xendev = opaque;
> +    evtchn_port_t port;
> +
> +    port = xc_evtchn_pending(xendev->evtchndev);
> +    if (port != xendev->local_port) {
> +        return;
> +    }
> +    xc_evtchn_unmask(xendev->evtchndev, port);
> +
> +    if (xendev->ops->event) {
> +        xendev->ops->event(xendev);
> +    }
> +}

This function is basically identical to xen_be_evtchn_event: could we
find a way to generalize it and have only one?


> +/* ------------------------------------------------------------- */
> +
> +int xen_fe_alloc_unbound(struct XenDevice *xendev, int dom, int remote_dom)
> +{
> +    xendev->local_port = xc_evtchn_bind_unbound_port(xendev->evtchndev,
> +                                                     remote_dom);
> +    if (xendev->local_port == -1) {
> +        xen_fe_printf(xendev, 0, "xc_evtchn_alloc_unbound failed\n");
> +        return -1;
> +    }
> +    xen_fe_printf(xendev, 2, "bind evtchn port %d\n", xendev->local_port);
> +    qemu_set_fd_handler(xc_evtchn_fd(xendev->evtchndev),
> +                        xen_fe_evtchn_event, NULL, xendev);
> +    return 0;
> +}
> +
> +/*
> + * Make sure, initialize the 'xendev->fe' in xendev->ops->init() or
> + * xendev->ops->initialize()
> + */
> +int xenbus_switch_state(struct XenDevice *xendev, enum xenbus_state xbus)
> +{
> +    xs_transaction_t xbt = XBT_NULL;
> +
> +    if (xendev->fe_state == xbus) {
> +        return 0;
> +    }
> +
> +    xendev->fe_state = xbus;
> +    if (xendev->fe == NULL) {
> +        xen_fe_printf(NULL, 0, "xendev->fe is NULL\n");
> +        return -1;
> +    }
> +
> +retry_transaction:
> +    xbt = xs_transaction_start(xenstore);
> +    if (xbt == XBT_NULL) {
> +        goto abort_transaction;
> +    }
> +
> +    if (xenstore_write_int(xendev->fe, "state", xbus)) {
> +        goto abort_transaction;
> +    }
> +
> +    if (!xs_transaction_end(xenstore, xbt, 0)) {
> +        if (errno == EAGAIN) {
> +            goto retry_transaction;
> +        }
> +    }
> +
> +    return 0;
> +
> +abort_transaction:
> +    xs_transaction_end(xenstore, xbt, 1);
> +    return -1;
> +}

This function could probably be unified with xen_be_set_state.


> +/*
> + * Simplify QEMU side, a thread is running in Xen backend, which will
> + * connect frontend when the frontend is initialised. Call these initialised
> + * functions.
> + */
> +static int xen_fe_try_init(void *opaque)
> +{
> +    struct XenDevOps *ops = opaque;
> +    int rc = -1;
> +
> +    if (ops->init) {
> +        rc = ops->init(NULL);
> +    }
> +
> +    return rc;
> +}
> +
> +static int xen_fe_try_initialise(struct XenDevice *xendev)
> +{
> +    int rc = 0, fe_state;
> +
> +    if (xenstore_read_fe_int(xendev, "state", &fe_state) == -1) {
> +        fe_state = XenbusStateUnknown;
> +    }
> +    xendev->fe_state = fe_state;
> +
> +    if (xendev->ops->initialise) {
> +        rc = xendev->ops->initialise(xendev);
> +    }
> +    if (rc != 0) {
> +        xen_fe_printf(xendev, 0, "initialise() failed\n");
> +        return rc;
> +    }
> +
> +    xenbus_switch_state(xendev, XenbusStateInitialised);
> +    return 0;
> +}
> +
> +static void xen_fe_try_connected(struct XenDevice *xendev)
> +{
> +    if (!xendev->ops->connected) {
> +        return;
> +    }
> +
> +    if (xendev->fe_state != XenbusStateConnected) {
> +        if (xendev->ops->flags & DEVOPS_FLAG_IGNORE_STATE) {
> +            xen_fe_printf(xendev, 2, "frontend not ready, ignoring\n");
> +        } else {
> +            xen_fe_printf(xendev, 2, "frontend not ready (yet)\n");
> +            return;
> +        }
> +    }
> +
> +    xendev->ops->connected(xendev);
> +}
> +
> +static int xen_fe_check(struct XenDevice *xendev, uint32_t domid,
> +                        int handle)
> +{
> +    int rc = 0;
> +
> +    rc = xen_fe_try_initialise(xendev);
> +    if (rc != 0) {
> +        xen_fe_printf(xendev, 0, "xendev %s initialise error\n",
> +                      xendev->name);
> +        goto err;
> +    }
> +    xen_fe_try_connected(xendev);
> +
> +    return rc;
> +
> +err:
> +    xen_del_xendev(domid, handle);
> +    return -1;
> +}
> +
> +static char *xenstore_fe_get_backend(const char *type, int be_domid,
> +                                     uint32_t domid, int *hdl)
> +{
> +    char *name, *str, *ret = NULL;
> +    uint32_t i, cdev;
> +    int handle = 0;
> +    char path[XEN_BUFSIZE];
> +    char **dev = NULL;
> +
> +    name = xenstore_get_domain_name(domid);
> +    snprintf(path, sizeof(path), "frontend/%s/%d", type, be_domid);
> +    dev = xs_directory(xenstore, 0, path, &cdev);
> +    for (i = 0; i < cdev; i++) {
> +        handle = i;
> +        snprintf(path, sizeof(path), "frontend/%s/%d/%d",
> +        type, be_domid, handle);
> +        str = xenstore_read_str(path, "domain");
> +        if (!strcmp(name, str)) {
> +            break;
> +        }
> +
> +        free(str);
> +
> +        /* Not the backend domain */
> +        if (handle == (cdev - 1)) {
> +            goto err;
> +        }
> +    }
> +
> +    snprintf(path, sizeof(path), "frontend/%s/%d/%d",
> +    type, be_domid, handle);
> +    str = xenstore_read_str(path, "backend");
> +    if (str != NULL) {
> +        ret = g_strdup(str);
> +        free(str);
> +    }
> +
> +    *hdl = handle;
> +    free(dev);
> +
> +    return ret;
> +err:
> +    *hdl = -1;
> +    free(dev);
> +    return NULL;
> +}
> +
> +static int xenstore_fe_scan(const char *type, uint32_t domid,
> +                            struct XenDevOps *ops)
> +{
> +    struct XenDevice *xendev;
> +    char path[XEN_BUFSIZE], token[XEN_BUFSIZE];
> +    unsigned int cdev, j;
> +    char *backend;
> +    char **dev = NULL;
> +    int rc;
> +
> +    /* ops .init check, xendev is NOT initialized */
> +    rc = xen_fe_try_init(ops);
> +    if (rc != 0) {
> +        return -1;
> +    }
> +
> +    /* Get /local/domain/0/${type}/{} directory */
> +    snprintf(path, sizeof(path), "frontend/%s", type);
> +    dev = xs_directory(xenstore, 0, path, &cdev);
> +    if (dev == NULL) {
> +        return 0;
> +    }
> +
> +    for (j = 0; j < cdev; j++) {
> +
> +        /* Get backend via domain name */
> +        backend = xenstore_fe_get_backend(type, atoi(dev[j]),
> +                                          domid, &xenstore_dev);
> +        if (backend == NULL) {
> +            continue;
> +        }
> +
> +        xendev = xen_fe_get_xendev(type, domid, xenstore_dev, backend, ops);
> +        free(backend);
> +        if (xendev == NULL) {
> +            xen_fe_printf(xendev, 0, "xendev is NULL.\n");
> +            continue;
> +        }
> +
> +        /*
> +         * Simplify QEMU side, a thread is running in Xen backend, which will
> +         * connect frontend when the frontend is initialised.
> +         */
> +        if (xen_fe_check(xendev, domid, xenstore_dev) < 0) {
> +            xen_fe_printf(xendev, 0, "xendev fe_check error.\n");
> +            continue;
> +        }
> +
> +        /* Setup watch */
> +        snprintf(token, sizeof(token), "be:%p:%d:%p",
> +                 type, domid, xendev->ops);
> +        if (!xs_watch(xenstore, xendev->be, token)) {
> +            xen_fe_printf(xendev, 0, "xs_watch failed.\n");
> +            continue;
> +        }
> +    }
> +
> +    free(dev);
> +    return 0;
> +}
> +
> +int xen_fe_register(const char *type, struct XenDevOps *ops)
> +{
> +    return xenstore_fe_scan(type, xen_domid, ops);
> +}
> +
> +/*
> + * msg_level:
> + *  0 == errors (stderr + logfile).
> + *  1 == informative debug messages (logfile only).
> + *  2 == noisy debug messages (logfile only).
> + *  3 == will flood your log (logfile only).
> + */
> +void xen_fe_printf(struct XenDevice *xendev, int msg_level,
> +                   const char *fmt, ...)
> +{
> +    va_list args;
> +
> +    if (xendev) {
> +        if (msg_level > xendev->debug) {
> +            return;
> +        }
> +        qemu_log("xen fe: %s: ", xendev->name);
> +        if (msg_level == 0) {
> +            fprintf(stderr, "xen fe: %s: ", xendev->name);
> +        }
> +    } else {
> +        if (msg_level > debug) {
> +            return;
> +        }
> +        qemu_log("xen fe core: ");
> +        if (msg_level == 0) {
> +            fprintf(stderr, "xen fe core: ");
> +        }
> +    }
> +    va_start(args, fmt);
> +    qemu_log_vprintf(fmt, args);
> +    va_end(args);
> +    if (msg_level == 0) {
> +        va_start(args, fmt);
> +        vfprintf(stderr, fmt, args);
> +        va_end(args);
> +    }
> +    qemu_log_flush();

There is no need to introduce another one of these. Just rename
xen_be_printf to xen_pv_printf and move it it xen_pvdev.c.

In general we should try to share as many functions as possible between
the frontend and the backend code. To do that we need to modify the
existing backend functions, such as xen_be_printf, generalize them so
that they can also be used by frontends (in this case renaming
xen_be_printf to xen_pv_printf should be enough), and move them to
xen_pvdev.c.


> diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
> new file mode 100644
> index 0000000..dac8639
> --- /dev/null
> +++ b/hw/xen/xen_pvdev.c
> @@ -0,0 +1,481 @@
> +/*
> + * Xen para-virtualization device
> + *
> + *  Copyright (c) 2015 Intel Corporation
> + *  Authors:
> + *    Quan Xu <address@hidden>
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> <http://www.gnu.org/licenses/>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/mman.h>
> +#include <sys/signal.h>
> +
> +#include "hw/hw.h"
> +#include "sysemu/char.h"
> +#include "qemu/log.h"
> +#include "hw/xen/xen_backend.h"
> +
> +#include <xen/grant_table.h>
> +
> +static QTAILQ_HEAD(XenDeviceHead, XenDevice) xendevs =
> +           QTAILQ_HEAD_INITIALIZER(xendevs);
> +static int debug;
> +
> +/* public */
> +XenXC xen_xc = XC_HANDLER_INITIAL_VALUE;
> +struct xs_handle *xenstore;
> +
> +/*
> + * find Xen device
> + */
> +struct XenDevice *xen_find_xendev(const char *type, int dom, int dev)
> +{
> +    struct XenDevice *xendev;
> +
> +    QTAILQ_FOREACH(xendev, &xendevs, next) {
> +        if (xendev->dom != dom) {
> +            continue;
> +        }
> +        if (xendev->dev != dev) {
> +            continue;
> +        }
> +        if (strcmp(xendev->type, type) != 0) {
> +            continue;
> +        }
> +        return xendev;
> +    }
> +    return NULL;
> +}
> +
> +/*
> + * get xen backend device, allocate a new one if it doesn't exist.
> + */
> +struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
> +                                    struct XenDevOps *ops)
> +{
> +    struct XenDevice *xendev;
> +
> +    xendev = xen_find_xendev(type, dom, dev);
> +    if (xendev) {
> +        return xendev;
> +    }
> +
> +    /* init new xendev */
> +    xendev = g_malloc0(ops->size);
> +    xendev->type  = type;
> +    xendev->dom   = dom;
> +    xendev->dev   = dev;
> +    xendev->ops   = ops;
> +
> +    snprintf(xendev->be, sizeof(xendev->be), "backend/%s/%d/%d",
> +             xendev->type, xendev->dom, xendev->dev);
> +    snprintf(xendev->name, sizeof(xendev->name), "%s-%d",
> +             xendev->type, xendev->dev);
> +
> +    xendev->debug      = debug;
> +    xendev->local_port = -1;
> +
> +    xendev->evtchndev = xen_xc_evtchn_open(NULL, 0);
> +    if (xendev->evtchndev == XC_HANDLER_INITIAL_VALUE) {
> +        xen_be_printf(NULL, 0, "can't open evtchn device\n");
> +        g_free(xendev);
> +        return NULL;
> +    }
> +    fcntl(xc_evtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
> +
> +    if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
> +        xendev->gnttabdev = xen_xc_gnttab_open(NULL, 0);
> +        if (xendev->gnttabdev == XC_HANDLER_INITIAL_VALUE) {
> +            xen_be_printf(NULL, 0, "can't open gnttab device\n");
> +            xc_evtchn_close(xendev->evtchndev);
> +            g_free(xendev);
> +            return NULL;
> +        }
> +    } else {
> +        xendev->gnttabdev = XC_HANDLER_INITIAL_VALUE;
> +    }
> +
> +    QTAILQ_INSERT_TAIL(&xendevs, xendev, next);
> +
> +    if (xendev->ops->alloc) {
> +        xendev->ops->alloc(xendev);
> +    }
> +
> +    return xendev;
> +}

Only functions that are shared between frontends and backends should be
here, like xen_find_xendev. Otherwise there is no benefit in moving them
to a separete, shared, source file.


> +/*
> + * get xen fe device, allocate a new one if it doesn't exist.
> + */
> +struct XenDevice *xen_fe_get_xendev(const char *type, int dom, int dev,
> +                                    char *backend, struct XenDevOps *ops)
> +{
> +    struct XenDevice *xendev;
> +
> +    xendev = xen_find_xendev(type, dom, dev);
> +    if (xendev) {
> +        return xendev;
> +    }
> +
> +    /* init new xendev */
> +    xendev = g_malloc0(ops->size);
> +    xendev->type  = type;
> +    xendev->dom   = dom;
> +    xendev->dev   = dev;
> +    xendev->ops   = ops;
> +
> +    /*return if the ops->flags is not DEVOPS_FLAG_FE*/
> +    if (!(ops->flags & DEVOPS_FLAG_FE)) {
> +        return NULL;
> +    }
> +
> +    snprintf(xendev->be, sizeof(xendev->be), "%s", backend);
> +    snprintf(xendev->name, sizeof(xendev->name), "%s-%d",
> +             xendev->type, xendev->dev);
> +
> +    xendev->debug = debug;
> +    xendev->local_port = -1;
> +
> +    xendev->evtchndev = xen_xc_evtchn_open(NULL, 0);
> +    if (xendev->evtchndev == XC_HANDLER_INITIAL_VALUE) {
> +        xen_be_printf(NULL, 0, "can't open evtchn device\n");
> +        g_free(xendev);
> +        return NULL;
> +    }
> +    fcntl(xc_evtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
> +
> +    if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
> +        xendev->gnttabdev = xen_xc_gnttab_open(NULL, 0);
> +        if (xendev->gnttabdev == XC_HANDLER_INITIAL_VALUE) {
> +            xen_be_printf(NULL, 0, "can't open gnttab device\n");
> +            xc_evtchn_close(xendev->evtchndev);
> +            g_free(xendev);
> +            return NULL;
> +        }
> +    } else {
> +        xendev->gnttabdev = XC_HANDLER_INITIAL_VALUE;
> +    }
> +
> +    QTAILQ_INSERT_TAIL(&xendevs, xendev, next);
> +
> +    if (xendev->ops->alloc) {
> +        xendev->ops->alloc(xendev);
> +    }
> +
> +    return xendev;
> +}

same here


> +/*
> + * release xen device
> + */
> +
> +struct XenDevice *xen_del_xendev(int dom, int dev)
> +{
> +    struct XenDevice *xendev, *xnext;
> +
> +    /*
> +     * This is pretty much like QTAILQ_FOREACH(xendev, &xendevs, next) but
> +     * we save the next pointer in xnext because we might free xendev.
> +     */
> +    xnext = xendevs.tqh_first;
> +    while (xnext) {
> +        xendev = xnext;
> +        xnext = xendev->next.tqe_next;
> +
> +        if (xendev->dom != dom) {
> +            continue;
> +        }
> +        if (xendev->dev != dev && dev != -1) {
> +            continue;
> +        }
> +
> +        if (xendev->ops->free) {
> +            xendev->ops->free(xendev);
> +        }
> +
> +        if (xendev->fe) {
> +            char token[XEN_BUFSIZE];
> +            snprintf(token, sizeof(token), "fe:%p", xendev);
> +            xs_unwatch(xenstore, xendev->fe, token);
> +            g_free(xendev->fe);
> +        }
> +
> +        if (xendev->evtchndev != XC_HANDLER_INITIAL_VALUE) {
> +            xc_evtchn_close(xendev->evtchndev);
> +        }
> +        if (xendev->gnttabdev != XC_HANDLER_INITIAL_VALUE) {
> +            xc_gnttab_close(xendev->gnttabdev);
> +        }
> +
> +        QTAILQ_REMOVE(&xendevs, xendev, next);
> +        g_free(xendev);
> +    }
> +    return NULL;
> +}
> +
> +/* ------------------------------------------------------------- */
> +
> +int xenstore_write_str(const char *base, const char *node, const char *val)
> +{
> +    char abspath[XEN_BUFSIZE];
> +
> +    snprintf(abspath, sizeof(abspath), "%s/%s", base, node);
> +    if (!xs_write(xenstore, 0, abspath, val, strlen(val))) {
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +char *xenstore_read_str(const char *base, const char *node)
> +{
> +    char abspath[XEN_BUFSIZE];
> +    unsigned int len;
> +    char *str, *ret = NULL;
> +
> +    snprintf(abspath, sizeof(abspath), "%s/%s", base, node);
> +    str = xs_read(xenstore, 0, abspath, &len);
> +    if (str != NULL) {
> +        /* move to qemu-allocated memory to make sure
> +         * callers can savely g_free() stuff. */
> +        ret = g_strdup(str);
> +        free(str);
> +    }
> +    return ret;
> +}
> +
> +int xenstore_write_int(const char *base, const char *node, int ival)
> +{
> +    char val[12];
> +
> +    snprintf(val, sizeof(val), "%d", ival);
> +    return xenstore_write_str(base, node, val);
> +}
> +
> +int xenstore_write_int64(const char *base, const char *node, int64_t ival)
> +{
> +    char val[21];
> +
> +    snprintf(val, sizeof(val), "%"PRId64, ival);
> +    return xenstore_write_str(base, node, val);
> +}
> +
> +int xenstore_read_int(const char *base, const char *node, int *ival)
> +{
> +    char *val;
> +    int rc = -1;
> +
> +    val = xenstore_read_str(base, node);
> +    if (val && 1 == sscanf(val, "%d", ival)) {
> +        rc = 0;
> +    }
> +    g_free(val);
> +    return rc;
> +}
> +
> +int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval)
> +{
> +    char *val;
> +    int rc = -1;
> +
> +    val = xenstore_read_str(base, node);
> +    if (val && 1 == sscanf(val, "%"SCNu64, uval)) {
> +        rc = 0;
> +    }
> +    g_free(val);
> +    return rc;
> +}
> +
> +char *xenstore_get_domain_name(uint32_t domid)
> +{
> +    char *dom_path, *str, *ret = NULL;;
> +
> +    dom_path = xs_get_domain_path(xenstore, domid);
> +    str = xenstore_read_str(dom_path, "name");
> +    free(dom_path);
> +    if (str != NULL) {
> +        ret = g_strdup(str);
> +        free(str);
> +    }
> +
> +    return ret;
> +}
> +
> +/*
> + * Sync internal data structures on xenstore updates.
> + * Node specifies the changed field.  node = NULL means
> + * update all fields (used for initialization).
> + */
> +void xen_be_backend_changed(struct XenDevice *xendev, const char *node)
> +{
> +    if (node == NULL  ||  strcmp(node, "online") == 0) {
> +        if (xenstore_read_be_int(xendev, "online", &xendev->online) == -1) {
> +            xendev->online = 0;
> +        }
> +    }
> +
> +    if (node) {
> +        xen_be_printf(xendev, 2, "backend update: %s\n", node);
> +        if (xendev->ops->backend_changed) {
> +            xendev->ops->backend_changed(xendev, node);
> +        }
> +    }
> +}
> +
> +void xen_be_frontend_changed(struct XenDevice *xendev, const char *node)
> +{
> +    int fe_state;
> +
> +    if (node == NULL  ||  strcmp(node, "state") == 0) {
> +        if (xenstore_read_fe_int(xendev, "state", &fe_state) == -1) {
> +            fe_state = XenbusStateUnknown;
> +        }
> +        if (xendev->fe_state != fe_state) {
> +            xen_be_printf(xendev, 1, "frontend state: %s -> %s\n",
> +                          xenbus_strstate(xendev->fe_state),
> +                          xenbus_strstate(fe_state));
> +        }
> +        xendev->fe_state = fe_state;
> +    }
> +    if (node == NULL  ||  strcmp(node, "protocol") == 0) {
> +        g_free(xendev->protocol);
> +        xendev->protocol = xenstore_read_fe_str(xendev, "protocol");
> +        if (xendev->protocol) {
> +            xen_be_printf(xendev, 1, "frontend protocol: %s\n",
> +                          xendev->protocol);
> +        }
> +    }
> +
> +    if (node) {
> +        xen_be_printf(xendev, 2, "frontend update: %s\n", node);
> +        if (xendev->ops->frontend_changed) {
> +            xendev->ops->frontend_changed(xendev, node);
> +        }
> +    }
> +}

Anything backend or frontend specific, like these two functions, should either:
    
1) be made generic, shared between frontends and backends, moved to this file
2) left in the corresponding xen_backend.c or xen_frontend.c source file

The first option is better, when possible.


> +static void xenstore_update_be(char *watch, char *type, int dom,
> +                        struct XenDevOps *ops)
> +{
> +    struct XenDevice *xendev;
> +    char path[XEN_BUFSIZE], *bepath;
> +    unsigned int len, dev;
> +
> +    len = snprintf(path, sizeof(path), "backend/%s/%d", type, dom);
> +
> +    if (strstr(watch, path) == NULL) {
> +        return;
> +    }
> +    if (sscanf(watch+len, "/%u/%255s", &dev, path) != 2) {
> +        strcpy(path, "");
> +        if (sscanf(watch+len, "/%u", &dev) != 1) {
> +            dev = -1;
> +        }
> +    }
> +    if (dev == -1) {
> +        return;
> +    }
> +
> +    xendev = xen_be_get_xendev(type, dom, dev, ops);
> +    if (xendev != NULL) {
> +        bepath = xs_read(xenstore, 0, xendev->be, &len);
> +        if (bepath == NULL) {
> +            xen_del_xendev(dom, dev);
> +        } else {
> +            free(bepath);
> +            xen_be_backend_changed(xendev, path);
> +            if (!(ops->flags & DEVOPS_FLAG_FE)) {
> +                xen_be_check_state(xendev);
> +            }
> +        }
> +    }
> +}
> +
> +static void xenstore_update_fe(char *watch, struct XenDevice *xendev)
> +{
> +    char *node;
> +    unsigned int len;
> +
> +    len = strlen(xendev->fe);
> +    if (strncmp(xendev->fe, watch, len) != 0) {
> +        return;
> +    }
> +    if (watch[len] != '/') {
> +        return;
> +    }
> +    node = watch + len + 1;
> +
> +    xen_be_frontend_changed(xendev, node);
> +    xen_be_check_state(xendev);
> +}
> +
> +static void xenstore_update(void *unused)
> +{
> +    char **vec = NULL;
> +    intptr_t type, ops, ptr;
> +    unsigned int dom, count;
> +
> +    vec = xs_read_watch(xenstore, &count);
> +    if (vec == NULL) {
> +        goto cleanup;
> +    }
> +
> +    if (sscanf(vec[XS_WATCH_TOKEN], "be:%" PRIxPTR ":%d:%" PRIxPTR,
> +               &type, &dom, &ops) == 3) {
> +        xenstore_update_be(vec[XS_WATCH_PATH], (void *)type, dom, (void 
> *)ops);
> +    }
> +    if (sscanf(vec[XS_WATCH_TOKEN], "fe:%" PRIxPTR, &ptr) == 1) {
> +        xenstore_update_fe(vec[XS_WATCH_PATH], (void *)ptr);
> +    }
> +
> +cleanup:
> +    free(vec);
> +}
> +
> +int xen_be_init(void)
> +{
> +    xenstore = xs_daemon_open();
> +    if (!xenstore) {
> +        xen_be_printf(NULL, 0, "can't connect to xenstored\n");
> +        return -1;
> +    }
> +
> +    if (qemu_set_fd_handler(xs_fileno(xenstore), xenstore_update,
> +        NULL, NULL) < 0) {
> +        goto err;
> +    }
> +
> +    if (xen_xc == XC_HANDLER_INITIAL_VALUE) {
> +
> +        /* Check if xen_init() have been called */
> +        goto err;
> +    }
> +    return 0;
> +
> +err:
> +    qemu_set_fd_handler(xs_fileno(xenstore), NULL, NULL, NULL);
> +    xs_daemon_close(xenstore);
> +    xenstore = NULL;
> +
> +    return -1;
> +}
> diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
> index 3b4125e..bb0b303 100644
> --- a/include/hw/xen/xen_backend.h
> +++ b/include/hw/xen/xen_backend.h
> @@ -15,6 +15,8 @@ struct XenDevice;
>  #define DEVOPS_FLAG_NEED_GNTDEV   1
>  /* don't expect frontend doing correct state transitions (aka console quirk) 
> */
>  #define DEVOPS_FLAG_IGNORE_STATE  2
> +/*dev is frontend device*/
> +#define DEVOPS_FLAG_FE            4
>  
>  struct XenDevOps {
>      size_t    size;
> @@ -59,6 +61,7 @@ struct XenDevice {
>  extern XenXC xen_xc;
>  extern struct xs_handle *xenstore;
>  extern const char *xen_protocol;
> +extern int xenstore_dev;
>  
>  /* xenstore helper functions */
>  int xenstore_write_str(const char *base, const char *node, const char *val);
> @@ -77,9 +80,18 @@ int xenstore_read_fe_int(struct XenDevice *xendev, const 
> char *node, int *ival);
>  int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
>  int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, 
> uint64_t *uval);
>  
> +char *xenstore_get_domain_name(uint32_t domid);
> +
>  const char *xenbus_strstate(enum xenbus_state state);
> -struct XenDevice *xen_be_find_xendev(const char *type, int dom, int dev);
> +struct XenDevice *xen_find_xendev(const char *type, int dom, int dev);
> +struct XenDevice *xen_del_xendev(int dom, int dev);
> +struct XenDevice *xen_be_get_xendev(const char *type, int dom, int dev,
> +                                    struct XenDevOps *ops);
> +struct XenDevice *xen_fe_get_xendev(const char *type, int dom, int dev,
> +                                    char *backend, struct XenDevOps *ops);
>  void xen_be_check_state(struct XenDevice *xendev);
> +void xen_be_backend_changed(struct XenDevice *xendev, const char *node);
> +void xen_be_frontend_changed(struct XenDevice *xendev, const char *node);
>  
>  /* xen backend driver bits */
>  int xen_be_init(void);
> @@ -90,6 +102,13 @@ void xen_be_unbind_evtchn(struct XenDevice *xendev);
>  int xen_be_send_notify(struct XenDevice *xendev);
>  void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt, 
> ...)
>      GCC_FMT_ATTR(3, 4);
> +void xen_fe_printf(struct XenDevice *xendev, int msg_level,
> +                   const char *fmt, ...)
> +    GCC_FMT_ATTR(3, 4);
> +/* Xen frontend driver */
> +int xen_fe_register(const char *type, struct XenDevOps *ops);
> +int xen_fe_alloc_unbound(struct XenDevice *xendev, int dom, int remote_dom);
> +int xenbus_switch_state(struct XenDevice *xendev, enum xenbus_state xbus);
>  
>  /* actual backend drivers */
>  extern struct XenDevOps xen_console_ops;      /* xen_console.c     */
> @@ -97,6 +116,7 @@ extern struct XenDevOps xen_kbdmouse_ops;     /* 
> xen_framebuffer.c */
>  extern struct XenDevOps xen_framebuffer_ops;  /* xen_framebuffer.c */
>  extern struct XenDevOps xen_blkdev_ops;       /* xen_disk.c        */
>  extern struct XenDevOps xen_netdev_ops;       /* xen_nic.c         */
> +extern struct XenDevOps xen_vtpmdev_ops;      /* xen_vtpm_frontend.c*/
>  
>  void xen_init_display(int domid);
>  
> -- 
> 1.8.3.2
>