From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xu, Quan" Subject: Re: [v3 2/5] Qemu-Xen-vTPM: Xen frontend driver infrastructure Date: Tue, 20 Jan 2015 02:47:10 +0000 Message-ID: <945CA011AD5F084CBEA3E851C0AB28890E869FE9__15427.6665569798$1421722215$gmane$org@SHSMSX101.ccr.corp.intel.com> References: <1419980582-18665-1-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: Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: "qemu-devel@nongnu.org" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com] > Sent: Tuesday, January 20, 2015 1:15 AM > To: Xu, Quan > Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org; > stefano.stabellini@eu.citrix.com > Subject: Re: [v3 2/5] Qemu-Xen-vTPM: Xen frontend driver infrastructure > > On Tue, 30 Dec 2014, 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. > > > > --Changes in v3: > > -New xen_frontend.c file > > -Move xenbus_switch_state() to xen_frontend.c -Move xen_stubdom_be() > > to xenstore_fe_read_be_str() -Move *_stubdom_*() to *_fe_*() > > > > Signed-off-by: Quan Xu > > --- > > hw/xen/Makefile.objs | 2 +- > > hw/xen/xen_backend.c | 11 +- > > hw/xen/xen_frontend.c | 323 > +++++++++++++++++++++++++++++++++++++++++++ > > include/hw/xen/xen_backend.h | 14 ++ > > 4 files changed, 348 insertions(+), 2 deletions(-) create mode > > 100644 hw/xen/xen_frontend.c > > > > diff --git a/hw/xen/Makefile.objs b/hw/xen/Makefile.objs index > > a0ca0aa..b0bb065 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 > > +common-obj-$(CONFIG_XEN_BACKEND) += xen_backend.o xen_devconfig.o > > +xen_frontend.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_backend.c b/hw/xen/xen_backend.c > > index b2cb22b..ad6e324 100644 > > --- a/hw/xen/xen_backend.c > > +++ b/hw/xen/xen_backend.c > > @@ -275,7 +275,7 @@ static struct XenDevice *xen_be_get_xendev(const > > char *type, int dom, int dev, > > /* > > * release xen backend device. > > */ > > -static struct XenDevice *xen_be_del_xendev(int dom, int dev) > > +struct XenDevice *xen_be_del_xendev(int dom, int dev) > > { > > struct XenDevice *xendev, *xnext; > > > > @@ -681,6 +681,10 @@ static void xenstore_update(void *unused) > > if (sscanf(vec[XS_WATCH_TOKEN], "fe:%" PRIxPTR, &ptr) == 1) { > > xenstore_update_fe(vec[XS_WATCH_PATH], (void*)ptr); > > } > > + if (sscanf(vec[XS_WATCH_TOKEN], "stub:%" PRIxPTR ":%d:%" PRIxPTR, > > + &type, &dom, &ops) == 3) { > > + xenstore_fe_update(vec[XS_WATCH_PATH], (void *)type, dom, > (void *)ops); > > + } > > Why "stub:"? Where is it coming from? 'stub:' is a head of token for xs_watch. The better head is 'fe:', but it is used by backend. So I call it 'stub:', which refers to 'Xen stubdomain backend'. 'stub:' is initialized in xenstore_fe_scan(). > > I think that the xenstore_update function should be moved to a new file: > we don't want xen_backend.c to be handling any frontend updates. > Maybe you could create a new file, hw/xen/xen_pvdev.c? > Make sense. > > > cleanup: > > free(vec); > > @@ -808,3 +812,8 @@ void xen_be_printf(struct XenDevice *xendev, int > msg_level, const char *fmt, ... > > } > > qemu_log_flush(); > > } > > + > > +void xen_qtail_insert_xendev(struct XenDevice *xendev) { > > + QTAILQ_INSERT_TAIL(&xendevs, xendev, next); } > > You should either move the xendevs queue to the new file xen_pvdev.c or you > should introduce a separate xendevs queue for frontends in xen_frontend.c. > > > > diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c new file > > mode 100644 index 0000000..07ffc5c > > --- /dev/null > > +++ b/hw/xen/xen_frontend.c > > @@ -0,0 +1,323 @@ > > +/* > > + * Xen frontend driver infrastructure > > + * > > + * Copyright (c) 2014 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 > > + > > +/* private */ > > +static int debug; > > + > > +/* ------------------------------------------------------------- */ > > +/*Get backend with fe type|domid, try to write the backend-id when > > +*create virtual machine. > > + * > > + *[XenStore] > > + * > > + *Dom.ID = "" > > + * device > > + * vtpm = "" > > + * 0 = "" > > + * backend-id = "ID" > > + *[..] > > + */ > > + > > +char *xenstore_fe_read_be_str(const char *type, int dom, int dev) { > > + char *val, *domu; > > + char path[XEN_BUFSIZE]; > > + unsigned int len, ival; > > + > > + /*fe path*/ > > + domu = xs_get_domain_path(xenstore, dom); > > + snprintf(path, sizeof(path), "%s/device/%s/%d/backend-id", > > + domu, type, dev); > > + g_free(domu); > > + > > + val = xs_read(xenstore, 0, path, &len); > > + if (!val || 1 != sscanf(val, "%d", &ival)) { > > + g_free(val); > > + return NULL; > > + } > > + g_free(val); > > + > > + /*be path*/ > > + domu = xs_get_domain_path(xenstore, ival); > > + > > + return domu; > > +} > > + > > +/*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_be_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; > > +} > > + > > +void xenstore_fe_update(char *watch, char *type, int dom, struct > > +XenDevOps *ops) { > > + struct XenDevice *xendev; > > + char path[XEN_BUFSIZE]; > > + char *ptr, *bepath; > > + unsigned int len, dev; > > + > > + if (!(ops->flags & DEVOPS_FLAG_FE)) { > > + return; > > + } > > + > > + len = snprintf(path, sizeof(path), "backend/%s/%d", type, dom); > > + ptr = strstr(watch, path); > > + if (ptr == NULL) { > > + return; > > + } > > + > > + if (sscanf(ptr+len, "/%u/%255s", &dev, path) != 2) { > > + strcpy(path, ""); > > + if (sscanf(ptr+len, "/%u", &dev) != 1) { > > + dev = -1; > > + } > > + } > > + > > + if (dev == -1) { > > + return; > > + } > > + > > + xendev = xen_fe_get_xendev(type, dom, dev, ops); > > + if (xendev != NULL) { > > + bepath = xs_read(xenstore, 0, xendev->be, &len); > > + /*bepath is NULL, indicates that the backend is not running, > > + *delete it. > > + */ > > + if (bepath == NULL) { > > + xen_be_del_xendev(dom, dev); > > + } else { > > + free(bepath); > > + if (xendev->ops->backend_changed) { > > + xendev->ops->backend_changed(xendev, path); > > + } > > + } > > + } > > +} > > + > > +/* > > + * get xen fe device, allocate a new one if it doesn't exist. > > + */ > > +struct XenDevice *xen_fe_get_xendev(const char *type, int dom, int dev, > > + struct XenDevOps *ops) { > > + struct XenDevice *xendev; > > + char *stub; > > + > > + xendev = xen_be_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; > > + } > > + > > + stub = xenstore_fe_read_be_str(xendev->type, xendev->dom, > xendev->dev); > > + snprintf(xendev->be, sizeof(xendev->be), "%s/backend/%s/%d/%d", > > + stub, xendev->type, xendev->dom, xendev->dev); > > + g_free(stub); > > + snprintf(xendev->name, sizeof(xendev->name), "%s-%d", > > + xendev->type, xendev->dev); > > + > > + xendev->debug = debug; > > + xendev->local_port = -1; > > + > > + xendev->evtchndev = xen_xc_evtchn_open(NULL, 0); > > + if (xendev->evtchndev == XC_HANDLER_INITIAL_VALUE) { > > + xen_be_printf(NULL, 0, "can't open evtchn device\n"); > > + g_free(xendev); > > + return NULL; > > + } > > + fcntl(xc_evtchn_fd(xendev->evtchndev), F_SETFD, FD_CLOEXEC); > > + > > + if (ops->flags & DEVOPS_FLAG_NEED_GNTDEV) { > > + xendev->gnttabdev = xen_xc_gnttab_open(NULL, 0); > > + if (xendev->gnttabdev == XC_HANDLER_INITIAL_VALUE) { > > + xen_be_printf(NULL, 0, "can't open gnttab device\n"); > > + xc_evtchn_close(xendev->evtchndev); > > + g_free(xendev); > > + return NULL; > > + } > > + } else { > > + xendev->gnttabdev = XC_HANDLER_INITIAL_VALUE; > > + } > > + > > + xen_qtail_insert_xendev(xendev); > > + > > + if (xendev->ops->alloc) { > > + xendev->ops->alloc(xendev); > > + } > > + > > + return xendev; > > +} > > + > > +/* 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_check(struct XenDevice *xendev, uint32_t domid, > > + int handle) > > +{ > > + int rc = 0; > > + > > + if (xendev->ops->init) { > > + rc = xendev->ops->init(xendev); > > + } > > + > > + if (rc != 0) { > > + xen_be_printf(xendev, 0, "xendev %s init error\n", > > + xendev->name); > > + goto err; > > + } > > + > > + if (xendev->ops->initialise) { > > + rc = xendev->ops->initialise(xendev); > > + } > > + > > + if (rc != 0) { > > + xen_be_printf(xendev, 0, "xendev %s initialise error\n", > > + xendev->name); > > + goto err; > > + } > > Shouldn't you be checking the backend status before setting the frontend to > connected? Something like xen_be_try_connected? > Yes, I will do it in v4. -Quan > > > + if (xendev->ops->connected) { > > + xendev->ops->connected(xendev); > > + } > > + > > + return rc; > > + > > +err: > > + xen_be_del_xendev(domid, handle); > > + return -1; > > +} > > + > > +static int xenstore_fe_scan(const char *type, uint32_t domid, > > + struct XenDevOps *ops) { > > + struct XenDevice *xendev; > > + char path[XEN_BUFSIZE], token[XEN_BUFSIZE]; > > + char *domu; > > + unsigned int cdev, j; > > + char **dev = NULL; > > + > > + /*Xen frontend : /local/domain/ID */ > > + domu = xs_get_domain_path(xenstore, domid); > > + snprintf(path, sizeof(path), "%s/device/%s", > > + domu, type); > > + free(domu); > > + dev = xs_directory(xenstore, 0, path, &cdev); > > + if (dev == NULL) { > > + return 0; > > + } > > + > > + for (j = 0; j < cdev; j++) { > > + xendev = xen_fe_get_xendev(type, domid, atoi(dev[j]), ops); > > + if (xendev == NULL) { > > + xen_be_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, atoi(dev[j])) < 0) { > > + xen_be_printf(xendev, 0, "xendev fe_check error.\n"); > > + continue; > > + } > > + > > + /*setup watch*/ > > + snprintf(token, sizeof(token), "stub:%p:%d:%p", > > + type, domid, xendev->ops); > > + if (!xs_watch(xenstore, xendev->be, token)) { > > + xen_be_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/include/hw/xen/xen_backend.h > > b/include/hw/xen/xen_backend.h index 3b4125e..06e202f 100644 > > --- a/include/hw/xen/xen_backend.h > > +++ b/include/hw/xen/xen_backend.h > > @@ -15,6 +15,8 @@ struct XenDevice; > > #define DEVOPS_FLAG_NEED_GNTDEV 1 > > /* don't expect frontend doing correct state transitions (aka console > > quirk) */ #define DEVOPS_FLAG_IGNORE_STATE 2 > > +/*dev is frontend device*/ > > +#define DEVOPS_FLAG_FE 4 > > > > struct XenDevOps { > > size_t size; > > @@ -80,6 +82,8 @@ int xenstore_read_fe_uint64(struct XenDevice > > *xendev, const char *node, uint64_t const char *xenbus_strstate(enum > > xenbus_state state); struct XenDevice *xen_be_find_xendev(const char > > *type, int dom, int dev); void xen_be_check_state(struct XenDevice > > *xendev); > > +struct XenDevice *xen_be_del_xendev(int dom, int dev); void > > +xen_qtail_insert_xendev(struct XenDevice *xendev); > > > > /* xen backend driver bits */ > > int xen_be_init(void); > > @@ -91,6 +95,16 @@ int xen_be_send_notify(struct XenDevice *xendev); > > void xen_be_printf(struct XenDevice *xendev, int msg_level, const char > *fmt, ...) > > GCC_FMT_ATTR(3, 4); > > > > +/*Xen frontend driver*/ > > +int xen_fe_register(const char *type, struct XenDevOps *ops); int > > +xen_be_alloc_unbound(struct XenDevice *xendev, int dom, int > > +remote_dom); char *xenstore_fe_read_be_str(const char *type, int dom, > > +int dev); int xenbus_switch_state(struct XenDevice *xendev, enum > > +xenbus_state xbus); struct XenDevice *xen_fe_get_xendev(const char *type, > int dom, int dev, > > + struct XenDevOps *ops); void > > +xenstore_fe_update(char *watch, char *type, int dom, > > + struct XenDevOps *ops); > > + > > /* actual backend drivers */ > > extern struct XenDevOps xen_console_ops; /* xen_console.c */ > > extern struct XenDevOps xen_kbdmouse_ops; /* xen_framebuffer.c */ > > -- > > 1.8.3.2 > >