Re: [Qemu-devel] [Xen-devel] [PATCH 15/19] Qemu-Xen-vTPM: Xen frontend driver infrastructure

[Anthony PERARD]

On Sun, Jul 10, 2016 at 02:47:46PM +0300, Emil Condrea 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.
> 
> Signed-off-by: Quan Xu <address@hidden>
> Signed-off-by: Emil Condrea <address@hidden>
> 
> ---
> Changes in v9:
>  * xenstore_dev should not be global, it will not work correctly with
> multiple xen frontends living in qemu
>  * reuse some common functions:
>     - xen_fe_printf -> xen_pv_printf
>     - xen_fe_evtchn_event -> xen_pv_evtchn_event
>  * use libxenevtchn stable API instead of xc_* calls:
>     - xc_evtchn_fd -> xenevtchn_fd
>     - xc_evtchn_close -> xenevtchn_close
>     - xc_evtchn_bind_unbound_port -> xenevtchn_bind_unbound_port
> ---
>  hw/xen/xen_frontend.c         | 308 
> ++++++++++++++++++++++++++++++++++++++++++
>  hw/xen/xen_pvdev.c            |  15 ++
>  include/hw/xen/xen_frontend.h |   6 +
>  include/hw/xen/xen_pvdev.h    |   1 +
>  4 files changed, 330 insertions(+)
> 
> diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c
> index 6b92cf1..7b305ce 100644
> --- a/hw/xen/xen_frontend.c
> +++ b/hw/xen/xen_frontend.c
> @@ -3,6 +3,10 @@
>   *
>   *  (c) 2008 Gerd Hoffmann <address@hidden>
>   *
> + *  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
> @@ -23,6 +27,8 @@
>  #include "hw/xen/xen_frontend.h"
>  #include "hw/xen/xen_backend.h"
>  
> +static int debug = 0;
> +
>  char *xenstore_read_fe_str(struct XenDevice *xendev, const char *node)
>  {
>      return xenstore_read_str(xendev->fe, node);
> @@ -86,3 +92,305 @@ void xenstore_update_fe(char *watch, struct XenDevice 
> *xendev)
>      xen_fe_frontend_changed(xendev, node);
>      xen_be_check_state(xendev);
>  }
> +
> +struct XenDevice *xen_fe_get_xendev(const char *type, int dom, int dev,
> +                                    char *backend, struct XenDevOps *ops)

This function looks similare to xen_be_get_xendev(), could they be
shared?

In any case, there is a few issue with this implementation.

> +{
> +    struct XenDevice *xendev;
> +
> +    xendev = xen_pv_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)) {

Here, xendev is leaked.

> +        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 = xenevtchn_open(NULL, 0);
> +    if (xendev->evtchndev == NULL) {
> +        xen_pv_printf(NULL, 0, "can't open evtchn device\n");
> +        g_free(xendev);

We could use goto here, so there would be only one cleanup path.

> +        return NULL;
> +    }
> +    fcntl(xenevtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC);
> +
> +    if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) {
> +        xendev->gnttabdev = xengnttab_open(NULL, 0);
> +        if (xendev->gnttabdev == NULL) {
> +            xen_pv_printf(NULL, 0, "can't open gnttab device\n");
> +            xenevtchn_close(xendev->evtchndev);
> +            g_free(xendev);

goto, same as before.

> +            return NULL;
> +        }
> +    } else {
> +        xendev->gnttabdev = NULL;
> +    }
> +
> +    xen_pv_insert_xendev(xendev);
> +
> +    if (xendev->ops->alloc) {
> +        xendev->ops->alloc(xendev);
> +    }
> +
> +    return xendev;
> +}
> +
> +int xen_fe_alloc_unbound(struct XenDevice *xendev, int dom, int remote_dom){

'{' should be on a new line.

> +    xendev->local_port = xenevtchn_bind_unbound_port(xendev->evtchndev,
> +                                                     remote_dom);
> +    if (xendev->local_port == -1) {
> +        xen_pv_printf(xendev, 0, "xenevtchn_bind_unbound_port failed\n");
> +        return -1;
> +    }
> +    xen_pv_printf(xendev, 2, "bind evtchn port %d\n", xendev->local_port);
> +    qemu_set_fd_handler(xenevtchn_fd(xendev->evtchndev),
> +                        xen_pv_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)

There is a function that do a similar job for backends,
xen_be_set_state(). I think we could rename this function
xen_fe_set_state. Also "xbus" is a confusing name, "state" would be
fine.

> +{
> +    xs_transaction_t xbt = XBT_NULL;
> +
> +    if (xendev->fe_state == xbus) {
> +        return 0;
> +    }
> +
> +    xendev->fe_state = xbus;
> +    if (xendev->fe == NULL) {
> +        xen_pv_printf(NULL, 0, "xendev->fe is NULL\n");
> +        return -1;
> +    }
> +
> +retry_transaction:
> +    xbt = xs_transaction_start(xenstore);
> +    if (xbt == XBT_NULL) {
> +        goto abort_transaction;
> +    }

There is a transaction started, but I don't think it is used by the
function below. Could you remove the 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;
> +}
> +
> +/*
> + * Simplify QEMU side, a thread is running in Xen backend, which will
> + * connect frontend when the frontend is initialised. Call these initialised
> + * functions.
> + */

This comment does not make much sense to me.

> +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_pv_printf(xendev, 0, "initialise() failed\n");
> +        return rc;
> +    }
> +
> +    xenbus_switch_state(xendev, XenbusStateInitialised);
> +    return 0;
> +}
> +
> +static void xen_fe_try_connected(struct XenDevice *xendev)

This function looks exactly the same as xen_be_try_connected, should not
it be different and check the status of the backend?

> +{
> +    if (!xendev->ops->connected) {
> +        return;
> +    }
> +
> +    if (xendev->fe_state != XenbusStateConnected) {
> +        if (xendev->ops->flags & DEVOPS_FLAG_IGNORE_STATE) {
> +            xen_pv_printf(xendev, 2, "frontend not ready, ignoring\n");
> +        } else {
> +            xen_pv_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)

What is this function checking? The name of the function is not very
helpfull.

> +{
> +    int rc = 0;
> +
> +    rc = xen_fe_try_initialise(xendev);
> +    if (rc != 0) {
> +        xen_pv_printf(xendev, 0, "xendev %s initialise error\n",
> +                      xendev->name);
> +        goto err;
> +    }
> +    xen_fe_try_connected(xendev);
> +
> +    return rc;
> +
> +err:
> +    xen_pv_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);

The path backend of the backend would normally be located in:
device/$type/$devid/backend

Could not you use that instead of a domain name?

> +    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;
> +    int xenstore_dev;
> +
> +    /* ops .init check, xendev is NOT initialized */
> +    rc = xen_fe_try_init(ops);
> +    if (rc != 0) {
> +        return -1;
> +    }
> +
> +    /* Get /local/domain/0/${type}/{} directory */

This comment does not reflect what is done by the next line, also what
is "{}"?

> +    snprintf(path, sizeof(path), "frontend/%s", type);

I have not seen "frontend" used anywhere else, frontends are usully
within "device", like "device/vbd/$DEVID/*" for a block device
frontend. Can this be change?

> +    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_pv_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_pv_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_pv_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);
> +}


Thanks,

-- 
Anthony PERARD