Re: [Qemu-devel] [PATCH v7 8/8] tpm: Added support for TPM emulator

["Valluri, Amarnath" <amarnath.valluri@intel.com>]

On Tue, 2017-09-26 at 14:24 +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Sep 26, 2017 at 2:05 PM, Valluri, Amarnath
> <amarnath.valluri@intel.com> wrote:
> > 
> > Hi Marc,
> > 
> > Thanks for your time reviewing this patchset. Please find my inline
> > comments.
> > 
> > On Sun, 2017-09-24 at 20:52 +0200, Marc-André Lureau wrote:
> > > 
> > > Hi
> > > 
> > > Thanks for the nice update, removing the exec() code, using
> > > chardev
> > > and a private socketpair. Some comments below:
> > > 
> > > On Fri, Sep 22, 2017 at 2:33 PM, Amarnath Valluri
> > > <amarnath.valluri@intel.com> wrote:
> > > > 
> > > > 
> > > > This change introduces a new TPM backend driver that can
> > > > communicate with
> > > > swtpm(software TPM emulator) using unix domain socket
> > > > interface.
> > > > QEMU talks to
> > > > TPM emulator using socket based chardev backend device.
> > > > 
> > > > Swtpm uses two Unix sockets for communications, one for plain
> > > > TPM
> > > > commands and
> > > > responses, and one for out-of-band control messages. QEMU
> > > > passes
> > > > data socket
> > > > been used over the control channel.
> > > > 
> > > > The swtpm and associated tools can be found here:
> > > >     https://github.com/stefanberger/swtpm
> > > > 
> > > > The swtpm's control channel protocol specification can be found
> > > > here:
> > > >     https://github.com/stefanberger/swtpm/wiki/Control-Channel-
> > > > Spec
> > > > ification
> > > > 
> > > > Usage:
> > > >     # setup TPM state directory
> > > >     mkdir /tmp/mytpm
> > > >     chown -R tss:root /tmp/mytpm
> > > >     /usr/bin/swtpm_setup --tpm-state /tmp/mytpm --createek
> > > > 
> > > >     # Ask qemu to use TPM emulator with given tpm state
> > > > directory
> > > >     qemu-system-x86_64 \
> > > >         [...] \
> > > >         -chardev socket,id=chrtpm,path=/tmp/swtpm-sock \
> > > >         -tpmdev emulator,id=tpm0,chardev=chrtpm \
> > > >         -device tpm-tis,tpmdev=tpm0 \
> > > >         [...]
> > > > 
> > > > Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>
> > > > ---
> > > >  configure             |  15 +-
> > > >  hmp.c                 |   5 +
> > > >  hw/tpm/Makefile.objs  |   1 +
> > > >  hw/tpm/tpm_emulator.c | 649
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  hw/tpm/tpm_ioctl.h    | 246 +++++++++++++++++++
> > > >  qapi/tpm.json         |  21 +-
> > > >  qemu-options.hx       |  22 +-
> > > >  7 files changed, 950 insertions(+), 9 deletions(-)
> > > >  create mode 100644 hw/tpm/tpm_emulator.c
> > > >  create mode 100644 hw/tpm/tpm_ioctl.h
> > > > 
> > > > diff --git a/configure b/configure
> > > > index cb0f7ed..ce2df2d 100755
> > > > --- a/configure
> > > > +++ b/configure
> > > > @@ -3461,10 +3461,15 @@ fi
> > > >  ##########################################
> > > >  # TPM passthrough is only on x86 Linux
> > > > 
> > > > -if test "$targetos" = Linux && test "$cpu" = i386 -o "$cpu" =
> > > > x86_64; then
> > > > -  tpm_passthrough=$tpm
> > > > +if test "$targetos" = Linux; then
> > > > +  tpm_emulator=$tpm
> > > > +  if test "$cpu" = i386 -o "$cpu" = x86_64; then
> > > > +    tpm_passthrough=$tpm
> > > > +  else
> > > > +    tpm_passthrough=no
> > > > +  fi
> > > >  else
> > > > -  tpm_passthrough=no
> > > > +  tpm_emulator=no
> > > >  fi
> > > > 
> > > >  ##########################################
> > > > @@ -5359,6 +5364,7 @@ echo "gcov enabled      $gcov"
> > > >  echo "TPM support       $tpm"
> > > >  echo "libssh2 support   $libssh2"
> > > >  echo "TPM passthrough   $tpm_passthrough"
> > > > +echo "TPM emulator      $tpm_emulator"
> > > >  echo "QOM debugging     $qom_cast_debug"
> > > >  echo "Live block migration $live_block_migration"
> > > >  echo "lzo support       $lzo"
> > > > @@ -5943,6 +5949,9 @@ if test "$tpm" = "yes"; then
> > > >    if test "$tpm_passthrough" = "yes"; then
> > > >      echo "CONFIG_TPM_PASSTHROUGH=y" >> $config_host_mak
> > > >    fi
> > > > +  if test "$tpm_emulator" = "yes"; then
> > > > +    echo "CONFIG_TPM_EMULATOR=y" >> $config_host_mak
> > > It shouldn't require Linux, but posix (and I assume a port to
> > > other
> > > systems isn't impossible). same for build-sys / help / comments.
> > I agree, Can you suggest, what is the Qemu way of limiting this to
> > 'posix'.
> > > 
> > > 
> there is CONFIG_POSIX
Ok, thanks. I will check
> 
> > 
> > > 
> > > > 
> > > > 
> > > > +  fi
> > > >  fi
> > > > 
> > > >  echo "TRACE_BACKENDS=$trace_backends" >> $config_host_mak
> > > > diff --git a/hmp.c b/hmp.c
> > > > index cf62b2e..7e69eca 100644
> > > > --- a/hmp.c
> > > > +++ b/hmp.c
> > > > @@ -995,6 +995,7 @@ void hmp_info_tpm(Monitor *mon, const QDict
> > > > *qdict)
> > > >      Error *err = NULL;
> > > >      unsigned int c = 0;
> > > >      TPMPassthroughOptions *tpo;
> > > > +    TPMEmulatorOptions *teo;
> > > > 
> > > >      info_list = qmp_query_tpm(&err);
> > > >      if (err) {
> > > > @@ -1024,6 +1025,10 @@ void hmp_info_tpm(Monitor *mon, const
> > > > QDict
> > > > *qdict)
> > > >                             tpo->has_cancel_path ? ",cancel-
> > > > path="
> > > > : "",
> > > >                             tpo->has_cancel_path ? tpo-
> > > > >cancel_path
> > > > : "");
> > > >              break;
> > > > +        case TPM_TYPE_EMULATOR:
> > > > +            teo = ti->options->u.emulator.data;
> > > > +            monitor_printf(mon, ",chardev=%s", teo->chardev);
> > > > +            break;
> > > >          case TPM_TYPE__MAX:
> > > >              break;
> > > >          }
> > > > diff --git a/hw/tpm/Makefile.objs b/hw/tpm/Makefile.objs
> > > > index 64cecc3..41f0b7a 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
> > > > tpm_util.o
> > > > +common-obj-$(CONFIG_TPM_EMULATOR) += tpm_emulator.o tpm_util.o
> > > > diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> > > > new file mode 100644
> > > > index 0000000..c02bbe2
> > > > --- /dev/null
> > > > +++ b/hw/tpm/tpm_emulator.c
> > > > @@ -0,0 +1,649 @@
> > > > +/*
> > > > + *  emulator TPM driver
> > > > + *
> > > > + *  Copyright (c) 2017 Intel Corporation
> > > > + *  Author: Amarnath Valluri <amarnath.valluri@intel.com>
> > > > + *
> > > > + *  Copyright (c) 2010 - 2013 IBM Corporation
> > > > + *  Authors:
> > > > + *    Stefan Berger <stefanb@us.ibm.com>
> > > > + *
> > > > + *  Copyright (C) 2011 IAIK, Graz University of Technology
> > > > + *    Author: Andreas Niederl
> > > > + *
> > > > + * 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.gn
> > > > u.or
> > > > g/licenses/>
> > > > + *
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include "qemu/sockets.h"
> > > > +#include "io/channel-socket.h"
> > > > +#include "sysemu/tpm_backend.h"
> > > > +#include "tpm_int.h"
> > > > +#include "hw/hw.h"
> > > > +#include "hw/i386/pc.h"
> > > > +#include "tpm_util.h"
> > > > +#include "tpm_ioctl.h"
> > > > +#include "migration/blocker.h"
> > > > +#include "qapi/error.h"
> > > > +#include "chardev/char-fe.h"
> > > > +
> > > > +#include <fcntl.h>
> > > > +#include <sys/types.h>
> > > > +#include <sys/stat.h>
> > > > +#include <stdio.h>
> > > > +
> > > > +#define DEBUG_TPM 0
> > > > +
> > > > +#define DPRINT(fmt, ...) do { \
> > > > +    if (DEBUG_TPM) { \
> > > > +        fprintf(stderr, fmt, ## __VA_ARGS__); \
> > > > +    } \
> > > > +} while (0);
> > > > +
> > > > +#define DPRINTF(fmt, ...) DPRINT("tpm-emulator: "fmt"\n",
> > > > __VA_ARGS__)
> > > I would define a single DPRINTF() (& drop DPRINT usage below)
> > Ok, will do
> > > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +
> > > > +#define TYPE_TPM_EMULATOR "tpm-emulator"
> > > > +#define TPM_EMULATOR(obj) \
> > > > +    OBJECT_CHECK(TPMEmulator, (obj), TYPE_TPM_EMULATOR)
> > > > +
> > > > +#define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps &
> > > > (cap)) == (cap))
> > > > +
> > > > +static const TPMDriverOps tpm_emulator_driver;
> > > > +
> > > > +/* data structures */
> > > > +typedef struct TPMEmulator {
> > > > +    TPMBackend parent;
> > > > +
> > > > +    TPMEmulatorOptions *options;
> > > Contrary to my comment on previous patch, I realize it is
> > > convenient
> > > to have a qapi pointer here, so you can use the free visitor
> > > later.
> > > 
> > > > 
> > > > 
> > > > +    CharBackend ctrl_dev;
> > > ctrl_chr perhaps? or just chr or be (the most common name in
> > > other
> > > devices).
> > > 
> > > > 
> > > > 
> > > > +    QIOChannel *data_ioc;
> > > > +    bool op_executing;
> > > > +    bool op_canceled;
> > > I think those 2 fields can be dropped, see below.
> > > 
> > > > 
> > > > 
> > > > +    bool had_startup_error;
> > > I think some little refactoring could remove the whole
> > > had_startup_error() backend & callback before or after the
> > > series.
> > > tpm_backend_startup_tpm() already returns an error code.  device
> > > or
> > > common backend code could handle & remember that.
> > Yes, i agree, we can avoid had_startup_error() from DriverOps, i
> > will
> > do this.
> > > 
> > > 
> > > > 
> > > > 
> > > > +    TPMVersion tpm_version;
> > > This is probably worth to consider in common code instead, but
> > > let's
> > > not worry about it.
> > > 
> > > > 
> > > > 
> > > > +    ptm_cap caps; /* capabilities of the TPM */
> > > > +    uint8_t cur_locty_number; /* last set locality */
> > > > +    QemuMutex state_lock;
> > > > +    Error *migration_blocker;
> > > > +} TPMEmulator;
> > > > +
> > > > +
> > > > +static int tpm_emulator_ctrlcmd(CharBackend *dev, unsigned
> > > > long
> > > > cmd, void *msg,
> > > > +                                size_t msg_len_in, size_t
> > > > msg_len_out)
> > > > +{
> > > > +    uint32_t cmd_no = cpu_to_be32(cmd);
> > > > +    ssize_t n = sizeof(uint32_t) + msg_len_in;
> > > > +    uint8_t *buf = NULL;
> > > > +
> > > > +    buf = (uint8_t *)malloc(n);
> > > could be g_alloca() instead. Alternatively, why not call 2
> > > write()
> > > instead?
> > > 
> > > none of the casts in this function are necessary, please remove
> > > them.
> > Ok, will change to aclloca()
> I just realized this is SOCK_SEQPACKET, ok
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +    memcpy(buf, &cmd_no, sizeof(cmd_no));
> > > > +    memcpy(buf + sizeof(cmd_no), msg, msg_len_in);
> > > > +
> > > > +    n += qemu_chr_fe_write_all(dev, (const uint8_t *)buf, n);
> > > weird
> > > 
> > > > 
> > > > 
> > > > +    free(buf);
> > > > +
> > > > +    if (n > 0) {
> > > with the n += above, you'll get interesting behaviour :)
> > Ya, it was typo :)
> > > 
> > > 
> > > You probably want to check if any write above failed, and return
> > > early
> > > for the error case.
> > > 
> > > Please improve the error handling in this function
> > > 
> > > > 
> > > > 
> > > > +        if (msg_len_out > 0) {
> > > > +            n = qemu_chr_fe_read_all(dev, (uint8_t *)msg,
> > > > msg_len_out);
> > > > +            /* simulate ioctl return value */
> > > > +            if (n > 0) {
> > > > +                n = 0;
> > > > +            }
> > > > +        } else {
> > > > +            n = 0;
> > > > +        }
> > > > +    }
> > > > +    return n;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_pt,
> > > tpm_pt was tpm_passthrough I suppose.
> > > 
> > > Please rename tpm_pt tpm_emu (or I would suggest "self", but this
> > > isn't common name in qemu code - it's actually common in a close
> > > c-object world., and it is quite convenient...)
> > My interpretation about _pt was 'pointer' ;-) i will consider your
> > suggetion.
> > > 
> > > 
> > > > 
> > > > 
> > > > +                                     const uint8_t *in,
> > > > uint32_t
> > > > in_len,
> > > > +                                     uint8_t *out, uint32_t
> > > > out_len,
> > > > +                                     bool *selftest_done)
> > > > +{
> > > > +    ssize_t ret;
> > > > +    bool is_selftest = false;
> > > > +    const struct tpm_resp_hdr *hdr = NULL;
> > > > +    Error *err = NULL;
> > > > +
> > > > +    tpm_pt->op_canceled = false;
> > > > +    tpm_pt->op_executing = true;
> > > > +    if (selftest_done) {
> > > > +        *selftest_done = false;
> > > > +        is_selftest = tpm_util_is_selftest(in, in_len);
> > > > +    }
> > > > +
> > > > +    ret = qio_channel_write(tpm_pt->data_ioc, (char *)in,
> > > > in_len,
> > > > &err);
> > > hmm, too bad qio_channel_write() doesn't take a void *
> > > 
> > > why not write_all()?
> > Agreed
> > > 
> > > 
> > > > 
> > > > 
> > > > +    if (ret != in_len || err) {
> > > > +        if (!tpm_pt->op_canceled || errno != ECANCELED) {
> > > I don't think ECANCELED make sense for emulator code.
> > > 
> > > > 
> > > > 
> > > > +            error_report("tpm-emulator: error while
> > > > transmitting
> > > > data "
> > > > +                         "to TPM: %s", err ?
> > > > error_get_pretty(err)
> > > > : "");
> > > > +            error_free(err);
> > > > +        }
> > > > +        goto err_exit;
> > > > +    }
> > > > +
> > > > +    tpm_pt->op_executing = false;
> > > > +
> > > > +    ret = qio_channel_read(tpm_pt->data_ioc, (char *)out,
> > > > out_len,
> > > > &err);
> > > > +    if (ret < 0 || err) {
> > > read_all() ?
> > The issue with read_all() is it does not return the no of bytes it
> > read, so i would like to stict to _read()
> ok
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +        if (!tpm_pt->op_canceled || errno != ECANCELED) {
> > > > +            error_report("tpm-emulator: error while reading
> > > > data
> > > > from "
> > > > +                         "TPM: %s", err ?
> > > > error_get_pretty(err) :
> > > > "");
> > > > +            error_free(err);
> > > > +        }
> > > > +    } else if (ret >= sizeof(*hdr)) {
> > > > +        hdr = (struct tpm_resp_hdr *)out;
> > > > +    }
> > > > +
> > > > +    if (!hdr || be32_to_cpu(hdr->len) != ret) {
> > > > +        error_report("tpm-emulator: received invalid response
> > > > "
> > > > +                     "packet from TPM with length :%ld", ret);
> > > > +        ret = -1;
> > > > +        goto err_exit;
> > > > +    }
> > > > +
> > > > +    if (is_selftest) {
> > > > +        *selftest_done = (be32_to_cpu(hdr->errcode) == 0);
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +
> > > > +err_exit:
> > > > +    if (ret < 0) {
> > > > +        tpm_util_write_fatal_error_response(out, out_len);
> > > > +    }
> > > > +
> > > > +    tpm_pt->op_executing = false;
> > > > +
> > > > +    return ret;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_set_locality(TPMEmulator *tpm_pt,
> > > > uint8_t
> > > > locty_number)
> > > > +{
> > > > +    ptm_loc loc;
> > > > +
> > > > +    DPRINTF("%s : locality: 0x%x", __func__, locty_number);
> > > > +
> > > > +    if (tpm_pt->cur_locty_number != locty_number) {
> > > return early instead if ==, to avoid code indent etc
> > Ok
> > > 
> > > 
> > > > 
> > > > 
> > > > +        DPRINTF("setting locality : 0x%x", locty_number);
> > > > +        loc.u.req.loc = locty_number;
> > > This number isn't set in be like the rest of the protocol?
> > I doubt if i get ur point :(, can you please elaborate.
> In the rest of the protocol, it uses big-endian. I suppose you should
> cpu_to_be32(locty_number).
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +        if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_SET_LOCALITY, &loc,
> > > > +                             sizeof(loc), sizeof(loc)) < 0) {
> > > > +            error_report("tpm-emulator: could not set locality
> > > > :
> > > > %s",
> > > > +                         strerror(errno));
> > > > +            return -1;
> > > > +        }
> > > > +        loc.u.resp.tpm_result =
> > > > be32_to_cpu(loc.u.resp.tpm_result);
> > > > +        if (loc.u.resp.tpm_result != 0) {
> > > > +            error_report("tpm-emulator: TPM result for set
> > > > locality : 0x%x",
> > > > +                         loc.u.resp.tpm_result);
> > > > +            return -1;
> > > > +        }
> > > > +        tpm_pt->cur_locty_number = locty_number;
> > > > +    }
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static void tpm_emulator_handle_request(TPMBackend *tb,
> > > > TPMBackendCmd cmd)
> > > > +{
> > > > +    TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > +    TPMLocality *locty = NULL;
> > > > +    bool selftest_done = false;
> > > > +
> > > > +    DPRINTF("processing command type %d", cmd);
> > > > +
> > > > +    switch (cmd) {
> > > > +    case TPM_BACKEND_CMD_PROCESS_CMD:
> > > > +        qemu_mutex_lock(&tpm_pt->state_lock);
> > > > +        locty = tb->tpm_state->locty_data;
> > > > +        if (tpm_emulator_set_locality(tpm_pt,
> > > > +                                      tb->tpm_state-
> > > > >locty_number)
> > > > < 0) {
> > > > +            tpm_util_write_fatal_error_response(locty-
> > > > > 
> > > > > r_buffer.buffer,
> > > > +                                           locty-
> > > > >r_buffer.size);
> > > return / goto here instead of else.
> > We should not retrun from here, we have to propagte the error
> > response
> > to device, and unlock the state.
> > > 
> > > 
> > > > 
> > > > 
> > > > +        } else {
> > > > +            tpm_emulator_unix_tx_bufs(tpm_pt, locty-
> > > > > 
> > > > > w_buffer.buffer,
> > > > +                                              locty->w_offset,
> > > > +                                              locty-
> > > > > 
> > > > > r_buffer.buffer,
> > > > +                                              locty-
> > > > > 
> > > > > r_buffer.size,
> > > > +                                              &selftest_done);
> > > no error handling?
> > The error case is supposed to handle by device, where we are
> > filling
> > into out buffer, using tpm_util_write_fatal_error_response().
> > > 
> > > 
> > > > 
> > > > 
> > > > +        }
> > > > +
> > > > +        tb->recv_data_callback(tb->tpm_state, tb->tpm_state-
> > > > > 
> > > > > locty_number,
> > > > +                               selftest_done);
> > > > +        qemu_mutex_unlock(&tpm_pt->state_lock);
> > > > +
> > > > +        break;
> > > > +    case TPM_BACKEND_CMD_INIT:
> > > > +    case TPM_BACKEND_CMD_END:
> > > > +    case TPM_BACKEND_CMD_TPM_RESET:
> > > > +        /* nothing to do */
> > > > +        break;
> > > > +    }
> > > > +}
> > > > +
> > > > +/*
> > > > + * Gracefully shut down the external unixio TPM
> > > > + */
> > > > +static void tpm_emulator_shutdown(TPMEmulator *tpm_pt)
> > > > +{
> > > > +    ptm_res res;
> > > > +
> > > > +    if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev, CMD_SHUTDOWN,
> > > > &res, 0,
> > > > +                         sizeof(res)) < 0) {
> > > > +        error_report("tpm-emulator: Could not cleanly shutdown
> > > > the
> > > > TPM: %s",
> > > > +                     strerror(errno));
> > > > +    } else if (res != 0) {
> > > > +        error_report("tpm-emulator: TPM result for sutdown:
> > > > 0x%x",
> > > > +                     be32_to_cpu(res));
> > > > +    }
> > > > +}
> > > > +
> > > > +static int tpm_emulator_probe_caps(TPMEmulator *tpm_pt)
> > > > +{
> > > > +    DPRINTF("%s", __func__);
> > > > +    if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_GET_CAPABILITY,
> > > > +                         &tpm_pt->caps, 0, sizeof(tpm_pt-
> > > > >caps)) <
> > > > 0) {
> > > > +        error_report("tpm-emulator: probing failed : %s",
> > > > strerror(errno));
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    tpm_pt->caps = be64_to_cpu(tpm_pt->caps);
> > > > +
> > > > +    DPRINTF("capbilities : 0x%lx", tpm_pt->caps);
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_check_caps(TPMEmulator *tpm_pt)
> > > > +{
> > > > +    ptm_cap caps = 0;
> > > > +    const char *tpm = NULL;
> > > > +
> > > > +    /* check for min. required capabilities */
> > > > +    switch (tpm_pt->tpm_version) {
> > > > +    case TPM_VERSION_1_2:
> > > > +        caps = PTM_CAP_INIT | PTM_CAP_SHUTDOWN |
> > > > PTM_CAP_GET_TPMESTABLISHED |
> > > > +               PTM_CAP_SET_LOCALITY;
> > > > +        tpm = "1.2";
> > > > +        break;
> > > > +    case TPM_VERSION_2_0:
> > > > +        caps = PTM_CAP_INIT | PTM_CAP_SHUTDOWN |
> > > > PTM_CAP_GET_TPMESTABLISHED |
> > > > +               PTM_CAP_SET_LOCALITY |
> > > > PTM_CAP_RESET_TPMESTABLISHED;
> > > > +        tpm = "2";
> > > > +        break;
> > > > +    case TPM_VERSION_UNSPEC:
> > > > +        error_report("tpm-emulator: TPM version has not been
> > > > set");
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_pt, caps)) {
> > > > +        error_report("tpm-emulator: TPM does not implement
> > > > minimum
> > > > set of "
> > > > +                     "required capabilities for TPM %s
> > > > (0x%x)",
> > > > tpm, (int)caps);
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_startup_tpm(TPMBackend *tb)
> > > > +{
> > > > +    TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > +    ptm_init init;
> > > > +    ptm_res res;
> > > > +
> > > > +    DPRINTF("%s", __func__);
> > > > +    if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev, CMD_INIT,
> > > > &init,
> > > > sizeof(init),
> > > > +                         sizeof(init)) < 0) {
> > > > +        error_report("tpm-emulator: could not send INIT: %s",
> > > > +                     strerror(errno));
> > > > +        goto err_exit;
> > > > +    }
> > > > +
> > > > +    res = be32_to_cpu(init.u.resp.tpm_result);
> > > > +    if (res) {
> > > > +        error_report("tpm-emulator: TPM result for CMD_INIT:
> > > > 0x%x", res);
> > > > +        goto err_exit;
> > > > +    }
> > > > +    return 0;
> > > > +
> > > > +err_exit:
> > > > +    tpm_pt->had_startup_error = true;
> > > > +    return -1;
> > > > +}
> > > > +
> > > > +static bool tpm_emulator_get_tpm_established_flag(TPMBackend
> > > > *tb)
> > > > +{
> > > > +    TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > +    ptm_est est;
> > > > +
> > > > +    DPRINTF("%s", __func__);
> > > > +    if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_GET_TPMESTABLISHED, &est, 0,
> > > > +                         sizeof(est)) < 0) {
> > > > +        error_report("tpm-emulator: Could not get the TPM
> > > > established flag: %s",
> > > > +                     strerror(errno));
> > > > +        return false;
> > > > +    }
> > > > +    DPRINTF("established flag: %0x", est.u.resp.bit);
> > > > +
> > > > +    return (est.u.resp.bit != 0);
> > > > +}
> > > > +
> > > > +static int tpm_emulator_reset_tpm_established_flag(TPMBackend
> > > > *tb,
> > > > +                                                   uint8_t
> > > > locty)
> > > > +{
> > > > +    TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > +    ptm_reset_est reset_est;
> > > > +    ptm_res res;
> > > > +
> > > > +    /* only a TPM 2.0 will support this */
> > > > +    if (tpm_pt->tpm_version == TPM_VERSION_2_0) {
> > > > +        reset_est.u.req.loc = tpm_pt->cur_locty_number;
> > > > +
> > > > +        if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_RESET_TPMESTABLISHED,
> > > > +                                 &reset_est,
> > > > sizeof(reset_est),
> > > > +                                 sizeof(reset_est)) < 0) {
> > > > +            error_report("tpm-emulator: Could not reset the
> > > > establishment bit: "
> > > > +                          "%s", strerror(errno));
> > > > +            return -1;
> > > > +        }
> > > > +
> > > > +        res = be32_to_cpu(reset_est.u.resp.tpm_result);
> > > > +        if (res) {
> > > > +            error_report("tpm-emulator: TPM result for rest
> > > > establixhed flag: "
> > > > +                         "0x%x", res);
> > > > +            return -1;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static bool tpm_emulator_had_startup_error(TPMBackend *tb)
> > > > +{
> > > > +    TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > +
> > > > +    return tpm_pt->had_startup_error;
> > > > +}
> > > > +
> > > > +static void tpm_emulator_cancel_cmd(TPMBackend *tb)
> > > > +{
> > > > +    TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > +    ptm_res res;
> > > > +
> > > > +    /*
> > > > +     * As of Linux 3.7 the tpm_tis driver does not properly
> > > > cancel
> > > > +     * commands on all TPM manufacturers' TPMs.
> > > > +     * Only cancel if we're busy so we don't cancel someone
> > > > else's
> > > > +     * command, e.g., a command executed on the host.
> > > > +     */
> > > > +    if (tpm_pt->op_executing) {
> > > The field is set in the worker thread. This is racy. Fortunately
> > > this
> > > is not relevant for emulator, I think you can simply drop that
> > > check
> > > and the variable. Stefan should confirm though.
> > > 
> > > > 
> > > > 
> > > > +        if (TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_pt,
> > > > PTM_CAP_CANCEL_TPM_CMD)) {
> > > > +            if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_CANCEL_TPM_CMD,
> > > > +                                     &res, 0, sizeof(res)) <
> > > > 0) {
> > > > +                error_report("tpm-emulator: Could not cancel
> > > > command: %s",
> > > > +                             strerror(errno));
> > > > +            } else if (res != 0) {
> > > > +                error_report("tpm-emulator: Failed to cancel
> > > > TPM:
> > > > 0x%x",
> > > > +                             be32_to_cpu(res));
> > > > +            } else {
> > > > +                tpm_pt->op_canceled = true;
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +}
> > > > +
> > > > +static void tpm_emulator_reset(TPMBackend *tb)
> > > > +{
> > > > +    DPRINTF("%s", __func__);
> > > > +
> > > > +    tpm_emulator_cancel_cmd(tb);
> > > > +}
> > > > +
> > > > +static TPMVersion tpm_emulator_get_tpm_version(TPMBackend *tb)
> > > > +{
> > > > +    TPMEmulator *tpm_pt = TPM_EMULATOR(tb);
> > > > +
> > > > +    return tpm_pt->tpm_version;
> > > > +}
> > > > +
> > > > +static void tpm_emulator_block_migration(TPMEmulator *tpm_pt)
> > > > +{
> > > > +    Error *err = NULL;
> > > > +
> > > > +    error_setg(&tpm_pt->migration_blocker,
> > > > +               "Migration disabled: TPM emulator not yet
> > > > migratable");
> > > > +    migrate_add_blocker(tpm_pt->migration_blocker, &err);
> > > > +    if (err) {
> > > > +        error_free(err);
> > > probably better to report_err it instead
> > Ok
> > > 
> > > 
> > > > 
> > > > 
> > > > +        error_free(tpm_pt->migration_blocker);
> > > > +        tpm_pt->migration_blocker = NULL;
> > > and return an error code.
> > Will do
> > > 
> > > 
> > > > 
> > > > 
> > > > +    }
> > > > +}
> > > > +
> > > > +static int tpm_emulator_prepare_data_fd(TPMEmulator *tpm_pt)
> > > > +{
> > > > +    ptm_res res;
> > > > +    Error *err = NULL;
> > > > +    int fds[2] = { -1, -1 };
> > > > +
> > > > +    if (socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds) < 0) {
> > > > +        error_report("tpm-emulator: Failed to create
> > > > socketpair");
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    qemu_chr_fe_set_msgfds(&tpm_pt->ctrl_dev, fds + 1, 1);
> > > > +
> > > > +    if (tpm_emulator_ctrlcmd(&tpm_pt->ctrl_dev,
> > > > CMD_SET_DATAFD,
> > > > &res, 0,
> > > > +                    sizeof(res)) || res != 0) {
> > > Yay! for making life easier and hiding a protocol detail.
> > > 
> > > > 
> > > > 
> > > > +        error_report("tpm-emulator: Failed to send
> > > > CMD_SET_DATAFD:
> > > > %s",
> > > > +                     strerror(errno));
> > > > +        goto err_exit;
> > > > +    }
> > > > +
> > > > +    tpm_pt->data_ioc =
> > > > QIO_CHANNEL(qio_channel_socket_new_fd(fds[0], &err));
> > > > +    if (err) {
> > > > +        error_report("tpm-emulator: Failed to create io
> > > > channel :
> > > > %s",
> > > > +                       error_get_pretty(err));
> > > error_prepend()?
> > Ok
> > > 
> > > 
> > > > 
> > > > 
> > > > +        error_free(err);
> > > > +        goto err_exit;
> > > > +    }
> > > close fds[1] ?
> > I guess we are not supposed to close the other end of the
> > socketpair/pipe, as it is not forked process.
> You will close this end, not the other end.
fds[1] is handed over to other end, fds[0] is used by qemu, so we
cannot close unless error case i guess.

- Amarnath