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

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 <quan.xu@intel.com>
Signed-off-by: Emil Condrea <emilcondrea@gmail.com>

---
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(+)

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 <quan.xu@intel.com>
> Signed-off-by: Emil Condrea <emilcondrea@gmail.com>
> 
> ---
> 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 <kraxel@redhat.com>
>   *
> + *  Copyright (c) 2015 Intel Corporation
> + *  Authors:
> + *    Quan Xu <quan.xu@intel.com>
> + *
>   * 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,

On Mon, Jul 25, 2016 at 7:01 PM, Anthony PERARD
<anthony.perard@citrix.com> wrote:
>
> 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 <quan.xu@intel.com>
> > Signed-off-by: Emil Condrea <emilcondrea@gmail.com>
> >
> > ---
> > 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 <kraxel@redhat.com>
> >   *
> > + *  Copyright (c) 2015 Intel Corporation
> > + *  Authors:
> > + *    Quan Xu <quan.xu@intel.com>
> > + *
> >   * 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.

There is some special initialization in each function but we can group
common behavior in xen_pv_get_xendev which will be called by
xen_be_get_xendev and xen_fe_get_xendev

>
> > +{
> > +    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.

Will fix in v10
>
>
> > +        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.

Will fix in v10
>
>
> > +        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.

Will fix in v10
>
>
> > +            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.

I will rename the function and the variable in v10
>
>
> > +{
> > +    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?

I will remove it. For current version I don't see a direct usage of
this transaction.
Quan, did you have a specific reason for past versions for this 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?

You are right, I will fix this in v10.

>
>
> > +{
> > +    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.

I will rename it to xen_fe_connect

>
>
> > +{
> > +    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?

Multiple domains might share one vTPM.

All frontends are under domain 0. For example, for 2 guests sharing a vTPM:

/local/domain/0/frontend/vtpm = ""
/local/domain/0/frontend/vtpm/41 = ""
/local/domain/0/frontend/vtpm/41/0 = ""
/local/domain/0/frontend/vtpm/41/0/backend = "/local/domain/41/backend/vtpm/0/0"
/local/domain/0/frontend/vtpm/41/0/backend-id = "41"
/local/domain/0/frontend/vtpm/41/0/state = "3"
/local/domain/0/frontend/vtpm/41/0/handle = "0"
/local/domain/0/frontend/vtpm/41/0/domain = "hvm-guest"
/local/domain/0/frontend/vtpm/41/0/ring-ref = "8"
/local/domain/0/frontend/vtpm/41/0/event-channel = "99"
/local/domain/0/frontend/vtpm/41/0/feature-protocol-v2 = "1"
/local/domain/0/frontend/vtpm/41/1 = ""
/local/domain/0/frontend/vtpm/41/1/backend = "/local/domain/41/backend/vtpm/0/1"
/local/domain/0/frontend/vtpm/41/1/backend-id = "41"
/local/domain/0/frontend/vtpm/41/1/state = "3"
/local/domain/0/frontend/vtpm/41/1/handle = "1"
/local/domain/0/frontend/vtpm/41/1/domain = "hvm-guest2"
/local/domain/0/frontend/vtpm/41/1/ring-ref = "9"
/local/domain/0/frontend/vtpm/41/1/event-channel = "104"
/local/domain/0/frontend/vtpm/41/1/feature-protocol-v2 = "1"

>
>
> > +    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 "{}"?

Will update the comment.
frontend/vtpm/ is a directory with all xen hvm vtpm frontends.
Eg. frontend/vtpm/[vtpm-domain-id]
>
>
> > +    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?

vTPM for pv architecture use /local/domain/0/device/ and it was
proposed that /local/domain/0/frontend/ would be used for hvm vTPM.

The design for this protocol was discussed last year here:
http://markmail.org/message/blpbzgyfzbpskwbf
>
>
> > +    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 for the review, Anthony.

>
> Thanks,
>
> --
> Anthony PERARD

On August 07, 2016 7:39 PM, Emil Condrea <emilcondrea@gmail.com> wrote:
On Mon, Jul 25, 2016 at 7:01 PM, Anthony PERARD <anthony.perard@citrix.com> wrote:
> >

> > > +{

> > > +    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:


Add a space here, then
   s/retry_transaction:/ retry_transaction:/
the same for the other cases..


> > > +    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?

>

> I will remove it. For current version I don't see a direct usage of this transaction.

> Quan, did you have a specific reason for past versions for this transaction?


No specific reason, maybe I copied these code from xen / libxl .. 
btw, why does libxl use ' retry_transaction: ' logic ... but QEMU doesn't ?


Thank you both.

Quan

> >

> >

> > > +    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;

> > > +}