|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 15/19] Qemu-Xen-vTPM: Xen frontend driver infrastructure
On Mon, Jul 25, 2016 at 7:01 PM, Anthony PERARD
<anthony.perard@xxxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Emil Condrea <emilcondrea@xxxxxxxxx>
> >
> > ---
> > 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@xxxxxxxxxx>
> > *
> > + * Copyright (c) 2015 Intel Corporation
> > + * Authors:
> > + * Quan Xu <quan.xu@xxxxxxxxx>
> > + *
> > * 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |