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

[Emil Condrea <emilcondrea@gmail.com>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel