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.vall...@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.vall...@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.vall...@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.vall...@intel.com> > > > > + * > > > > + * Copyright (c) 2010 - 2013 IBM Corporation > > > > + * Authors: > > > > + * Stefan Berger <stef...@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
