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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 20 Jan 2015, Xu, Quan wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@xxxxxxxxxxxxx]
> > Sent: Tuesday, January 20, 2015 1:15 AM
> > To: Xu, Quan
> > Cc: qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx;
> > stefano.stabellini@xxxxxxxxxxxxx
> > 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@xxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxx>
> > > + *
> > > + * 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
> > >
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel