From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49274) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYJCf-00020I-Sr for qemu-devel@nongnu.org; Wed, 18 Mar 2015 15:00:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YYJCa-0001tv-5r for qemu-devel@nongnu.org; Wed, 18 Mar 2015 15:00:29 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:44744) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYJCa-0001s1-0T for qemu-devel@nongnu.org; Wed, 18 Mar 2015 15:00:24 -0400 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 18 Mar 2015 15:00:22 -0400 Received: from b01cxnp22036.gho.pok.ibm.com (b01cxnp22036.gho.pok.ibm.com [9.57.198.26]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id 41B8F6E803C for ; Wed, 18 Mar 2015 14:52:10 -0400 (EDT) Received: from d01av05.pok.ibm.com (d01av05.pok.ibm.com [9.56.224.195]) by b01cxnp22036.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t2IJ0Bp426607820 for ; Wed, 18 Mar 2015 19:00:19 GMT Received: from d01av05.pok.ibm.com (localhost [127.0.0.1]) by d01av05.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t2IIxktJ023273 for ; Wed, 18 Mar 2015 14:59:47 -0400 Message-ID: <5509CB09.1030405@linux.vnet.ibm.com> Date: Wed, 18 Mar 2015 14:59:21 -0400 From: Stefan Berger MIME-Version: 1.0 References: <1425989673-2812-1-git-send-email-quan.xu@intel.com> <1425989673-2812-4-git-send-email-quan.xu@intel.com> In-Reply-To: <1425989673-2812-4-git-send-email-quan.xu@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 3/5] Qemu-Xen-vTPM: Register Xen stubdom vTPM frontend driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Quan Xu , stefano.stabellini@eu.citrix.com, qemu-devel@nongnu.org, armbru@redhat.com, lcapitulino@redhat.com, aliguori@amazon.com, pbonzini@redhat.com, eblake@redhat.com, kraxel@redhat.com, meyering@redhat.com, mjt@tls.msk.ru, sw@weilnetz.de, wei.liu2@citrix.com Cc: xen-devel@lists.xen.org On 03/10/2015 08:14 AM, Quan Xu wrote: > This drvier transfers any request/repond between TPM xenstubdoms > driver and Xen vTPM stubdom, and facilitates communications between > Xen vTPM stubdom domain and vTPM xenstubdoms driver. It is a glue for > the TPM xenstubdoms driver and Xen stubdom vTPM domain that provides > the actual TPM functionality. > > (Xen) Xen backend driver should run before running this frontend, and > initialize XenStore as the following for communication. > > [XenStore] > > for example: > > Domain 0: runs QEMU for guest A > Domain 1: vtpmmgr > Domain 2: vTPM for guest A > Domain 3: HVM guest A > > [...] > local = "" > domain = "" > 0 = "" > frontend = "" > vtpm = "" > 2 = "" > 0 = "" > backend = "/local/domain/2/backend/vtpm/0/0" > backend-id = "2" > state = "*" > handle = "0" > domain = "Domain3's name" > ring-ref = "*" > event-channel = "*" > feature-protocol-v2 = "1" > backend = "" > qdisk = "" > [...] > console = "" > vif = "" > [...] > 2 = "" > [...] > backend = "" > vtpm = "" > 0 = "" > 0 = "" > frontend = "/local/domain/0/frontend/vtpm/2/0" > frontend-id = "0" ('0', frontend is running in Domain-0) > [...] > 3 = "" > [...] > device = "" (frontend device, the backend is running in QEMU/.etc) > vkbd = "" > [...] > vif = "" > [...] > > .. > > (QEMU) xen_vtpmdev_ops is initialized with the following process: > xen_hvm_init() > [...] > -->xen_fe_register("vtpm", ...) > -->xenstore_fe_scan() > -->xen_fe_try_init(ops) > --> XenDevOps.init() > -->xen_fe_get_xendev() > --> XenDevOps.alloc() > -->xen_fe_check() > -->xen_fe_try_initialise() > --> XenDevOps.initialise() > -->xen_fe_try_connected() > --> XenDevOps.connected() > -->xs_watch() > [...] > > --Changes in v3: > -Move xen_stubdom_vtpm.c to xen_vtpm_frontend.c > -Read Xen vTPM status via XenStore > > --Changes in v4: > -Redesign vTPM xenstore architecture for HVM virtual machine. > -Remove unnecessary busy loop. > -Call xen_fe_register(vtpm ...) directly and move some initialzation > chunk in the xen_vtpmdev_ops .init function. > > Signed-off-by: Quan Xu > --- > hw/tpm/Makefile.objs | 1 + > hw/tpm/xen_vtpm_frontend.c | 278 +++++++++++++++++++++++++++++++++++++++++++ > hw/xen/xen_frontend.c | 20 ++++ > include/hw/xen/xen_backend.h | 5 + > include/hw/xen/xen_common.h | 6 + > xen-hvm.c | 5 + > 6 files changed, 315 insertions(+) > create mode 100644 hw/tpm/xen_vtpm_frontend.c > > diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs > index 99f5983..57919fa 100644 > --- a/hw/tpm/Makefile.objs > +++ b/hw/tpm/Makefile.objs > @@ -1,2 +1,3 @@ > common-obj-$(CONFIG_TPM_TIS) += tpm_tis.o > common-obj-$(CONFIG_TPM_PASSTHROUGH) += tpm_passthrough.o > +common-obj-$(CONFIG_TPM_XENSTUBDOMS) += xen_vtpm_frontend.o > diff --git a/hw/tpm/xen_vtpm_frontend.c b/hw/tpm/xen_vtpm_frontend.c > new file mode 100644 > index 0000000..4ef0a26 > --- /dev/null > +++ b/hw/tpm/xen_vtpm_frontend.c > @@ -0,0 +1,278 @@ > +/* > + * Connect to Xen vTPM stubdom domain > + * > + * 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 > +#include > +#include > +#include > + > +#include "hw/hw.h" > +#include "block/aio.h" > +#include "hw/xen/xen_backend.h" > + > +#define XS_STUBDOM_VTPM_ENABLE "1" > + > +enum tpmif_state { > + TPMIF_STATE_IDLE, /* no contents / vTPM idle / cancel complete */ > + TPMIF_STATE_SUBMIT, /* request ready / vTPM working */ > + TPMIF_STATE_FINISH, /* response ready / vTPM idle */ > + TPMIF_STATE_CANCEL, /* cancel requested / vTPM working */ > +}; > + > +static AioContext *vtpm_aio_ctx; > + > +enum status_bits { > + VTPM_STATUS_RUNNING = 0x1, > + VTPM_STATUS_IDLE = 0x2, > + VTPM_STATUS_RESULT = 0x4, > + VTPM_STATUS_CANCELED = 0x8, > +}; > + > +struct tpmif_shared_page { > + uint32_t length; /* request/response length in bytes */ > + > + uint8_t state; /* enum tpmif_state */ > + uint8_t locality; /* for the current request */ > + uint8_t pad; /* should be zero */ > + > + uint8_t nr_extra_pages; /* extra pages for long packets; may be zero */ > + uint32_t extra_pages[0]; /* grant IDs; length is actually nr_extra_pages */ Can you at least mention here that beyond the extra_pages entries is the actual request/response? > +}; > + > +struct xen_vtpm_dev { > + struct XenDevice xendev; /* must be first */ > + struct tpmif_shared_page *shr; > + xc_gntshr *xen_xcs; > + int ring_ref; > + int bedomid; > + QEMUBH *sr_bh; > +}; > + > +static uint8_t vtpm_status(struct xen_vtpm_dev *vtpmdev) > +{ > + switch (vtpmdev->shr->state) { > + case TPMIF_STATE_IDLE: > + case TPMIF_STATE_FINISH: > + return VTPM_STATUS_IDLE; > + case TPMIF_STATE_SUBMIT: > + case TPMIF_STATE_CANCEL: > + return VTPM_STATUS_RUNNING; > + default: > + return 0; > + } > +} > + > +static bool vtpm_aio_wait(AioContext *ctx) > +{ > + return aio_poll(ctx, true); > +} > + > +static void sr_bh_handler(void *opaque) > +{ > +} > + > +int vtpm_recv(struct XenDevice *xendev, uint8_t* buf, size_t *count) > +{ I think you should pass the size of the buf here as well. > + struct xen_vtpm_dev *vtpmdev = container_of(xendev, struct xen_vtpm_dev, > + xendev); > + struct tpmif_shared_page *shr = vtpmdev->shr; > + unsigned int offset; > + > + if (shr->state == TPMIF_STATE_IDLE) { > + return -ECANCELED; > + } > + > + offset = sizeof(*shr) + 4*shr->nr_extra_pages; Replace the '4' with sizeof(uint32_t), if this is what it stands for. Or better sizeof(shr->extra_pages[0]). > + memcpy(buf, offset + (uint8_t *)shr, shr->length); Buf is large enough? I think you should have the buf_size here and ensure you don't overwrite memory. > + *count = shr->length; > + > + return 0; > +} > + > +int vtpm_send(struct XenDevice *xendev, uint8_t* buf, size_t count) > +{ > + struct xen_vtpm_dev *vtpmdev = container_of(xendev, struct xen_vtpm_dev, > + xendev); > + struct tpmif_shared_page *shr = vtpmdev->shr; > + unsigned int offset = sizeof(*shr) + 4*shr->nr_extra_pages; Same comment here. > + > + while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) { > + vtpm_aio_wait(vtpm_aio_ctx); > + } > + > + memcpy(offset + (uint8_t *)shr, buf, count); Target is always large enough? Introduce a check. > + shr->length = count; > + barrier(); > + shr->state = TPMIF_STATE_SUBMIT; > + xen_wmb(); > + xen_be_send_notify(&vtpmdev->xendev); > + > + while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) { > + vtpm_aio_wait(vtpm_aio_ctx); > + } > + > + return count; > +} > + > +static int vtpm_initialise(struct XenDevice *xendev) > +{ > + struct xen_vtpm_dev *vtpmdev = container_of(xendev, struct xen_vtpm_dev, > + xendev); > + xs_transaction_t xbt = XBT_NULL; > + unsigned int ring_ref; > + > + vtpmdev->xendev.fe = xenstore_read_be_str(&vtpmdev->xendev, "frontend"); > + if (vtpmdev->xendev.fe == NULL) { > + return -1; > + } > + > + /* Get backend domid */ > + if (xenstore_read_fe_int(&vtpmdev->xendev, "backend-id", > + &vtpmdev->bedomid)) { > + return -1; > + } > + > + /* Alloc share page */ shared->shared > + vtpmdev->shr = xc_gntshr_share_pages(vtpmdev->xen_xcs, vtpmdev->bedomid, 1, > + &ring_ref, PROT_READ|PROT_WRITE); > + vtpmdev->ring_ref = ring_ref; > + if (vtpmdev->shr == NULL) { > + return -1; > + } > + > + /* Create event channel */ > + if (xen_fe_alloc_unbound(&vtpmdev->xendev, 0, vtpmdev->bedomid)) { > + xc_gntshr_munmap(vtpmdev->xen_xcs, vtpmdev->shr, 1); > + return -1; > + } > + > + xc_evtchn_unmask(vtpmdev->xendev.evtchndev, > + vtpmdev->xendev.local_port); > + > +again: > + xbt = xs_transaction_start(xenstore); > + if (xbt == XBT_NULL) { > + goto abort_transaction; > + } > + > + if (xenstore_write_int(vtpmdev->xendev.fe, "ring-ref", > + vtpmdev->ring_ref)) { > + goto abort_transaction; > + } > + > + if (xenstore_write_int(vtpmdev->xendev.fe, "event-channel", > + vtpmdev->xendev.local_port)) { > + goto abort_transaction; > + } > + > + /* Publish protocol v2 feature */ > + if (xenstore_write_int(vtpmdev->xendev.fe, "feature-protocol-v2", 1)) { > + goto abort_transaction; > + } > + > + if (!xs_transaction_end(xenstore, xbt, 0)) { > + if (errno == EAGAIN) { > + goto again; > + } > + } > + > + return 0; > + > +abort_transaction: > + xc_gntshr_munmap(vtpmdev->xen_xcs, vtpmdev->shr, 1); > + xs_transaction_end(xenstore, xbt, 1); > + return -1; > +} > + > +static int vtpm_free(struct XenDevice *xendev) > +{ > + struct xen_vtpm_dev *vtpmdev = container_of(xendev, struct xen_vtpm_dev, > + xendev); add empty line here > + aio_poll(vtpm_aio_ctx, false); > + qemu_bh_delete(vtpmdev->sr_bh); > + if (vtpmdev->shr) { > + xc_gntshr_munmap(vtpmdev->xen_xcs, vtpmdev->shr, 1); > + } > + xc_interface_close(vtpmdev->xen_xcs); > + return 0; > +} > + > +static int vtpm_init(struct XenDevice *xendev) > +{ > + char path[XEN_BUFSIZE]; > + char *value; > + unsigned int stubdom_vtpm = 0; > + > + snprintf(path, sizeof(path), "/local/domain/%d/platform/acpi_stubdom_vtpm", > + xen_domid); > + value = xs_read(xenstore, 0, path, &stubdom_vtpm); > + if (stubdom_vtpm <= 0 || strcmp(value, XS_STUBDOM_VTPM_ENABLE)) { > + free(value); > + return -1; > + } > + free(value); > + return 0; > +} > + > +static void vtpm_alloc(struct XenDevice *xendev) > +{ > + struct xen_vtpm_dev *vtpmdev = container_of(xendev, struct xen_vtpm_dev, > + xendev); > + > + vtpm_aio_ctx = aio_context_new(NULL); > + if (vtpm_aio_ctx == NULL) { > + return; > + } > + vtpmdev->sr_bh = aio_bh_new(vtpm_aio_ctx, sr_bh_handler, vtpmdev); > + qemu_bh_schedule(vtpmdev->sr_bh); > + vtpmdev->xen_xcs = xen_xc_gntshr_open(0, 0); > +} > + > +static void vtpm_event(struct XenDevice *xendev) > +{ > + struct xen_vtpm_dev *vtpmdev = container_of(xendev, struct xen_vtpm_dev, > + xendev); > + > + qemu_bh_schedule(vtpmdev->sr_bh); > +} > + > +struct XenDevOps xen_vtpmdev_ops = { > + .size = sizeof(struct xen_vtpm_dev), > + .flags = DEVOPS_FLAG_IGNORE_STATE | > + DEVOPS_FLAG_FE, > + .event = vtpm_event, > + .free = vtpm_free, > + .init = vtpm_init, > + .alloc = vtpm_alloc, > + .initialise = vtpm_initialise, > + .backend_changed = vtpm_backend_changed, > +}; > diff --git a/hw/xen/xen_frontend.c b/hw/xen/xen_frontend.c > index 55af45a..1ca7342 100644 > --- a/hw/xen/xen_frontend.c > +++ b/hw/xen/xen_frontend.c > @@ -60,6 +60,26 @@ static void xen_fe_evtchn_event(void *opaque) > > /* ------------------------------------------------------------- */ > > +void vtpm_backend_changed(struct XenDevice *xendev, const char *node) > +{ > + int be_state; > + > + if (strcmp(node, "state") == 0) { > + xenstore_read_be_int(xendev, node, &be_state); > + switch (be_state) { > + case XenbusStateConnected: > + /* TODO */ > + break; > + case XenbusStateClosing: > + case XenbusStateClosed: > + xenbus_switch_state(xendev, XenbusStateClosing); > + break; > + default: > + break; > + } > + } > +} > + > int xen_fe_alloc_unbound(struct XenDevice *xendev, int dom, int remote_dom) > { > xendev->local_port = xc_evtchn_bind_unbound_port(xendev->evtchndev, > diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h > index bb0b303..b959413 100644 > --- a/include/hw/xen/xen_backend.h > +++ b/include/hw/xen/xen_backend.h > @@ -110,6 +110,11 @@ 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); > > +/* Xen vtpm */ > +int vtpm_send(struct XenDevice *xendev, uint8_t* buf, size_t count); > +int vtpm_recv(struct XenDevice *xendev, uint8_t* buf, size_t *count); > +void vtpm_backend_changed(struct XenDevice *xendev, const char *node); > + > /* actual backend drivers */ > extern struct XenDevOps xen_console_ops; /* xen_console.c */ > extern struct XenDevOps xen_kbdmouse_ops; /* xen_framebuffer.c */ > diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h > index 07731b9..e81d230 100644 > --- a/include/hw/xen/xen_common.h > +++ b/include/hw/xen/xen_common.h > @@ -130,6 +130,12 @@ static inline XenXC xen_xc_interface_open(void *logger, void *dombuild_logger, > return xc_interface_open(logger, dombuild_logger, open_flags); > } > > +static inline xc_gntshr *xen_xc_gntshr_open(void *logger, > + unsigned int open_flags) > +{ > + return xc_gntshr_open(logger, open_flags); > +} > + > /* FIXME There is now way to have the xen fd */ > static inline int xc_fd(xc_interface *xen_xc) > { > diff --git a/xen-hvm.c b/xen-hvm.c > index 05e522c..e21dfee 100644 > --- a/xen-hvm.c > +++ b/xen-hvm.c > @@ -1071,6 +1071,11 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size, > fprintf(stderr, "%s: xen backend core setup failed\n", __FUNCTION__); > return -1; > } > + > +#ifdef CONFIG_TPM_XENSTUBDOMS > + xen_fe_register("vtpm", &xen_vtpmdev_ops); > +#endif > + > xen_be_register("console", &xen_console_ops); > xen_be_register("vkbd", &xen_kbdmouse_ops); > xen_be_register("qdisk", &xen_blkdev_ops); Stefan