[v3,3/5] Qemu-Xen-vTPM: Register Xen stubdom vTPM frontend driver

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]
 ..
  FE.DOMAIN.ID
   device = ""
    vtpm = ""
     0 = ""
      backend = "/local/domain/{BE.DOMAIN.ID}/backend/vtpm/{FE.DOMAIN.ID}/0"
      backend-id = "BE.DOMAIN.ID"
      state = "1"
      handle = "0"
 ..

(QEMU) xen_vtpmdev_ops is initialized with the following process:
  xen_hvm_init()
    [...]
    -->xen_fe_register("vtpm", ...)
      -->xenstore_fe_scan()
        -->xen_fe_get_xendev()
          --> XenDevOps.alloc()
        -->xen_fe_check()
          --> XenDevOps.init()
          --> XenDevOps.initialise()
          --> XenDevOps.connected()
        -->xs_watch()
    [...]

--Changes in v3:
-Move xen_stubdom_vtpm.c to xen_vtpm_frontend.c
-Read Xen vTPM status via XenStore

Signed-off-by: Quan Xu <quan.xu@intel.com>
---
 hw/tpm/Makefile.objs         |   1 +
 hw/tpm/xen_vtpm_frontend.c   | 264 +++++++++++++++++++++++++++++++++++++++++++
 hw/xen/xen_backend.c         |  34 ++++++
 include/hw/xen/xen_backend.h |   9 +-
 include/hw/xen/xen_common.h  |   6 +
 xen-hvm.c                    |  16 +++
 6 files changed, 328 insertions(+), 2 deletions(-)
 create mode 100644 hw/tpm/xen_vtpm_frontend.c

On Tue, 30 Dec 2014, 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]
>  ..
>   FE.DOMAIN.ID
>    device = ""
>     vtpm = ""
>      0 = ""
>       backend = "/local/domain/{BE.DOMAIN.ID}/backend/vtpm/{FE.DOMAIN.ID}/0"
>       backend-id = "BE.DOMAIN.ID"
>       state = "1"
>       handle = "0"
>  ..
> 
> (QEMU) xen_vtpmdev_ops is initialized with the following process:
>   xen_hvm_init()
>     [...]
>     -->xen_fe_register("vtpm", ...)
>       -->xenstore_fe_scan()
>         -->xen_fe_get_xendev()
>           --> XenDevOps.alloc()
>         -->xen_fe_check()
>           --> XenDevOps.init()
>           --> XenDevOps.initialise()
>           --> XenDevOps.connected()
>         -->xs_watch()
>     [...]
> 
> --Changes in v3:
> -Move xen_stubdom_vtpm.c to xen_vtpm_frontend.c
> -Read Xen vTPM status via XenStore
> 
> Signed-off-by: Quan Xu <quan.xu@intel.com>
> ---
>  hw/tpm/Makefile.objs         |   1 +
>  hw/tpm/xen_vtpm_frontend.c   | 264 +++++++++++++++++++++++++++++++++++++++++++
>  hw/xen/xen_backend.c         |  34 ++++++
>  include/hw/xen/xen_backend.h |   9 +-
>  include/hw/xen/xen_common.h  |   6 +
>  xen-hvm.c                    |  16 +++
>  6 files changed, 328 insertions(+), 2 deletions(-)
>  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..00cc888
> --- /dev/null
> +++ b/hw/tpm/xen_vtpm_frontend.c
> @@ -0,0 +1,264 @@
> +/*
> + * Connect to Xen vTPM stubdom domain
> + *
> + *  Copyright (c) 2014 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
> + * 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 <http://www.gnu.org/licenses/>
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <signal.h>
> +#include <inttypes.h>
> +#include <time.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <sys/ioctl.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/mman.h>
> +#include <sys/uio.h>
> +
> +#include "hw/hw.h"
> +#include "block/aio.h"
> +#include "hw/xen/xen_backend.h"
> +
> +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 */
> +};
> +
> +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)
> +{
> +    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;
> +    }
> +
> +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> +        vtpm_aio_wait(vtpm_aio_ctx);
> +    }
> +
> +    offset = sizeof(*shr) + 4*shr->nr_extra_pages;
> +    memcpy(buf, offset + (uint8_t *)shr, shr->length);
> +    *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;
> +
> +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> +        vtpm_aio_wait(vtpm_aio_ctx);
> +    }
> +
> +    memcpy(offset + (uint8_t *)shr, buf, count);
> +    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*/
> +    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_be_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;
> +        }
> +    }
> +    /* Tell vtpm backend that we are ready */
> +    xenbus_switch_state(&vtpmdev->xendev, XenbusStateInitialised);
> +
> +    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);
> +    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 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,
> +    .alloc            = vtpm_alloc,
> +    .initialise       = vtpm_initialise,
> +    .backend_changed  = vtpm_backend_changed,
> +};
> diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c
> index ad6e324..377a9c8 100644
> --- a/hw/xen/xen_backend.c
> +++ b/hw/xen/xen_backend.c
> @@ -741,6 +741,20 @@ int xen_be_register(const char *type, struct XenDevOps *ops)
>      return xenstore_scan(type, xen_domid, ops);
>  }
>  
> +int xen_be_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_be_printf(xendev, 0, "xc_evtchn_alloc_unbound failed\n");
> +        return -1;
> +    }
> +    xen_be_printf(xendev, 2, "bind evtchn port %d\n", xendev->local_port);
> +    qemu_set_fd_handler(xc_evtchn_fd(xendev->evtchndev),
> +                        xen_be_evtchn_event, NULL, xendev);
> +    return 0;
> +}

Why are you calling this xen_be_alloc_unbound? Shouldn't it be called
xen_fe_alloc_unbound and be in xen_frontend.c, given that is called only
by a frontend?


>  int xen_be_bind_evtchn(struct XenDevice *xendev)
>  {
>      if (xendev->local_port != -1) {
> @@ -813,6 +827,26 @@ void xen_be_printf(struct XenDevice *xendev, int msg_level, const char *fmt, ...
>      qemu_log_flush();
>  }
>  
> +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;
> +        }
> +    }
> +}

This should be written as a generic fe function in xen_frontend.c


>  void xen_qtail_insert_xendev(struct XenDevice *xendev)
>  {
>      QTAILQ_INSERT_TAIL(&xendevs, xendev, next);
> diff --git a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
> index 06e202f..d07ae36 100644
> --- a/include/hw/xen/xen_backend.h
> +++ b/include/hw/xen/xen_backend.h
> @@ -102,8 +102,12 @@ 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);
> +void xenstore_fe_update(char *watch, char *type, int dom, struct XenDevOps *ops);
> +
> +/*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     */
> @@ -111,6 +115,7 @@ extern struct XenDevOps xen_kbdmouse_ops;     /* xen_framebuffer.c */
>  extern struct XenDevOps xen_framebuffer_ops;  /* xen_framebuffer.c */
>  extern struct XenDevOps xen_blkdev_ops;       /* xen_disk.c        */
>  extern struct XenDevOps xen_netdev_ops;       /* xen_nic.c         */
> +extern struct XenDevOps xen_vtpmdev_ops;      /* xen_vtpm_frontend.c*/
>  
>  void xen_init_display(int domid);
>  
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 07731b9..2e9bb62 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..5b93cd4 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -986,6 +986,12 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size, ram_addr_t *above_4g_mem_size,
>      int i, rc;
>      unsigned long ioreq_pfn;
>      unsigned long bufioreq_evtchn;
> +
> +#ifdef CONFIG_TPM_XENSTUBDOMS
> +    char path[XEN_BUFSIZE];
> +    unsigned int stubdom_vtpm = 0;
> +#endif
> +
>      XenIOState *state;
>  
>      state = g_malloc0(sizeof (XenIOState));
> @@ -1071,6 +1077,16 @@ 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
> +    snprintf(path, sizeof(path), "/local/domain/%d/platform/acpi_stubdom_vtpm",
> +             xen_domid);
> +    xs_read(state->xenstore, 0, path, &stubdom_vtpm);
> +    if (stubdom_vtpm) {
> +        xen_fe_register("vtpm", &xen_vtpmdev_ops);
> +    }
> +#endif

I think you should always call xen_fe_register(vtpm ...) and move this
chunk in the xen_vtpmdev_ops .init function.


>      xen_be_register("console", &xen_console_ops);
>      xen_be_register("vkbd", &xen_kbdmouse_ops);
>      xen_be_register("qdisk", &xen_blkdev_ops);
> -- 
> 1.8.3.2
>

On Tue, 30 Dec 2014, Quan Xu wrote:
> +int vtpm_recv(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;
> +
> +    if (shr->state == TPMIF_STATE_IDLE) {
> +        return -ECANCELED;
> +    }
> +
> +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> +        vtpm_aio_wait(vtpm_aio_ctx);
> +    }

Is this really necessary to write this as a busy loop?
I think you should write it as a proper aio callback for efficiency:
QEMU is going to burn 100% of the cpu polling and not doing anything
else!


> +    offset = sizeof(*shr) + 4*shr->nr_extra_pages;
> +    memcpy(buf, offset + (uint8_t *)shr, shr->length);
> +    *count = shr->length;
> +
> +    return 0;
> +}

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, January 20, 2015 1:19 AM
> To: Xu, Quan
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org;
> stefano.stabellini@eu.citrix.com
> Subject: Re: [v3 3/5] Qemu-Xen-vTPM: Register Xen stubdom vTPM frontend
> driver
> 
> On Tue, 30 Dec 2014, Quan Xu wrote:
> > +int vtpm_recv(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;
> > +
> > +    if (shr->state == TPMIF_STATE_IDLE) {
> > +        return -ECANCELED;
> > +    }
> > +
> > +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> > +        vtpm_aio_wait(vtpm_aio_ctx);
> > +    }
> 
> Is this really necessary to write this as a busy loop?
> I think you should write it as a proper aio callback for efficiency:
> QEMU is going to burn 100% of the cpu polling and not doing anything else!

I agree.
I will improve it in v4.

-Quan
> 
> 
> > +    offset = sizeof(*shr) + 4*shr->nr_extra_pages;
> > +    memcpy(buf, offset + (uint8_t *)shr, shr->length);
> > +    *count = shr->length;
> > +
> > +    return 0;
> > +}

> -----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 3/5] Qemu-Xen-vTPM: Register Xen stubdom vTPM frontend
> driver
> 
> On Tue, 30 Dec 2014, 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]
> >  ..
> >   FE.DOMAIN.ID
> >    device = ""
> >     vtpm = ""
> >      0 = ""
> >       backend =
> "/local/domain/{BE.DOMAIN.ID}/backend/vtpm/{FE.DOMAIN.ID}/0"
> >       backend-id = "BE.DOMAIN.ID"
> >       state = "1"
> >       handle = "0"
> >  ..
> >
> > (QEMU) xen_vtpmdev_ops is initialized with the following process:
> >   xen_hvm_init()
> >     [...]
> >     -->xen_fe_register("vtpm", ...)
> >       -->xenstore_fe_scan()
> >         -->xen_fe_get_xendev()
> >           --> XenDevOps.alloc()
> >         -->xen_fe_check()
> >           --> XenDevOps.init()
> >           --> XenDevOps.initialise()
> >           --> XenDevOps.connected()
> >         -->xs_watch()
> >     [...]
> >
> > --Changes in v3:
> > -Move xen_stubdom_vtpm.c to xen_vtpm_frontend.c -Read Xen vTPM status
> > via XenStore
> >
> > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > ---
> >  hw/tpm/Makefile.objs         |   1 +
> >  hw/tpm/xen_vtpm_frontend.c   | 264
> +++++++++++++++++++++++++++++++++++++++++++
> >  hw/xen/xen_backend.c         |  34 ++++++
> >  include/hw/xen/xen_backend.h |   9 +-
> >  include/hw/xen/xen_common.h  |   6 +
> >  xen-hvm.c                    |  16 +++
> >  6 files changed, 328 insertions(+), 2 deletions(-)  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..00cc888
> > --- /dev/null
> > +++ b/hw/tpm/xen_vtpm_frontend.c
> > @@ -0,0 +1,264 @@
> > +/*
> > + * Connect to Xen vTPM stubdom domain
> > + *
> > + *  Copyright (c) 2014 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
> > + * 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
> > +<http://www.gnu.org/licenses/>  */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <stdarg.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <signal.h>
> > +#include <inttypes.h>
> > +#include <time.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <sys/mman.h>
> > +#include <sys/uio.h>
> > +
> > +#include "hw/hw.h"
> > +#include "block/aio.h"
> > +#include "hw/xen/xen_backend.h"
> > +
> > +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 */ };
> > +
> > +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)
> > +{
> > +    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;
> > +    }
> > +
> > +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> > +        vtpm_aio_wait(vtpm_aio_ctx);
> > +    }
> > +
> > +    offset = sizeof(*shr) + 4*shr->nr_extra_pages;
> > +    memcpy(buf, offset + (uint8_t *)shr, shr->length);
> > +    *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;
> > +
> > +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> > +        vtpm_aio_wait(vtpm_aio_ctx);
> > +    }
> > +
> > +    memcpy(offset + (uint8_t *)shr, buf, count);
> > +    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*/
> > +    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_be_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;
> > +        }
> > +    }
> > +    /* Tell vtpm backend that we are ready */
> > +    xenbus_switch_state(&vtpmdev->xendev, XenbusStateInitialised);
> > +
> > +    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);
> > +    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 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,
> > +    .alloc            = vtpm_alloc,
> > +    .initialise       = vtpm_initialise,
> > +    .backend_changed  = vtpm_backend_changed, };
> > diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c index
> > ad6e324..377a9c8 100644
> > --- a/hw/xen/xen_backend.c
> > +++ b/hw/xen/xen_backend.c
> > @@ -741,6 +741,20 @@ int xen_be_register(const char *type, struct
> XenDevOps *ops)
> >      return xenstore_scan(type, xen_domid, ops);  }
> >
> > +int xen_be_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_be_printf(xendev, 0, "xc_evtchn_alloc_unbound failed\n");
> > +        return -1;
> > +    }
> > +    xen_be_printf(xendev, 2, "bind evtchn port %d\n", xendev->local_port);
> > +    qemu_set_fd_handler(xc_evtchn_fd(xendev->evtchndev),
> > +                        xen_be_evtchn_event, NULL, xendev);
> > +    return 0;
> > +}
> 
> Why are you calling this xen_be_alloc_unbound? Shouldn't it be called
> xen_fe_alloc_unbound and be in xen_frontend.c, given that is called only by a
> frontend?
> 
> 
> >  int xen_be_bind_evtchn(struct XenDevice *xendev)  {
> >      if (xendev->local_port != -1) {
> > @@ -813,6 +827,26 @@ void xen_be_printf(struct XenDevice *xendev, int
> msg_level, const char *fmt, ...
> >      qemu_log_flush();
> >  }
> >
> > +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;
> > +        }
> > +    }
> > +}
> 
> This should be written as a generic fe function in xen_frontend.c
> 
> 
> >  void xen_qtail_insert_xendev(struct XenDevice *xendev)  {
> >      QTAILQ_INSERT_TAIL(&xendevs, xendev, next); diff --git
> > a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h index
> > 06e202f..d07ae36 100644
> > --- a/include/hw/xen/xen_backend.h
> > +++ b/include/hw/xen/xen_backend.h
> > @@ -102,8 +102,12 @@ 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);
> > +void xenstore_fe_update(char *watch, char *type, int dom, struct
> > +XenDevOps *ops);
> > +
> > +/*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     */
> > @@ -111,6 +115,7 @@ extern struct XenDevOps xen_kbdmouse_ops;     /*
> xen_framebuffer.c */
> >  extern struct XenDevOps xen_framebuffer_ops;  /* xen_framebuffer.c */
> >  extern struct XenDevOps xen_blkdev_ops;       /* xen_disk.c        */
> >  extern struct XenDevOps xen_netdev_ops;       /* xen_nic.c         */
> > +extern struct XenDevOps xen_vtpmdev_ops;      /*
> xen_vtpm_frontend.c*/
> >
> >  void xen_init_display(int domid);
> >
> > diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> > index 07731b9..2e9bb62 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..5b93cd4 100644
> > --- a/xen-hvm.c
> > +++ b/xen-hvm.c
> > @@ -986,6 +986,12 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size,
> ram_addr_t *above_4g_mem_size,
> >      int i, rc;
> >      unsigned long ioreq_pfn;
> >      unsigned long bufioreq_evtchn;
> > +
> > +#ifdef CONFIG_TPM_XENSTUBDOMS
> > +    char path[XEN_BUFSIZE];
> > +    unsigned int stubdom_vtpm = 0;
> > +#endif
> > +
> >      XenIOState *state;
> >
> >      state = g_malloc0(sizeof (XenIOState)); @@ -1071,6 +1077,16 @@
> > 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
> > +    snprintf(path, sizeof(path),
> "/local/domain/%d/platform/acpi_stubdom_vtpm",
> > +             xen_domid);
> > +    xs_read(state->xenstore, 0, path, &stubdom_vtpm);
> > +    if (stubdom_vtpm) {
> > +        xen_fe_register("vtpm", &xen_vtpmdev_ops);
> > +    }
> > +#endif
> 
> I think you should always call xen_fe_register(vtpm ...) and move this chunk in
> the xen_vtpmdev_ops .init function.

It should get xenstore status before to register vtpm.
xen_fe_register()
  --> xenstore_fe_scan()
    --> xen_fe_get_xendev() ... 
      --> xen_fe_check()
        --> ops .init()

It will do more much more redundant work, if move this chunk in this chunk in. maybe QEMU will crash if Xen doesn't 
register vtpm.  I think I this chunk could simplify further. 

-Quan

> 
> 
> >      xen_be_register("console", &xen_console_ops);
> >      xen_be_register("vkbd", &xen_kbdmouse_ops);
> >      xen_be_register("qdisk", &xen_blkdev_ops);
> > --
> > 1.8.3.2
> >

On Tue, 20 Jan 2015, Xu, Quan wrote:
> > -----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 3/5] Qemu-Xen-vTPM: Register Xen stubdom vTPM frontend
> > driver
> > 
> > On Tue, 30 Dec 2014, 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]
> > >  ..
> > >   FE.DOMAIN.ID
> > >    device = ""
> > >     vtpm = ""
> > >      0 = ""
> > >       backend =
> > "/local/domain/{BE.DOMAIN.ID}/backend/vtpm/{FE.DOMAIN.ID}/0"
> > >       backend-id = "BE.DOMAIN.ID"
> > >       state = "1"
> > >       handle = "0"
> > >  ..
> > >
> > > (QEMU) xen_vtpmdev_ops is initialized with the following process:
> > >   xen_hvm_init()
> > >     [...]
> > >     -->xen_fe_register("vtpm", ...)
> > >       -->xenstore_fe_scan()
> > >         -->xen_fe_get_xendev()
> > >           --> XenDevOps.alloc()
> > >         -->xen_fe_check()
> > >           --> XenDevOps.init()
> > >           --> XenDevOps.initialise()
> > >           --> XenDevOps.connected()
> > >         -->xs_watch()
> > >     [...]
> > >
> > > --Changes in v3:
> > > -Move xen_stubdom_vtpm.c to xen_vtpm_frontend.c -Read Xen vTPM status
> > > via XenStore
> > >
> > > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > > ---
> > >  hw/tpm/Makefile.objs         |   1 +
> > >  hw/tpm/xen_vtpm_frontend.c   | 264
> > +++++++++++++++++++++++++++++++++++++++++++
> > >  hw/xen/xen_backend.c         |  34 ++++++
> > >  include/hw/xen/xen_backend.h |   9 +-
> > >  include/hw/xen/xen_common.h  |   6 +
> > >  xen-hvm.c                    |  16 +++
> > >  6 files changed, 328 insertions(+), 2 deletions(-)  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..00cc888
> > > --- /dev/null
> > > +++ b/hw/tpm/xen_vtpm_frontend.c
> > > @@ -0,0 +1,264 @@
> > > +/*
> > > + * Connect to Xen vTPM stubdom domain
> > > + *
> > > + *  Copyright (c) 2014 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
> > > + * 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
> > > +<http://www.gnu.org/licenses/>  */
> > > +
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <stdarg.h>
> > > +#include <string.h>
> > > +#include <unistd.h>
> > > +#include <signal.h>
> > > +#include <inttypes.h>
> > > +#include <time.h>
> > > +#include <fcntl.h>
> > > +#include <errno.h>
> > > +#include <sys/ioctl.h>
> > > +#include <sys/types.h>
> > > +#include <sys/stat.h>
> > > +#include <sys/mman.h>
> > > +#include <sys/uio.h>
> > > +
> > > +#include "hw/hw.h"
> > > +#include "block/aio.h"
> > > +#include "hw/xen/xen_backend.h"
> > > +
> > > +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 */ };
> > > +
> > > +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)
> > > +{
> > > +    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;
> > > +    }
> > > +
> > > +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> > > +        vtpm_aio_wait(vtpm_aio_ctx);
> > > +    }
> > > +
> > > +    offset = sizeof(*shr) + 4*shr->nr_extra_pages;
> > > +    memcpy(buf, offset + (uint8_t *)shr, shr->length);
> > > +    *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;
> > > +
> > > +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> > > +        vtpm_aio_wait(vtpm_aio_ctx);
> > > +    }
> > > +
> > > +    memcpy(offset + (uint8_t *)shr, buf, count);
> > > +    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*/
> > > +    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_be_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;
> > > +        }
> > > +    }
> > > +    /* Tell vtpm backend that we are ready */
> > > +    xenbus_switch_state(&vtpmdev->xendev, XenbusStateInitialised);
> > > +
> > > +    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);
> > > +    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 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,
> > > +    .alloc            = vtpm_alloc,
> > > +    .initialise       = vtpm_initialise,
> > > +    .backend_changed  = vtpm_backend_changed, };
> > > diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c index
> > > ad6e324..377a9c8 100644
> > > --- a/hw/xen/xen_backend.c
> > > +++ b/hw/xen/xen_backend.c
> > > @@ -741,6 +741,20 @@ int xen_be_register(const char *type, struct
> > XenDevOps *ops)
> > >      return xenstore_scan(type, xen_domid, ops);  }
> > >
> > > +int xen_be_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_be_printf(xendev, 0, "xc_evtchn_alloc_unbound failed\n");
> > > +        return -1;
> > > +    }
> > > +    xen_be_printf(xendev, 2, "bind evtchn port %d\n", xendev->local_port);
> > > +    qemu_set_fd_handler(xc_evtchn_fd(xendev->evtchndev),
> > > +                        xen_be_evtchn_event, NULL, xendev);
> > > +    return 0;
> > > +}
> > 
> > Why are you calling this xen_be_alloc_unbound? Shouldn't it be called
> > xen_fe_alloc_unbound and be in xen_frontend.c, given that is called only by a
> > frontend?
> > 
> > 
> > >  int xen_be_bind_evtchn(struct XenDevice *xendev)  {
> > >      if (xendev->local_port != -1) {
> > > @@ -813,6 +827,26 @@ void xen_be_printf(struct XenDevice *xendev, int
> > msg_level, const char *fmt, ...
> > >      qemu_log_flush();
> > >  }
> > >
> > > +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;
> > > +        }
> > > +    }
> > > +}
> > 
> > This should be written as a generic fe function in xen_frontend.c
> > 
> > 
> > >  void xen_qtail_insert_xendev(struct XenDevice *xendev)  {
> > >      QTAILQ_INSERT_TAIL(&xendevs, xendev, next); diff --git
> > > a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h index
> > > 06e202f..d07ae36 100644
> > > --- a/include/hw/xen/xen_backend.h
> > > +++ b/include/hw/xen/xen_backend.h
> > > @@ -102,8 +102,12 @@ 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);
> > > +void xenstore_fe_update(char *watch, char *type, int dom, struct
> > > +XenDevOps *ops);
> > > +
> > > +/*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     */
> > > @@ -111,6 +115,7 @@ extern struct XenDevOps xen_kbdmouse_ops;     /*
> > xen_framebuffer.c */
> > >  extern struct XenDevOps xen_framebuffer_ops;  /* xen_framebuffer.c */
> > >  extern struct XenDevOps xen_blkdev_ops;       /* xen_disk.c        */
> > >  extern struct XenDevOps xen_netdev_ops;       /* xen_nic.c         */
> > > +extern struct XenDevOps xen_vtpmdev_ops;      /*
> > xen_vtpm_frontend.c*/
> > >
> > >  void xen_init_display(int domid);
> > >
> > > diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> > > index 07731b9..2e9bb62 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..5b93cd4 100644
> > > --- a/xen-hvm.c
> > > +++ b/xen-hvm.c
> > > @@ -986,6 +986,12 @@ int xen_hvm_init(ram_addr_t *below_4g_mem_size,
> > ram_addr_t *above_4g_mem_size,
> > >      int i, rc;
> > >      unsigned long ioreq_pfn;
> > >      unsigned long bufioreq_evtchn;
> > > +
> > > +#ifdef CONFIG_TPM_XENSTUBDOMS
> > > +    char path[XEN_BUFSIZE];
> > > +    unsigned int stubdom_vtpm = 0;
> > > +#endif
> > > +
> > >      XenIOState *state;
> > >
> > >      state = g_malloc0(sizeof (XenIOState)); @@ -1071,6 +1077,16 @@
> > > 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
> > > +    snprintf(path, sizeof(path),
> > "/local/domain/%d/platform/acpi_stubdom_vtpm",
> > > +             xen_domid);
> > > +    xs_read(state->xenstore, 0, path, &stubdom_vtpm);
> > > +    if (stubdom_vtpm) {
> > > +        xen_fe_register("vtpm", &xen_vtpmdev_ops);
> > > +    }
> > > +#endif
> > 
> > I think you should always call xen_fe_register(vtpm ...) and move this chunk in
> > the xen_vtpmdev_ops .init function.
> 
> It should get xenstore status before to register vtpm.
> xen_fe_register()
>   --> xenstore_fe_scan()
>     --> xen_fe_get_xendev() ... 
>       --> xen_fe_check()
>         --> ops .init()
> 
> It will do more much more redundant work, if move this chunk in this chunk in. maybe QEMU will crash if Xen doesn't 
> register vtpm.  I think I this chunk could simplify further. 

This is just setup code, ran only once at startup time. We can afford to
be a bit inefficient.

Consider that is most cases, if acpi_stubdom_vtpm is missing, than the
entire vtpm directory on xenstore is going to be missing, therefore
xenstore_fe_scan won't call xen_fe_get_xendev for vtpm.



> -Quan
> 
> > 
> > 
> > >      xen_be_register("console", &xen_console_ops);
> > >      xen_be_register("vkbd", &xen_kbdmouse_ops);
> > >      xen_be_register("qdisk", &xen_blkdev_ops);
> > > --
> > > 1.8.3.2
> > >
>

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, January 20, 2015 6:50 PM
> To: Xu, Quan
> Cc: Stefano Stabellini; qemu-devel@nongnu.org; xen-devel@lists.xen.org
> Subject: RE: [v3 3/5] Qemu-Xen-vTPM: Register Xen stubdom vTPM frontend
> driver
> 
> On Tue, 20 Jan 2015, Xu, Quan wrote:
> > > -----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 3/5] Qemu-Xen-vTPM: Register Xen stubdom vTPM
> > > frontend driver
> > >
> > > On Tue, 30 Dec 2014, 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]
> > > >  ..
> > > >   FE.DOMAIN.ID
> > > >    device = ""
> > > >     vtpm = ""
> > > >      0 = ""
> > > >       backend =
> > > "/local/domain/{BE.DOMAIN.ID}/backend/vtpm/{FE.DOMAIN.ID}/0"
> > > >       backend-id = "BE.DOMAIN.ID"
> > > >       state = "1"
> > > >       handle = "0"
> > > >  ..
> > > >
> > > > (QEMU) xen_vtpmdev_ops is initialized with the following process:
> > > >   xen_hvm_init()
> > > >     [...]
> > > >     -->xen_fe_register("vtpm", ...)
> > > >       -->xenstore_fe_scan()
> > > >         -->xen_fe_get_xendev()
> > > >           --> XenDevOps.alloc()
> > > >         -->xen_fe_check()
> > > >           --> XenDevOps.init()
> > > >           --> XenDevOps.initialise()
> > > >           --> XenDevOps.connected()
> > > >         -->xs_watch()
> > > >     [...]
> > > >
> > > > --Changes in v3:
> > > > -Move xen_stubdom_vtpm.c to xen_vtpm_frontend.c -Read Xen vTPM
> > > > status via XenStore
> > > >
> > > > Signed-off-by: Quan Xu <quan.xu@intel.com>
> > > > ---
> > > >  hw/tpm/Makefile.objs         |   1 +
> > > >  hw/tpm/xen_vtpm_frontend.c   | 264
> > > +++++++++++++++++++++++++++++++++++++++++++
> > > >  hw/xen/xen_backend.c         |  34 ++++++
> > > >  include/hw/xen/xen_backend.h |   9 +-
> > > >  include/hw/xen/xen_common.h  |   6 +
> > > >  xen-hvm.c                    |  16 +++
> > > >  6 files changed, 328 insertions(+), 2 deletions(-)  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..00cc888
> > > > --- /dev/null
> > > > +++ b/hw/tpm/xen_vtpm_frontend.c
> > > > @@ -0,0 +1,264 @@
> > > > +/*
> > > > + * Connect to Xen vTPM stubdom domain
> > > > + *
> > > > + *  Copyright (c) 2014 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
> > > > + * 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
> > > > +<http://www.gnu.org/licenses/>  */
> > > > +
> > > > +#include <stdio.h>
> > > > +#include <stdlib.h>
> > > > +#include <stdarg.h>
> > > > +#include <string.h>
> > > > +#include <unistd.h>
> > > > +#include <signal.h>
> > > > +#include <inttypes.h>
> > > > +#include <time.h>
> > > > +#include <fcntl.h>
> > > > +#include <errno.h>
> > > > +#include <sys/ioctl.h>
> > > > +#include <sys/types.h>
> > > > +#include <sys/stat.h>
> > > > +#include <sys/mman.h>
> > > > +#include <sys/uio.h>
> > > > +
> > > > +#include "hw/hw.h"
> > > > +#include "block/aio.h"
> > > > +#include "hw/xen/xen_backend.h"
> > > > +
> > > > +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 */ };
> > > > +
> > > > +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) {
> > > > +    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;
> > > > +    }
> > > > +
> > > > +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> > > > +        vtpm_aio_wait(vtpm_aio_ctx);
> > > > +    }
> > > > +
> > > > +    offset = sizeof(*shr) + 4*shr->nr_extra_pages;
> > > > +    memcpy(buf, offset + (uint8_t *)shr, shr->length);
> > > > +    *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;
> > > > +
> > > > +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> > > > +        vtpm_aio_wait(vtpm_aio_ctx);
> > > > +    }
> > > > +
> > > > +    memcpy(offset + (uint8_t *)shr, buf, count);
> > > > +    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*/
> > > > +    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_be_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;
> > > > +        }
> > > > +    }
> > > > +    /* Tell vtpm backend that we are ready */
> > > > +    xenbus_switch_state(&vtpmdev->xendev,
> > > > + XenbusStateInitialised);
> > > > +
> > > > +    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);
> > > > +    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 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,
> > > > +    .alloc            = vtpm_alloc,
> > > > +    .initialise       = vtpm_initialise,
> > > > +    .backend_changed  = vtpm_backend_changed, };
> > > > diff --git a/hw/xen/xen_backend.c b/hw/xen/xen_backend.c index
> > > > ad6e324..377a9c8 100644
> > > > --- a/hw/xen/xen_backend.c
> > > > +++ b/hw/xen/xen_backend.c
> > > > @@ -741,6 +741,20 @@ int xen_be_register(const char *type, struct
> > > XenDevOps *ops)
> > > >      return xenstore_scan(type, xen_domid, ops);  }
> > > >
> > > > +int xen_be_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_be_printf(xendev, 0, "xc_evtchn_alloc_unbound failed\n");
> > > > +        return -1;
> > > > +    }
> > > > +    xen_be_printf(xendev, 2, "bind evtchn port %d\n",
> xendev->local_port);
> > > > +    qemu_set_fd_handler(xc_evtchn_fd(xendev->evtchndev),
> > > > +                        xen_be_evtchn_event, NULL, xendev);
> > > > +    return 0;
> > > > +}
> > >
> > > Why are you calling this xen_be_alloc_unbound? Shouldn't it be
> > > called xen_fe_alloc_unbound and be in xen_frontend.c, given that is
> > > called only by a frontend?
> > >
> > >
> > > >  int xen_be_bind_evtchn(struct XenDevice *xendev)  {
> > > >      if (xendev->local_port != -1) { @@ -813,6 +827,26 @@ void
> > > > xen_be_printf(struct XenDevice *xendev, int
> > > msg_level, const char *fmt, ...
> > > >      qemu_log_flush();
> > > >  }
> > > >
> > > > +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;
> > > > +        }
> > > > +    }
> > > > +}
> > >
> > > This should be written as a generic fe function in xen_frontend.c
> > >
> > >
> > > >  void xen_qtail_insert_xendev(struct XenDevice *xendev)  {
> > > >      QTAILQ_INSERT_TAIL(&xendevs, xendev, next); diff --git
> > > > a/include/hw/xen/xen_backend.h b/include/hw/xen/xen_backend.h
> > > > index
> > > > 06e202f..d07ae36 100644
> > > > --- a/include/hw/xen/xen_backend.h
> > > > +++ b/include/hw/xen/xen_backend.h
> > > > @@ -102,8 +102,12 @@ 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);
> > > > +void xenstore_fe_update(char *watch, char *type, int dom, struct
> > > > +XenDevOps *ops);
> > > > +
> > > > +/*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
> */
> > > > @@ -111,6 +115,7 @@ extern struct XenDevOps xen_kbdmouse_ops;
> /*
> > > xen_framebuffer.c */
> > > >  extern struct XenDevOps xen_framebuffer_ops;  /* xen_framebuffer.c
> */
> > > >  extern struct XenDevOps xen_blkdev_ops;       /* xen_disk.c
> */
> > > >  extern struct XenDevOps xen_netdev_ops;       /* xen_nic.c
> */
> > > > +extern struct XenDevOps xen_vtpmdev_ops;      /*
> > > xen_vtpm_frontend.c*/
> > > >
> > > >  void xen_init_display(int domid);
> > > >
> > > > diff --git a/include/hw/xen/xen_common.h
> > > > b/include/hw/xen/xen_common.h index 07731b9..2e9bb62 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..5b93cd4 100644
> > > > --- a/xen-hvm.c
> > > > +++ b/xen-hvm.c
> > > > @@ -986,6 +986,12 @@ int xen_hvm_init(ram_addr_t
> > > > *below_4g_mem_size,
> > > ram_addr_t *above_4g_mem_size,
> > > >      int i, rc;
> > > >      unsigned long ioreq_pfn;
> > > >      unsigned long bufioreq_evtchn;
> > > > +
> > > > +#ifdef CONFIG_TPM_XENSTUBDOMS
> > > > +    char path[XEN_BUFSIZE];
> > > > +    unsigned int stubdom_vtpm = 0; #endif
> > > > +
> > > >      XenIOState *state;
> > > >
> > > >      state = g_malloc0(sizeof (XenIOState)); @@ -1071,6 +1077,16
> > > > @@ 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
> > > > +    snprintf(path, sizeof(path),
> > > "/local/domain/%d/platform/acpi_stubdom_vtpm",
> > > > +             xen_domid);
> > > > +    xs_read(state->xenstore, 0, path, &stubdom_vtpm);
> > > > +    if (stubdom_vtpm) {
> > > > +        xen_fe_register("vtpm", &xen_vtpmdev_ops);
> > > > +    }
> > > > +#endif
> > >
> > > I think you should always call xen_fe_register(vtpm ...) and move
> > > this chunk in the xen_vtpmdev_ops .init function.
> >
> > It should get xenstore status before to register vtpm.
> > xen_fe_register()
> >   --> xenstore_fe_scan()
> >     --> xen_fe_get_xendev() ...
> >       --> xen_fe_check()
> >         --> ops .init()
> >
> > It will do more much more redundant work, if move this chunk in this
> > chunk in. maybe QEMU will crash if Xen doesn't register vtpm.  I think I this
> chunk could simplify further.
> 
> This is just setup code, ran only once at startup time. We can afford to be a bit
> inefficient.
> 
> Consider that is most cases, if acpi_stubdom_vtpm is missing, than the entire
> vtpm directory on xenstore is going to be missing, therefore xenstore_fe_scan
> won't call xen_fe_get_xendev for vtpm.
> 

Okay, I will move this chunk in the xen_vtpmdev_ops .init function.

-Quan

> 
> 
> > -Quan
> >
> > >
> > >
> > > >      xen_be_register("console", &xen_console_ops);
> > > >      xen_be_register("vkbd", &xen_kbdmouse_ops);
> > > >      xen_be_register("qdisk", &xen_blkdev_ops);
> > > > --
> > > > 1.8.3.2
> > > >
> >

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, January 20, 2015 1:19 AM
> To: Xu, Quan
> Cc: qemu-devel@nongnu.org; xen-devel@lists.xen.org;
> stefano.stabellini@eu.citrix.com
> Subject: Re: [v3 3/5] Qemu-Xen-vTPM: Register Xen stubdom vTPM frontend
> driver
> 
> On Tue, 30 Dec 2014, Quan Xu wrote:
> > +int vtpm_recv(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;
> > +
> > +    if (shr->state == TPMIF_STATE_IDLE) {
> > +        return -ECANCELED;
> > +    }
> > +
> > +    while (vtpm_status(vtpmdev) != VTPM_STATUS_IDLE) {
> > +        vtpm_aio_wait(vtpm_aio_ctx);
> > +    }
> 
> Is this really necessary to write this as a busy loop?
> I think you should write it as a proper aio callback for efficiency:
> QEMU is going to burn 100% of the cpu polling and not doing anything else!

For further check and test, it is unnecessary to implement a busy loop in vtpm_recv. I can remove
It in v4.
It is similar to Linux PV frontend driver. In theory, the aio at the end of vtpm_send makes 
This busy loop unnecessary. 


           -------------
          |             |
          |    vtpm     |
          |   Domain    |
          |             |
           -------------
                 ^
          (Send) ||
            .    ||
            .    ||(Recv)
          (Send) ||
                  v
           -------------
          |             |
          |    QEMU     |
          |    vtpm     |
          |  frontend   |
          |             |
           -------------

The aio of vTPM is to make Send/Recv in order.


-Quan


> 
> 
> > +    offset = sizeof(*shr) + 4*shr->nr_extra_pages;
> > +    memcpy(buf, offset + (uint8_t *)shr, shr->length);
> > +    *count = shr->length;
> > +
> > +    return 0;
> > +}