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...@intel.com>
> Signed-off-by: Emil Condrea <emilcond...@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 <kra...@redhat.com>
> *
> + * Copyright (c) 2015 Intel Corporation
> + * Authors:
> + * Quan Xu <quan...@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,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
