From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v8 3/7] Qemu-Xen-vTPM: Xen frontend driver infrastructure Date: Mon, 22 Jun 2015 18:40:43 +0100 Message-ID: References: <1431840030-2797-1-git-send-email-quan.xu@intel.com> <1431840030-2797-5-git-send-email-quan.xu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1431840030-2797-5-git-send-email-quan.xu@intel.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Quan Xu Cc: wei.liu2@citrix.com, stefanb@linux.vnet.ibm.com, stefano.stabellini@eu.citrix.com, qemu-devel@nongnu.org, xen-devel@lists.xen.org, dgdegra@tycho.nsa.gov, eblake@redhat.com List-Id: xen-devel@lists.xenproject.org On Sun, 17 May 2015, Quan Xu 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 > --- > hw/xen/Makefile.objs | 2 +- > hw/xen/xen_frontend.c | 345 +++++++++++++++++++++++++++++++++++++++++++ > include/hw/xen/xen_backend.h | 8 + > 3 files changed, 354 insertions(+), 1 deletion(-) > create mode 100644 hw/xen/xen_frontend.c > > diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs > index 9ac9f7c..95eb9d0 100644 > --- a/hw/xen/Makefile.objs > +++ b/hw/xen/Makefile.objs > @@ -1,5 +1,5 @@ > # xen backend driver support > -common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o xen_pvdev.o > +common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o xen_frontend.o xen_pvdev.o > > obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen-host-pci-device.o > obj-$(CONFIG_XEN_PCI_PASSTHROUGH) += xen_pt.o xen_pt_config_init.o xen_pt_msi.o > diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c > new file mode 100644 > index 0000000..55af45a > --- /dev/null > +++ b/hw/xen/xen_frontend.c > @@ -0,0 +1,345 @@ > +/* > + * Xen frontend driver infrastructure > + * > + * Copyright (c) 2015 Intel Corporation > + * Authors: > + * Quan Xu > + * > + * 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 > + * version 2 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, see > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "hw/hw.h" > +#include "sysemu/char.h" > +#include "qemu/log.h" > +#include "hw/xen/xen_backend.h" > +#include > + > +int xenstore_dev; > + > +/* private */ > +static int debug; > + > +static void xen_fe_evtchn_event(void *opaque) > +{ > + struct XenDevice *xendev = opaque; > + evtchn_port_t port; > + > + port = xc_evtchn_pending(xendev->evtchndev); > + if (port != xendev->local_port) { > + return; > + } > + xc_evtchn_unmask(xendev->evtchndev, port); > + > + if (xendev->ops->event) { > + xendev->ops->event(xendev); > + } > +} This looks very similar to xen_be_evtchn_event. I think we should share the same function between frontends and backends. > +/* ------------------------------------------------------------- */ > + > +int xen_fe_alloc_unbound(struct XenDevice *xendev, int dom, int remote_dom) > +{ > + xendev->local_port = xc_evtchn_bind_unbound_port(xendev->evtchndev, > + remote_dom); > + if (xendev->local_port == -1) { > + xen_fe_printf(xendev, 0, "xc_evtchn_alloc_unbound failed\n"); > + return -1; > + } > + xen_fe_printf(xendev, 2, "bind evtchn port %d\n", xendev->local_port); > + qemu_set_fd_handler(xc_evtchn_fd(xendev->evtchndev), > + xen_fe_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_fe_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; > +} Given that this is xen_frontend.c, shouldn't this function be called xenbus_fe_switch_state? Also why is it so different from xen_be_set_state? I think it should be the same except for: xenstore_write_fe_int(xendev->fe, "state", state); > +/* > + * 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_fe_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_fe_printf(xendev, 2, "frontend not ready, ignoring\n"); > + } else { > + xen_fe_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_fe_printf(xendev, 0, "xendev %s initialise error\n", > + xendev->name); > + goto err; > + } > + xen_fe_try_connected(xendev); > + > + return rc; > + > +err: > + xen_del_xendev(domid, handle); > + return -1; > +} This should be called xen_fe_try_initialise > +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; > + } This is an uncommon xenstore protocol. I don't recognize "frontend" and "domain". Usually: /local/domain/$DOMID_BACKEND/backend/$TYPE/$DOMID_FRONTEND/$VDEV/frontend contains: /local/domain/$DOMID_FRONTEND/device/$TYPE/$VDEV Vice versa: /local/domain/$FRONTEND_DOMID/device/$TYPE/$VDEV/backend contains: /local/domain/$BACKEND_DOMID/backend/$TYPE/$DOMID_FRONTEND/$VDEV Am I getting it wrong? Is there a reason to diverge from this? > + 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; > + > + /* ops .init check, xendev is NOT initialized */ > + rc = xen_fe_try_init(ops); > + if (rc != 0) { > + return -1; > + } The init function should only be called when the corresponding xenstore entries are present. Here it looks like you are initializing the frontend unconditionally, even if /local/domain/0/device/$TYPE is missing. > + /* Get /local/domain/0/${type}/{} directory */ > + snprintf(path, sizeof(path), "frontend/%s", type); Why "frontend"? The string for frontends is usually "device". > + dev = xs_directory(xenstore, 0, path, &cdev); > + if (dev == NULL) { > + return 0; > + } It might actually be easier to scan /local/domain/$DOMID/backend instead of /local/domain/0/$TYPE, as I expect it to contain only one entry. Of course you would still need to double check that /local/domain/$DOMID_BACKEND/backend/$TYPE/$DOMID_FRONTEND contains the domain id of the domain QEMU is running on. > + 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_fe_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_fe_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_fe_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); > +} > + > +/* > + * msg_level: > + * 0 == errors (stderr + logfile). > + * 1 == informative debug messages (logfile only). > + * 2 == noisy debug messages (logfile only). > + * 3 == will flood your log (logfile only). > + */ > +void xen_fe_printf(struct XenDevice *xendev, int msg_level, > + const char *fmt, ...) > +{ > + va_list args; > + > + if (xendev) { > + if (msg_level > xendev->debug) { > + return; > + } > + qemu_log("xen fe: %s: ", xendev->name); > + if (msg_level == 0) { > + fprintf(stderr, "xen fe: %s: ", xendev->name); > + } > + } else { > + if (msg_level > debug) { > + return; > + } > + qemu_log("xen fe core: "); > + if (msg_level == 0) { > + fprintf(stderr, "xen fe core: "); > + } > + } > + va_start(args, fmt); > + qemu_log_vprintf(fmt, args); > + va_end(args); > + if (msg_level == 0) { > + va_start(args, fmt); > + vfprintf(stderr, fmt, args); > + va_end(args); > + } > + qemu_log_flush(); > +} As I wrote before, I would like to avoid having two separe xen printf functions. Please share the same function between frontends and backends.