| Message ID | 1468151270-12984-16-git-send-email-emilcondrea@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > > > +}
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);