diff mbox

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

Message ID 1468151270-12984-16-git-send-email-emilcondrea@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Emil Condrea July 10, 2016, 11:47 a.m. UTC
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(+)

Comments

Anthony PERARD July 25, 2016, 4:01 p.m. UTC | #1
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,
Emil Condrea Aug. 7, 2016, 11:39 a.m. UTC | #2
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
Xuquan (Euler) Aug. 9, 2016, 11:40 a.m. UTC | #3
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;

> > > +}
diff mbox

Patch

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)
+{
+    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)) {
+        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);
+        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);
+            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){
+    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)
+{
+    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;
+    }
+
+    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.
+ */
+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)
+{
+    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)
+{
+    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);
+    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 */
+    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_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);
+}
diff --git a/hw/xen/xen_pvdev.c b/hw/xen/xen_pvdev.c
index 394ddc1..cb51e87 100644
--- a/hw/xen/xen_pvdev.c
+++ b/hw/xen/xen_pvdev.c
@@ -121,6 +121,21 @@  cleanup:
     free(vec);
 }
 
+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;
+}
+
 const char *xenbus_strstate(enum xenbus_state state)
 {
     static const char *const name[] = {
diff --git a/include/hw/xen/xen_frontend.h b/include/hw/xen/xen_frontend.h
index 5d03f4e..8fc0422 100644
--- a/include/hw/xen/xen_frontend.h
+++ b/include/hw/xen/xen_frontend.h
@@ -8,6 +8,12 @@  int xenstore_read_fe_int(struct XenDevice *xendev, const char *node, int *ival);
 int xenstore_read_fe_uint64(struct XenDevice *xendev, const char *node, uint64_t *uval);
 void xenstore_update_fe(char *watch, struct XenDevice *xendev);
 
+struct XenDevice *xen_fe_get_xendev(const char *type, int dom, int dev,
+                                    char *backend, struct XenDevOps *ops);
 void xen_fe_frontend_changed(struct XenDevice *xendev, const char *node);
+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);
+
 
 #endif /* QEMU_HW_XEN_FRONTEND_H */
diff --git a/include/hw/xen/xen_pvdev.h b/include/hw/xen/xen_pvdev.h
index c985a9d..11bab56 100644
--- a/include/hw/xen/xen_pvdev.h
+++ b/include/hw/xen/xen_pvdev.h
@@ -65,6 +65,7 @@  char *xenstore_read_str(const char *base, const char *node);
 int xenstore_read_int(const char *base, const char *node, int *ival);
 int xenstore_read_uint64(const char *base, const char *node, uint64_t *uval);
 void xenstore_update(void *unused);
+char *xenstore_get_domain_name(uint32_t domid);
 
 const char *xenbus_strstate(enum xenbus_state state);