[Qemu-devel] [PATCH v4 0/2] tpm: Extend TPM with state migration support

[Qemu-devel] [PATCH v4 0/2] tpm: Extend TPM with state migration support

[Stefan Berger]

This series of patches implements support for migrating the state of the
external 'swtpm' TPM emulator as well as that of the TIS interface. 

For testing of TPM 2 (migration) please use the following git repos and
branches:

libtpms: 
   - repo: https://github.com/stefanberger/libtpms
   - branch: tpm2-preview.rev146.v2

swtpm:
   - repo: https://github.com/stefanberger/swtpm
   - branch: tpm2-preview.rev146.v2

Regards,
  Stefan

Changes:
 v3->v4:
   - dropped the size limit enforcement on blobs received from the swtpm
   - the .post_load migration function requires errno's to be returned.
     -> some of the functions have been converted to return a better errno

Stefan Berger (2):
  tpm: extend TPM emulator with state migration support
  tpm: extend TPM TIS with state migration support

 hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/tpm/tpm_tis.c      |  54 ++++++++-
 2 files changed, 355 insertions(+), 11 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Stefan Berger]

Extend the TPM emulator backend device with state migration support.

The external TPM emulator 'swtpm' provides a protocol over
its control channel to retrieve its state blobs. We implement
functions for getting and setting the different state blobs.
In case the setting of the state blobs fails, we return a
negative errno code to fail the start of the VM.

Since we have an external TPM emulator, we need to make sure
that we do not migrate the state for as long as it is busy
processing a request. We need to wait for notification that
the request has completed processing.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 302 insertions(+), 10 deletions(-)

diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index b787aee..da877e5 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -55,6 +55,19 @@
 #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
 
 /* data structures */
+
+/* blobs from the TPM; part of VM state when migrating */
+typedef struct TPMBlobBuffers {
+    uint32_t permanent_flags;
+    TPMSizedBuffer permanent;
+
+    uint32_t volatil_flags;
+    TPMSizedBuffer volatil;
+
+    uint32_t savestate_flags;
+    TPMSizedBuffer savestate;
+} TPMBlobBuffers;
+
 typedef struct TPMEmulator {
     TPMBackend parent;
 
@@ -70,6 +83,8 @@ typedef struct TPMEmulator {
 
     unsigned int established_flag:1;
     unsigned int established_flag_cached:1;
+
+    TPMBlobBuffers state_blobs;
 } TPMEmulator;
 
 
@@ -301,7 +316,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
     return 0;
 }
 
-static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
+                                     bool is_resume)
 {
     TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
     ptm_init init = {
@@ -309,12 +325,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
     };
     ptm_res res;
 
+    DPRINTF("%s   is_resume: %d", __func__, is_resume);
+
     if (buffersize != 0 &&
         tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
         goto err_exit;
     }
 
-    DPRINTF("%s", __func__);
+    if (is_resume) {
+        init.u.req.init_flags = cpu_to_be32(PTM_INIT_FLAG_DELETE_VOLATILE);
+    }
+
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
                              sizeof(init)) < 0) {
         error_report("tpm-emulator: could not send INIT: %s",
@@ -333,6 +354,11 @@ err_exit:
     return -1;
 }
 
+static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+{
+    return _tpm_emulator_startup_tpm(tb, buffersize, false);
+}
+
 static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
 {
     TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
@@ -431,16 +457,21 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
 static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
 {
     Error *err = NULL;
+    ptm_cap caps = PTM_CAP_GET_STATEBLOB | PTM_CAP_SET_STATEBLOB |
+                   PTM_CAP_STOP;
 
-    error_setg(&tpm_emu->migration_blocker,
-               "Migration disabled: TPM emulator not yet migratable");
-    migrate_add_blocker(tpm_emu->migration_blocker, &err);
-    if (err) {
-        error_report_err(err);
-        error_free(tpm_emu->migration_blocker);
-        tpm_emu->migration_blocker = NULL;
+    if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_emu, caps)) {
+        error_setg(&tpm_emu->migration_blocker,
+                   "Migration disabled: TPM emulator does not support "
+                   "migration");
+        migrate_add_blocker(tpm_emu->migration_blocker, &err);
+        if (err) {
+            error_report_err(err);
+            error_free(tpm_emu->migration_blocker);
+            tpm_emu->migration_blocker = NULL;
 
-        return -1;
+            return -1;
+        }
     }
 
     return 0;
@@ -569,6 +600,263 @@ static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
     { /* end of list */ },
 };
 
+/*
+ * Transfer a TPM state blob from the TPM into a provided buffer.
+ *
+ * @tpm_emu: TPMEmulator
+ * @type: the type of blob to transfer
+ * @tsb: the TPMSizeBuffer to fill with the blob
+ * @flags: the flags to return to the caller
+ */
+static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
+                                       uint8_t type,
+                                       TPMSizedBuffer *tsb,
+                                       uint32_t *flags)
+{
+    ptm_getstate pgs;
+    ptm_res res;
+    ssize_t n;
+    uint32_t totlength, length;
+
+    tpm_sized_buffer_reset(tsb);
+
+    pgs.u.req.state_flags = cpu_to_be32(PTM_STATE_FLAG_DECRYPTED);
+    pgs.u.req.type = cpu_to_be32(type);
+    pgs.u.req.offset = 0;
+
+    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_STATEBLOB,
+                             &pgs, sizeof(pgs.u.req),
+                             offsetof(ptm_getstate, u.resp.data)) < 0) {
+        error_report("tpm-emulator: could not get state blob type %d : %s",
+                     type, strerror(errno));
+        return -1;
+    }
+
+    res = be32_to_cpu(pgs.u.resp.tpm_result);
+    if (res != 0 && (res & 0x800) == 0) {
+        error_report("tpm-emulator: Getting the stateblob (type %d) failed "
+                     "with a TPM error 0x%x", type, res);
+        return -1;
+    }
+
+    totlength = be32_to_cpu(pgs.u.resp.totlength);
+    length = be32_to_cpu(pgs.u.resp.length);
+    if (totlength != length) {
+        error_report("tpm-emulator: Expecting to read %u bytes "
+                     "but would get %u", totlength, length);
+        return -1;
+    }
+
+    *flags = be32_to_cpu(pgs.u.resp.state_flags);
+
+    tsb->buffer = g_malloc(totlength);
+
+    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
+    if (n != totlength) {
+        error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
+                     type, strerror(errno));
+        return -1;
+    }
+    tsb->size = totlength;
+
+    DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
+            type, tsb->size, *flags);
+
+    return 0;
+}
+
+static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
+{
+    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
+
+    if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
+                                    &state_blobs->permanent,
+                                    &state_blobs->permanent_flags) < 0 ||
+        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
+                                    &state_blobs->volatil,
+                                    &state_blobs->volatil_flags) < 0 ||
+        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
+                                    &state_blobs->savestate,
+                                    &state_blobs->savestate_flags) < 0) {
+        goto err_exit;
+    }
+
+    return 0;
+
+ err_exit:
+    tpm_sized_buffer_reset(&state_blobs->volatil);
+    tpm_sized_buffer_reset(&state_blobs->permanent);
+    tpm_sized_buffer_reset(&state_blobs->savestate);
+
+    return -1;
+}
+
+/*
+ * Transfer a TPM state blob to the TPM emulator.
+ *
+ * @tpm_emu: TPMEmulator
+ * @type: the type of TPM state blob to transfer
+ * @tsb: TPMSizeBuffer containing the TPM state blob
+ * @flags: Flags describing the (encryption) state of the TPM state blob
+ */
+static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
+                                       uint32_t type,
+                                       TPMSizedBuffer *tsb,
+                                       uint32_t flags)
+{
+    uint32_t offset = 0;
+    ssize_t n;
+    ptm_setstate pss;
+    ptm_res tpm_result;
+
+    if (tsb->size == 0) {
+        return 0;
+    }
+
+    pss = (ptm_setstate) {
+        .u.req.state_flags = cpu_to_be32(flags),
+        .u.req.type = cpu_to_be32(type),
+        .u.req.length = cpu_to_be32(tsb->size),
+    };
+
+    /* write the header only */
+    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
+                             offsetof(ptm_setstate, u.req.data), 0) < 0) {
+        error_report("tpm-emulator: could not set state blob type %d : %s",
+                     type, strerror(errno));
+        return -1;
+    }
+
+    /* now the body */
+    n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
+                              &tsb->buffer[offset], tsb->size);
+    if (n != tsb->size) {
+        error_report("tpm-emulator: Writing the stateblob (type %d) "
+                     "failed: %s", type, strerror(errno));
+        return -1;
+    }
+
+    /* now get the result */
+    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
+                             (uint8_t *)&pss, sizeof(pss.u.resp));
+    if (n != sizeof(pss.u.resp)) {
+        error_report("tpm-emulator: Reading response from writing stateblob "
+                     "(type %d) failed: %s", type, strerror(errno));
+        return -1;
+    }
+
+    tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
+    if (tpm_result != 0) {
+        error_report("tpm-emulator: Setting the stateblob (type %d) failed "
+                     "with a TPM error 0x%x", type, tpm_result);
+        return -1;
+    }
+
+    DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
+            type, tsb->size, flags);
+
+    return 0;
+}
+
+/*
+ * Set all the TPM state blobs.
+ *
+ * Returns a negative errno code in case of error.
+ */
+static int tpm_emulator_set_state_blobs(TPMBackend *tb)
+{
+    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
+    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
+
+    DPRINTF("%s: %d", __func__, __LINE__);
+
+    if (tpm_emulator_stop_tpm(tb) < 0) {
+        return -EIO;
+    }
+
+    if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
+                                    &state_blobs->permanent,
+                                    state_blobs->permanent_flags) < 0 ||
+        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
+                                    &state_blobs->volatil,
+                                    state_blobs->volatil_flags) < 0 ||
+        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
+                                    &state_blobs->savestate,
+                                    state_blobs->savestate_flags) < 0) {
+        return -EBADMSG;
+    }
+
+    DPRINTF("DONE SETTING STATEBLOBS");
+
+    return 0;
+}
+
+static int tpm_emulator_pre_save(void *opaque)
+{
+    TPMBackend *tb = opaque;
+    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
+
+    DPRINTF("%s", __func__);
+
+    tpm_backend_finish_sync(tb);
+
+    /* get the state blobs from the TPM */
+    return tpm_emulator_get_state_blobs(tpm_emu);
+}
+
+/*
+ * Load the TPM state blobs into the TPM.
+ *
+ * Returns negative errno codes in case of error.
+ */
+static int tpm_emulator_post_load(void *opaque,
+                                  int version_id __attribute__((unused)))
+{
+    TPMBackend *tb = opaque;
+    int ret;
+
+    ret = tpm_emulator_set_state_blobs(tb);
+    if (ret < 0) {
+        return ret;
+    }
+
+    if (_tpm_emulator_startup_tpm(tb, 0, true) < 0) {
+        return -EIO;
+    }
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_tpm_emulator = {
+    .name = "tpm-emulator",
+    .version_id = 1,
+    .minimum_version_id = 0,
+    .minimum_version_id_old = 0,
+    .pre_save = tpm_emulator_pre_save,
+    .post_load = tpm_emulator_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
+        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
+        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
+                                     TPMEmulator, 1, 0,
+                                     state_blobs.permanent.size),
+
+        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
+        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
+        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
+                                     TPMEmulator, 1, 0,
+                                     state_blobs.volatil.size),
+
+        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
+        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
+        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
+                                     TPMEmulator, 1, 0,
+                                     state_blobs.savestate.size),
+
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void tpm_emulator_inst_init(Object *obj)
 {
     TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
@@ -577,6 +865,8 @@ static void tpm_emulator_inst_init(Object *obj)
     tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
     tpm_emu->cur_locty_number = ~0;
     qemu_mutex_init(&tpm_emu->mutex);
+
+    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
 }
 
 /*
@@ -613,6 +903,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
     }
 
     qemu_mutex_destroy(&tpm_emu->mutex);
+
+    vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);
 }
 
 static void tpm_emulator_class_init(ObjectClass *klass, void *data)
-- 
2.5.5

[Qemu-devel] [PATCH v4 2/2] tpm: extend TPM TIS with state migration support

[Stefan Berger]

Extend the TPM TIS interface with state migration support.

We need to synchronize with the backend thread to make sure that a command
being processed by the external TPM emulator has completed and its
response been received.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 hw/tpm/tpm_tis.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 834eef7..5016d28 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -902,9 +902,61 @@ static void tpm_tis_reset(DeviceState *dev)
     tpm_backend_startup_tpm(s->be_driver, s->be_buffer_size);
 }
 
+/* persistent state handling */
+
+static int tpm_tis_pre_save(void *opaque)
+{
+    TPMState *s = opaque;
+    uint8_t locty = s->active_locty;
+
+    DPRINTF("tpm_tis: suspend: locty = %d : rw_offset = %u\n",
+            locty, s->rw_offset);
+#ifdef DEBUG_TIS
+    tpm_tis_dump_state(opaque, 0);
+#endif
+
+    /*
+     * Synchronize with backend completion.
+     */
+    tpm_backend_finish_sync(s->be_driver);
+
+    return 0;
+}
+
+static const VMStateDescription vmstate_locty = {
+    .name = "loc",
+    .version_id = 1,
+    .minimum_version_id = 0,
+    .minimum_version_id_old = 0,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32(state, TPMLocality),
+        VMSTATE_UINT32(inte, TPMLocality),
+        VMSTATE_UINT32(ints, TPMLocality),
+        VMSTATE_UINT8(access, TPMLocality),
+        VMSTATE_UINT32(sts, TPMLocality),
+        VMSTATE_UINT32(iface_id, TPMLocality),
+        VMSTATE_END_OF_LIST(),
+    }
+};
+
 static const VMStateDescription vmstate_tpm_tis = {
     .name = "tpm",
-    .unmigratable = 1,
+    .version_id = 1,
+    .minimum_version_id = 0,
+    .minimum_version_id_old = 0,
+    .pre_save  = tpm_tis_pre_save,
+    .fields = (VMStateField[]) {
+        VMSTATE_BUFFER(buffer, TPMState),
+        VMSTATE_UINT16(rw_offset, TPMState),
+        VMSTATE_UINT8(active_locty, TPMState),
+        VMSTATE_UINT8(aborting_locty, TPMState),
+        VMSTATE_UINT8(next_locty, TPMState),
+
+        VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 1,
+                             vmstate_locty, TPMLocality),
+
+        VMSTATE_END_OF_LIST()
+    }
 };
 
 static Property tpm_tis_properties[] = {
-- 
2.5.5

Re: [Qemu-devel] [PATCH v4 0/2] tpm: Extend TPM with state migration support

[Stefan Berger]

On 03/01/2018 02:59 PM, Stefan Berger wrote:
> This series of patches implements support for migrating the state of the
> external 'swtpm' TPM emulator as well as that of the TIS interface.
>
> For testing of TPM 2 (migration) please use the following git repos and
> branches:
>
> libtpms:
>     - repo: https://github.com/stefanberger/libtpms
>     - branch: tpm2-preview.rev146.v2
>
> swtpm:
>     - repo: https://github.com/stefanberger/swtpm
>     - branch: tpm2-preview.rev146.v2

Typo: -> tpm2-preview.v2


   Stefan

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Marc-André Lureau]

Hi Stefan

On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Extend the TPM emulator backend device with state migration support.
>
> The external TPM emulator 'swtpm' provides a protocol over
> its control channel to retrieve its state blobs. We implement
> functions for getting and setting the different state blobs.
> In case the setting of the state blobs fails, we return a
> negative errno code to fail the start of the VM.
>
> Since we have an external TPM emulator, we need to make sure
> that we do not migrate the state for as long as it is busy
> processing a request. We need to wait for notification that
> the request has completed processing.

Because qemu will have to deal with an external state, managed by the
tpm emulator (different from other storage/nvram):

- the emulator statedir must be different between source and dest? Can
it be the same?
- what is the story regarding versionning? Can you migrate from/to any
version, or only the same version?
- can the host source and destination be of different endianess?
- how is suspend and snapshot working?

There are probably other complications that I can't think of
immediately, but David or Juan help may help avoid mistakes.

It probably deserves a few explanations in docs/specs/tpm.txt.

>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--

It would be worth to add some basic tests. Either growing our own
tests/tpm-emu.c test emulator

or checking that swtpm is present (and of required version?) to
run/skip the test a more complete test.

>  1 file changed, 302 insertions(+), 10 deletions(-)
>
> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> index b787aee..da877e5 100644
> --- a/hw/tpm/tpm_emulator.c
> +++ b/hw/tpm/tpm_emulator.c
> @@ -55,6 +55,19 @@
>  #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
>
>  /* data structures */
> +
> +/* blobs from the TPM; part of VM state when migrating */
> +typedef struct TPMBlobBuffers {
> +    uint32_t permanent_flags;
> +    TPMSizedBuffer permanent;
> +
> +    uint32_t volatil_flags;
> +    TPMSizedBuffer volatil;
> +
> +    uint32_t savestate_flags;
> +    TPMSizedBuffer savestate;
> +} TPMBlobBuffers;
> +
>  typedef struct TPMEmulator {
>      TPMBackend parent;
>
> @@ -70,6 +83,8 @@ typedef struct TPMEmulator {
>
>      unsigned int established_flag:1;
>      unsigned int established_flag_cached:1;
> +
> +    TPMBlobBuffers state_blobs;

Suspicious addition, it shouldn't be necessary in running state. It
could be allocated on demand? Even better if the field is removed, but
this may not be possible with VMSTATE macros.

>  } TPMEmulator;
>
>
> @@ -301,7 +316,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>      return 0;
>  }
>
> -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> +static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
> +                                     bool is_resume)

Using underscore-prefixed names is discouraged. Call it tpm_emulator_init?

>  {
>      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>      ptm_init init = {
> @@ -309,12 +325,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>      };
>      ptm_res res;
>
> +    DPRINTF("%s   is_resume: %d", __func__, is_resume);
> +

extra spaces, you may also add buffersize while at it.

>      if (buffersize != 0 &&
>          tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
>          goto err_exit;
>      }
>
> -    DPRINTF("%s", __func__);
> +    if (is_resume) {
> +        init.u.req.init_flags = cpu_to_be32(PTM_INIT_FLAG_DELETE_VOLATILE);
> +    }

Since it's a flags, and for future extensibility, I would suggest |=.

> +
>      if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
>                               sizeof(init)) < 0) {
>          error_report("tpm-emulator: could not send INIT: %s",
> @@ -333,6 +354,11 @@ err_exit:
>      return -1;
>  }
>
> +static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> +{
> +    return _tpm_emulator_startup_tpm(tb, buffersize, false);
> +}
> +
>  static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
>  {
>      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> @@ -431,16 +457,21 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
>  static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
>  {
>      Error *err = NULL;
> +    ptm_cap caps = PTM_CAP_GET_STATEBLOB | PTM_CAP_SET_STATEBLOB |
> +                   PTM_CAP_STOP;
>
> -    error_setg(&tpm_emu->migration_blocker,
> -               "Migration disabled: TPM emulator not yet migratable");
> -    migrate_add_blocker(tpm_emu->migration_blocker, &err);
> -    if (err) {
> -        error_report_err(err);
> -        error_free(tpm_emu->migration_blocker);
> -        tpm_emu->migration_blocker = NULL;
> +    if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_emu, caps)) {
> +        error_setg(&tpm_emu->migration_blocker,
> +                   "Migration disabled: TPM emulator does not support "
> +                   "migration");
> +        migrate_add_blocker(tpm_emu->migration_blocker, &err);
> +        if (err) {
> +            error_report_err(err);
> +            error_free(tpm_emu->migration_blocker);
> +            tpm_emu->migration_blocker = NULL;
>
> -        return -1;
> +            return -1;
> +        }
>      }
>
>      return 0;
> @@ -569,6 +600,263 @@ static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
>      { /* end of list */ },
>  };
>
> +/*
> + * Transfer a TPM state blob from the TPM into a provided buffer.
> + *
> + * @tpm_emu: TPMEmulator
> + * @type: the type of blob to transfer
> + * @tsb: the TPMSizeBuffer to fill with the blob
> + * @flags: the flags to return to the caller
> + */
> +static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
> +                                       uint8_t type,
> +                                       TPMSizedBuffer *tsb,
> +                                       uint32_t *flags)
> +{
> +    ptm_getstate pgs;
> +    ptm_res res;
> +    ssize_t n;
> +    uint32_t totlength, length;
> +
> +    tpm_sized_buffer_reset(tsb);
> +
> +    pgs.u.req.state_flags = cpu_to_be32(PTM_STATE_FLAG_DECRYPTED);
> +    pgs.u.req.type = cpu_to_be32(type);
> +    pgs.u.req.offset = 0;
> +
> +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_STATEBLOB,
> +                             &pgs, sizeof(pgs.u.req),
> +                             offsetof(ptm_getstate, u.resp.data)) < 0) {
> +        error_report("tpm-emulator: could not get state blob type %d : %s",
> +                     type, strerror(errno));
> +        return -1;

(I guess some day vmstate functions should have an Error ** argument)

> +    }
> +
> +    res = be32_to_cpu(pgs.u.resp.tpm_result);
> +    if (res != 0 && (res & 0x800) == 0) {
> +        error_report("tpm-emulator: Getting the stateblob (type %d) failed "
> +                     "with a TPM error 0x%x", type, res);
> +        return -1;
> +    }
> +
> +    totlength = be32_to_cpu(pgs.u.resp.totlength);
> +    length = be32_to_cpu(pgs.u.resp.length);
> +    if (totlength != length) {
> +        error_report("tpm-emulator: Expecting to read %u bytes "
> +                     "but would get %u", totlength, length);
> +        return -1;
> +    }
> +
> +    *flags = be32_to_cpu(pgs.u.resp.state_flags);
> +
> +    tsb->buffer = g_malloc(totlength);

That TPMSizedBuffer is not really useful. Use GArray or qemu Buffer ?
(and we could drop TPMSizedBuffer).

> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
> +    if (n != totlength) {
> +        error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
> +                     type, strerror(errno));
> +        return -1;
> +    }
> +    tsb->size = totlength;
> +
> +    DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
> +            type, tsb->size, *flags);
> +
> +    return 0;
> +}
> +
> +static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
> +{
> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
> +
> +    if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
> +                                    &state_blobs->permanent,
> +                                    &state_blobs->permanent_flags) < 0 ||
> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
> +                                    &state_blobs->volatil,
> +                                    &state_blobs->volatil_flags) < 0 ||
> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
> +                                    &state_blobs->savestate,
> +                                    &state_blobs->savestate_flags) < 0) {
> +        goto err_exit;
> +    }
> +
> +    return 0;
> +
> + err_exit:
> +    tpm_sized_buffer_reset(&state_blobs->volatil);
> +    tpm_sized_buffer_reset(&state_blobs->permanent);
> +    tpm_sized_buffer_reset(&state_blobs->savestate);
> +
> +    return -1;
> +}
> +
> +/*
> + * Transfer a TPM state blob to the TPM emulator.
> + *
> + * @tpm_emu: TPMEmulator
> + * @type: the type of TPM state blob to transfer
> + * @tsb: TPMSizeBuffer containing the TPM state blob
> + * @flags: Flags describing the (encryption) state of the TPM state blob
> + */
> +static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
> +                                       uint32_t type,
> +                                       TPMSizedBuffer *tsb,
> +                                       uint32_t flags)
> +{
> +    uint32_t offset = 0;
> +    ssize_t n;
> +    ptm_setstate pss;
> +    ptm_res tpm_result;
> +
> +    if (tsb->size == 0) {
> +        return 0;
> +    }
> +
> +    pss = (ptm_setstate) {
> +        .u.req.state_flags = cpu_to_be32(flags),
> +        .u.req.type = cpu_to_be32(type),
> +        .u.req.length = cpu_to_be32(tsb->size),
> +    };
> +
> +    /* write the header only */
> +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
> +                             offsetof(ptm_setstate, u.req.data), 0) < 0) {
> +        error_report("tpm-emulator: could not set state blob type %d : %s",
> +                     type, strerror(errno));
> +        return -1;
> +    }
> +
> +    /* now the body */
> +    n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
> +                              &tsb->buffer[offset], tsb->size);
> +    if (n != tsb->size) {
> +        error_report("tpm-emulator: Writing the stateblob (type %d) "
> +                     "failed: %s", type, strerror(errno));
> +        return -1;
> +    }
> +
> +    /* now get the result */
> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
> +                             (uint8_t *)&pss, sizeof(pss.u.resp));
> +    if (n != sizeof(pss.u.resp)) {
> +        error_report("tpm-emulator: Reading response from writing stateblob "
> +                     "(type %d) failed: %s", type, strerror(errno));
> +        return -1;
> +    }
> +
> +    tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
> +    if (tpm_result != 0) {
> +        error_report("tpm-emulator: Setting the stateblob (type %d) failed "
> +                     "with a TPM error 0x%x", type, tpm_result);
> +        return -1;
> +    }
> +
> +    DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
> +            type, tsb->size, flags);
> +
> +    return 0;
> +}
> +
> +/*
> + * Set all the TPM state blobs.
> + *
> + * Returns a negative errno code in case of error.
> + */
> +static int tpm_emulator_set_state_blobs(TPMBackend *tb)
> +{
> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
> +
> +    DPRINTF("%s: %d", __func__, __LINE__);
> +
> +    if (tpm_emulator_stop_tpm(tb) < 0) {
> +        return -EIO;
> +    }
> +
> +    if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
> +                                    &state_blobs->permanent,
> +                                    state_blobs->permanent_flags) < 0 ||
> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
> +                                    &state_blobs->volatil,
> +                                    state_blobs->volatil_flags) < 0 ||
> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
> +                                    &state_blobs->savestate,
> +                                    state_blobs->savestate_flags) < 0) {
> +        return -EBADMSG;
> +    }
> +
> +    DPRINTF("DONE SETTING STATEBLOBS");

UPPERCASES!

> +
> +    return 0;
> +}
> +
> +static int tpm_emulator_pre_save(void *opaque)
> +{
> +    TPMBackend *tb = opaque;
> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> +
> +    DPRINTF("%s", __func__);
> +
> +    tpm_backend_finish_sync(tb);
> +
> +    /* get the state blobs from the TPM */
> +    return tpm_emulator_get_state_blobs(tpm_emu);
> +}
> +
> +/*
> + * Load the TPM state blobs into the TPM.
> + *
> + * Returns negative errno codes in case of error.
> + */
> +static int tpm_emulator_post_load(void *opaque,
> +                                  int version_id __attribute__((unused)))

unnecessary attribute

> +{
> +    TPMBackend *tb = opaque;
> +    int ret;
> +
> +    ret = tpm_emulator_set_state_blobs(tb);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    if (_tpm_emulator_startup_tpm(tb, 0, true) < 0) {
> +        return -EIO;
> +    }
> +

I dislike the mix of functions returning errno and others, and the
lack of Error **, but it's not specific to this patch. Ok.  :)

> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_tpm_emulator = {
> +    .name = "tpm-emulator",
> +    .version_id = 1,
> +    .minimum_version_id = 0,

version_id = 1 & minimum_version_id = 0 ?

It's the first version, let's have version_id = 0 (and you could
remove minimum_version).

> +    .minimum_version_id_old = 0,

No need to specify that, no load_state_old() either

> +    .pre_save = tpm_emulator_pre_save,
> +    .post_load = tpm_emulator_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
> +        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
> +                                     TPMEmulator, 1, 0,
> +                                     state_blobs.permanent.size),
> +
> +        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
> +        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
> +                                     TPMEmulator, 1, 0,
> +                                     state_blobs.volatil.size),
> +
> +        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
> +        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
> +                                     TPMEmulator, 1, 0,
> +                                     state_blobs.savestate.size),
> +
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void tpm_emulator_inst_init(Object *obj)
>  {
>      TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
> @@ -577,6 +865,8 @@ static void tpm_emulator_inst_init(Object *obj)
>      tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
>      tpm_emu->cur_locty_number = ~0;
>      qemu_mutex_init(&tpm_emu->mutex);
> +
> +    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
>  }
>
>  /*
> @@ -613,6 +903,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
>      }
>
>      qemu_mutex_destroy(&tpm_emu->mutex);
> +
> +    vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);
>  }
>
>  static void tpm_emulator_class_init(ObjectClass *klass, void *data)
> --
> 2.5.5
>
>

looks good to me overall

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v4 2/2] tpm: extend TPM TIS with state migration support

[Marc-André Lureau]

Hi

On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Extend the TPM TIS interface with state migration support.
>
> We need to synchronize with the backend thread to make sure that a command
> being processed by the external TPM emulator has completed and its
> response been received.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  hw/tpm/tpm_tis.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 834eef7..5016d28 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -902,9 +902,61 @@ static void tpm_tis_reset(DeviceState *dev)
>      tpm_backend_startup_tpm(s->be_driver, s->be_buffer_size);
>  }
>
> +/* persistent state handling */
> +
> +static int tpm_tis_pre_save(void *opaque)
> +{
> +    TPMState *s = opaque;
> +    uint8_t locty = s->active_locty;
> +
> +    DPRINTF("tpm_tis: suspend: locty = %d : rw_offset = %u\n",

suspend -> pre_save ?

> +            locty, s->rw_offset);
> +#ifdef DEBUG_TIS
> +    tpm_tis_dump_state(opaque, 0);
> +#endif

To avoid dead code, you could write:

if (DEBUG_TIS) {
...
}


> +
> +    /*
> +     * Synchronize with backend completion.
> +     */
> +    tpm_backend_finish_sync(s->be_driver);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_locty = {
> +    .name = "loc",

I would use a more descriptive name, such as "tpm-tis/locty"

> +    .version_id = 1,
> +    .minimum_version_id = 0,

It's the first version, make it all 0.

> +    .minimum_version_id_old = 0,

not needed

> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT32(state, TPMLocality),
> +        VMSTATE_UINT32(inte, TPMLocality),
> +        VMSTATE_UINT32(ints, TPMLocality),
> +        VMSTATE_UINT8(access, TPMLocality),
> +        VMSTATE_UINT32(sts, TPMLocality),
> +        VMSTATE_UINT32(iface_id, TPMLocality),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +
>  static const VMStateDescription vmstate_tpm_tis = {
>      .name = "tpm",

It's time to use a more specific name "tpm-tis" ?

> -    .unmigratable = 1,
> +    .version_id = 1,
> +    .minimum_version_id = 0,

It's the first version, make it all 0.

> +    .minimum_version_id_old = 0,

not needed

> +    .pre_save  = tpm_tis_pre_save,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BUFFER(buffer, TPMState),
> +        VMSTATE_UINT16(rw_offset, TPMState),
> +        VMSTATE_UINT8(active_locty, TPMState),
> +        VMSTATE_UINT8(aborting_locty, TPMState),
> +        VMSTATE_UINT8(next_locty, TPMState),
> +
> +        VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 1,
> +                             vmstate_locty, TPMLocality),
> +
> +        VMSTATE_END_OF_LIST()
> +    }
>  };
>
>  static Property tpm_tis_properties[] = {
> --
> 2.5.5
>
>

looks good otherwise

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> Hi Stefan
> 
> On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
> > Extend the TPM emulator backend device with state migration support.
> >
> > The external TPM emulator 'swtpm' provides a protocol over
> > its control channel to retrieve its state blobs. We implement
> > functions for getting and setting the different state blobs.
> > In case the setting of the state blobs fails, we return a
> > negative errno code to fail the start of the VM.
> >
> > Since we have an external TPM emulator, we need to make sure
> > that we do not migrate the state for as long as it is busy
> > processing a request. We need to wait for notification that
> > the request has completed processing.
> 
> Because qemu will have to deal with an external state, managed by the
> tpm emulator (different from other storage/nvram):
> 
> - the emulator statedir must be different between source and dest? Can
> it be the same?
> - what is the story regarding versionning? Can you migrate from/to any
> version, or only the same version?
> - can the host source and destination be of different endianess?
> - how is suspend and snapshot working?
> 
> There are probably other complications that I can't think of
> immediately, but David or Juan help may help avoid mistakes.

Yes, I think that just about covers it.

> It probably deserves a few explanations in docs/specs/tpm.txt.
> 
> >
> > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > ---
> >  hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--
> 
> It would be worth to add some basic tests. Either growing our own
> tests/tpm-emu.c test emulator
> 
> or checking that swtpm is present (and of required version?) to
> run/skip the test a more complete test.
> 
> >  1 file changed, 302 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> > index b787aee..da877e5 100644
> > --- a/hw/tpm/tpm_emulator.c
> > +++ b/hw/tpm/tpm_emulator.c
> > @@ -55,6 +55,19 @@
> >  #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
> >
> >  /* data structures */
> > +
> > +/* blobs from the TPM; part of VM state when migrating */
> > +typedef struct TPMBlobBuffers {
> > +    uint32_t permanent_flags;
> > +    TPMSizedBuffer permanent;
> > +
> > +    uint32_t volatil_flags;
> > +    TPMSizedBuffer volatil;
> > +
> > +    uint32_t savestate_flags;
> > +    TPMSizedBuffer savestate;
> > +} TPMBlobBuffers;
> > +
> >  typedef struct TPMEmulator {
> >      TPMBackend parent;
> >
> > @@ -70,6 +83,8 @@ typedef struct TPMEmulator {
> >
> >      unsigned int established_flag:1;
> >      unsigned int established_flag_cached:1;
> > +
> > +    TPMBlobBuffers state_blobs;
> 
> Suspicious addition, it shouldn't be necessary in running state. It
> could be allocated on demand? Even better if the field is removed, but
> this may not be possible with VMSTATE macros.
> 
> >  } TPMEmulator;
> >
> >
> > @@ -301,7 +316,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
> >      return 0;
> >  }
> >
> > -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> > +static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
> > +                                     bool is_resume)
> 
> Using underscore-prefixed names is discouraged. Call it tpm_emulator_init?
> 
> >  {
> >      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> >      ptm_init init = {
> > @@ -309,12 +325,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> >      };
> >      ptm_res res;
> >
> > +    DPRINTF("%s   is_resume: %d", __func__, is_resume);
> > +
>
> extra spaces, you may also add buffersize while at it.

Also it would be good to use trace_'s where possible rather than
DPRINTFs.

> 
> >      if (buffersize != 0 &&
> >          tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
> >          goto err_exit;
> >      }
> >
> > -    DPRINTF("%s", __func__);
> > +    if (is_resume) {
> > +        init.u.req.init_flags = cpu_to_be32(PTM_INIT_FLAG_DELETE_VOLATILE);
> > +    }
> 
> Since it's a flags, and for future extensibility, I would suggest |=.
> 
> > +
> >      if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
> >                               sizeof(init)) < 0) {
> >          error_report("tpm-emulator: could not send INIT: %s",
> > @@ -333,6 +354,11 @@ err_exit:
> >      return -1;
> >  }
> >
> > +static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> > +{
> > +    return _tpm_emulator_startup_tpm(tb, buffersize, false);
> > +}
> > +
> >  static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
> >  {
> >      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> > @@ -431,16 +457,21 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
> >  static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
> >  {
> >      Error *err = NULL;
> > +    ptm_cap caps = PTM_CAP_GET_STATEBLOB | PTM_CAP_SET_STATEBLOB |
> > +                   PTM_CAP_STOP;
> >
> > -    error_setg(&tpm_emu->migration_blocker,
> > -               "Migration disabled: TPM emulator not yet migratable");
> > -    migrate_add_blocker(tpm_emu->migration_blocker, &err);
> > -    if (err) {
> > -        error_report_err(err);
> > -        error_free(tpm_emu->migration_blocker);
> > -        tpm_emu->migration_blocker = NULL;
> > +    if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_emu, caps)) {
> > +        error_setg(&tpm_emu->migration_blocker,
> > +                   "Migration disabled: TPM emulator does not support "
> > +                   "migration");
> > +        migrate_add_blocker(tpm_emu->migration_blocker, &err);
> > +        if (err) {
> > +            error_report_err(err);
> > +            error_free(tpm_emu->migration_blocker);
> > +            tpm_emu->migration_blocker = NULL;
> >
> > -        return -1;
> > +            return -1;
> > +        }
> >      }
> >
> >      return 0;
> > @@ -569,6 +600,263 @@ static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
> >      { /* end of list */ },
> >  };
> >
> > +/*
> > + * Transfer a TPM state blob from the TPM into a provided buffer.
> > + *
> > + * @tpm_emu: TPMEmulator
> > + * @type: the type of blob to transfer
> > + * @tsb: the TPMSizeBuffer to fill with the blob
> > + * @flags: the flags to return to the caller
> > + */
> > +static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
> > +                                       uint8_t type,
> > +                                       TPMSizedBuffer *tsb,
> > +                                       uint32_t *flags)
> > +{
> > +    ptm_getstate pgs;
> > +    ptm_res res;
> > +    ssize_t n;
> > +    uint32_t totlength, length;
> > +
> > +    tpm_sized_buffer_reset(tsb);
> > +
> > +    pgs.u.req.state_flags = cpu_to_be32(PTM_STATE_FLAG_DECRYPTED);
> > +    pgs.u.req.type = cpu_to_be32(type);
> > +    pgs.u.req.offset = 0;
> > +
> > +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_STATEBLOB,
> > +                             &pgs, sizeof(pgs.u.req),
> > +                             offsetof(ptm_getstate, u.resp.data)) < 0) {
> > +        error_report("tpm-emulator: could not get state blob type %d : %s",
> > +                     type, strerror(errno));
> > +        return -1;
> 
> (I guess some day vmstate functions should have an Error ** argument)
> 
> > +    }
> > +
> > +    res = be32_to_cpu(pgs.u.resp.tpm_result);
> > +    if (res != 0 && (res & 0x800) == 0) {
> > +        error_report("tpm-emulator: Getting the stateblob (type %d) failed "
> > +                     "with a TPM error 0x%x", type, res);
> > +        return -1;
> > +    }
> > +
> > +    totlength = be32_to_cpu(pgs.u.resp.totlength);
> > +    length = be32_to_cpu(pgs.u.resp.length);
> > +    if (totlength != length) {
> > +        error_report("tpm-emulator: Expecting to read %u bytes "
> > +                     "but would get %u", totlength, length);
> > +        return -1;
> > +    }
> > +
> > +    *flags = be32_to_cpu(pgs.u.resp.state_flags);
> > +
> > +    tsb->buffer = g_malloc(totlength);
> 
> That TPMSizedBuffer is not really useful. Use GArray or qemu Buffer ?
> (and we could drop TPMSizedBuffer).

Also, given this is an arbitrary sized chunk, this should probably use
g_try_malloc and check the result rather than letting g_malloc assert on
failure (especially true on the source side).

> > +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
> > +    if (n != totlength) {
> > +        error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
> > +                     type, strerror(errno));
> > +        return -1;
> > +    }
> > +    tsb->size = totlength;
> > +
> > +    DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
> > +            type, tsb->size, *flags);
> > +
> > +    return 0;
> > +}
> > +
> > +static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
> > +{
> > +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
> > +
> > +    if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
> > +                                    &state_blobs->permanent,
> > +                                    &state_blobs->permanent_flags) < 0 ||
> > +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
> > +                                    &state_blobs->volatil,
> > +                                    &state_blobs->volatil_flags) < 0 ||
> > +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
> > +                                    &state_blobs->savestate,
> > +                                    &state_blobs->savestate_flags) < 0) {
> > +        goto err_exit;
> > +    }
> > +
> > +    return 0;
> > +
> > + err_exit:
> > +    tpm_sized_buffer_reset(&state_blobs->volatil);
> > +    tpm_sized_buffer_reset(&state_blobs->permanent);
> > +    tpm_sized_buffer_reset(&state_blobs->savestate);
> > +
> > +    return -1;
> > +}
> > +
> > +/*
> > + * Transfer a TPM state blob to the TPM emulator.
> > + *
> > + * @tpm_emu: TPMEmulator
> > + * @type: the type of TPM state blob to transfer
> > + * @tsb: TPMSizeBuffer containing the TPM state blob
> > + * @flags: Flags describing the (encryption) state of the TPM state blob
> > + */
> > +static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
> > +                                       uint32_t type,
> > +                                       TPMSizedBuffer *tsb,
> > +                                       uint32_t flags)
> > +{
> > +    uint32_t offset = 0;
> > +    ssize_t n;
> > +    ptm_setstate pss;
> > +    ptm_res tpm_result;
> > +
> > +    if (tsb->size == 0) {
> > +        return 0;
> > +    }
> > +
> > +    pss = (ptm_setstate) {
> > +        .u.req.state_flags = cpu_to_be32(flags),
> > +        .u.req.type = cpu_to_be32(type),
> > +        .u.req.length = cpu_to_be32(tsb->size),
> > +    };
> > +
> > +    /* write the header only */
> > +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
> > +                             offsetof(ptm_setstate, u.req.data), 0) < 0) {
> > +        error_report("tpm-emulator: could not set state blob type %d : %s",
> > +                     type, strerror(errno));
> > +        return -1;
> > +    }
> > +
> > +    /* now the body */
> > +    n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
> > +                              &tsb->buffer[offset], tsb->size);
> > +    if (n != tsb->size) {
> > +        error_report("tpm-emulator: Writing the stateblob (type %d) "
> > +                     "failed: %s", type, strerror(errno));
> > +        return -1;
> > +    }
> > +
> > +    /* now get the result */
> > +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
> > +                             (uint8_t *)&pss, sizeof(pss.u.resp));
> > +    if (n != sizeof(pss.u.resp)) {
> > +        error_report("tpm-emulator: Reading response from writing stateblob "
> > +                     "(type %d) failed: %s", type, strerror(errno));
> > +        return -1;
> > +    }
> > +
> > +    tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
> > +    if (tpm_result != 0) {
> > +        error_report("tpm-emulator: Setting the stateblob (type %d) failed "
> > +                     "with a TPM error 0x%x", type, tpm_result);
> > +        return -1;
> > +    }
> > +
> > +    DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
> > +            type, tsb->size, flags);
> > +
> > +    return 0;
> > +}
> > +
> > +/*
> > + * Set all the TPM state blobs.
> > + *
> > + * Returns a negative errno code in case of error.
> > + */
> > +static int tpm_emulator_set_state_blobs(TPMBackend *tb)
> > +{
> > +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> > +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
> > +
> > +    DPRINTF("%s: %d", __func__, __LINE__);
> > +
> > +    if (tpm_emulator_stop_tpm(tb) < 0) {
> > +        return -EIO;
> > +    }
> > +
> > +    if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
> > +                                    &state_blobs->permanent,
> > +                                    state_blobs->permanent_flags) < 0 ||
> > +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
> > +                                    &state_blobs->volatil,
> > +                                    state_blobs->volatil_flags) < 0 ||
> > +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
> > +                                    &state_blobs->savestate,
> > +                                    state_blobs->savestate_flags) < 0) {
> > +        return -EBADMSG;
> > +    }
> > +
> > +    DPRINTF("DONE SETTING STATEBLOBS");
> 
> UPPERCASES!
> 
> > +
> > +    return 0;
> > +}
> > +
> > +static int tpm_emulator_pre_save(void *opaque)
> > +{
> > +    TPMBackend *tb = opaque;
> > +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> > +
> > +    DPRINTF("%s", __func__);
> > +
> > +    tpm_backend_finish_sync(tb);
> > +
> > +    /* get the state blobs from the TPM */
> > +    return tpm_emulator_get_state_blobs(tpm_emu);
> > +}
> > +
> > +/*
> > + * Load the TPM state blobs into the TPM.
> > + *
> > + * Returns negative errno codes in case of error.
> > + */
> > +static int tpm_emulator_post_load(void *opaque,
> > +                                  int version_id __attribute__((unused)))
> 
> unnecessary attribute
> 
> > +{
> > +    TPMBackend *tb = opaque;
> > +    int ret;
> > +
> > +    ret = tpm_emulator_set_state_blobs(tb);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    if (_tpm_emulator_startup_tpm(tb, 0, true) < 0) {
> > +        return -EIO;
> > +    }
> > +
> 
> I dislike the mix of functions returning errno and others, and the
> lack of Error **, but it's not specific to this patch. Ok.  :)
> 
> > +    return 0;
> > +}
> > +
> > +static const VMStateDescription vmstate_tpm_emulator = {
> > +    .name = "tpm-emulator",
> > +    .version_id = 1,
> > +    .minimum_version_id = 0,
> 
> version_id = 1 & minimum_version_id = 0 ?
> 
> It's the first version, let's have version_id = 0 (and you could
> remove minimum_version).
> 
> > +    .minimum_version_id_old = 0,
> 
> No need to specify that, no load_state_old() either
> 
> > +    .pre_save = tpm_emulator_pre_save,
> > +    .post_load = tpm_emulator_post_load,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
> > +        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
> > +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
> > +                                     TPMEmulator, 1, 0,
> > +                                     state_blobs.permanent.size),
> > +
> > +        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
> > +        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
> > +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
> > +                                     TPMEmulator, 1, 0,
> > +                                     state_blobs.volatil.size),
> > +
> > +        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
> > +        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
> > +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
> > +                                     TPMEmulator, 1, 0,
> > +                                     state_blobs.savestate.size),
> > +
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static void tpm_emulator_inst_init(Object *obj)
> >  {
> >      TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
> > @@ -577,6 +865,8 @@ static void tpm_emulator_inst_init(Object *obj)
> >      tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
> >      tpm_emu->cur_locty_number = ~0;
> >      qemu_mutex_init(&tpm_emu->mutex);
> > +
> > +    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
> >  }
> >
> >  /*
> > @@ -613,6 +903,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
> >      }
> >
> >      qemu_mutex_destroy(&tpm_emu->mutex);
> > +
> > +    vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);

It's best to avoid the vmstate_register/unregister and just set the
dc->vmsd pointer during class_init; see hw/virtio/virtio-balloon.c
virtio_balloon_class_init for an example.

Dave
> >  }
> >
> >  static void tpm_emulator_class_init(ObjectClass *klass, void *data)
> > --
> > 2.5.5
> >
> >
> 
> looks good to me overall
> 
> -- 
> Marc-André Lureau
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Stefan Berger]

On 03/02/2018 04:26 AM, Marc-André Lureau wrote:
> Hi Stefan
>
> On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> Extend the TPM emulator backend device with state migration support.
>>
>> The external TPM emulator 'swtpm' provides a protocol over
>> its control channel to retrieve its state blobs. We implement
>> functions for getting and setting the different state blobs.
>> In case the setting of the state blobs fails, we return a
>> negative errno code to fail the start of the VM.
>>
>> Since we have an external TPM emulator, we need to make sure
>> that we do not migrate the state for as long as it is busy
>> processing a request. We need to wait for notification that
>> the request has completed processing.
> Because qemu will have to deal with an external state, managed by the
> tpm emulator (different from other storage/nvram):
>
> - the emulator statedir must be different between source and dest? Can
> it be the same?

You mean for local migration? I would suggest it be different 
directories though it may work with the same. I haven't tried it like 
that and the critical case here is local migration between different 
versions of the swtpm and for example downgrading of state from v3 to v1 
being refused.

> - what is the story regarding versionning? Can you migrate from/to any
> version, or only the same version?

I just redid the state marshalling/unmarshalling of the TPM 2 
implementation. All structures that are written out are ID'ed and 
versioned and accomodate compile-time optional parts. As for migration 
between different versions, I am not sure what to promise about this 
right now. So if you migrate a structure of the TPM 2 from v1 to v2 and 
v2 has some new fields that v1 didn't have, we need default values to 
assign to these new fields, which may or may not be possible depending 
on circumstances and semantics of the fields. In the end it comes down 
to upgradeability of the TPM 2 implementation. The current 
implementation supports revision 146 of the TPM 2 specs. I cannot 
predict the future and what future versions may add to the code and the 
state of the device. Design/development of the TPM 2 is ongoing while 
companies are producing TPM 2 chips following specs of a certain time. 
They also have that problem of having to upgrade a device to a later 
spec but could refuse to do so and tell you to buy a new machine. So in 
our case it may actually be more difficult with the expected upgrade 
path. If the state cannot be upgraded because default values cannot be 
found, we would have to stop at a certain revision.

> - can the host source and destination be of different endianess?

Yes. All state is marshalled in big endian format. I have test cases in 
swtpm that cover that. State produced on a x86_64 can be consumed on 
ppc64 big endian.

> - how is suspend and snapshot working?

The tpm_emulator backend uses the same mechanisms to store state as all 
the other devices do, except that we retrieve the state via .pre_save 
from an external device where we fill buffers that are part of the 
'normal' state of the device. You have come across the TPMSizedBuffers 
that are being marshalled and those carry that state.

I have tried local migration to a file as well as local migration via 
netcat. Both work.

>
> There are probably other complications that I can't think of
> immediately, but David or Juan help may help avoid mistakes.
>
> It probably deserves a few explanations in docs/specs/tpm.txt.
>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--
> It would be worth to add some basic tests. Either growing our own
> tests/tpm-emu.c test emulator
>
> or checking that swtpm is present (and of required version?) to
> run/skip the test a more complete test.

Ok, I'll have to look into that. I do have several test cases in swtpm 
for that already.

>
>>   1 file changed, 302 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
>> index b787aee..da877e5 100644
>> --- a/hw/tpm/tpm_emulator.c
>> +++ b/hw/tpm/tpm_emulator.c
>> @@ -55,6 +55,19 @@
>>   #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
>>
>>   /* data structures */
>> +
>> +/* blobs from the TPM; part of VM state when migrating */
>> +typedef struct TPMBlobBuffers {
>> +    uint32_t permanent_flags;
>> +    TPMSizedBuffer permanent;
>> +
>> +    uint32_t volatil_flags;
>> +    TPMSizedBuffer volatil;
>> +
>> +    uint32_t savestate_flags;
>> +    TPMSizedBuffer savestate;
>> +} TPMBlobBuffers;
>> +
>>   typedef struct TPMEmulator {
>>       TPMBackend parent;
>>
>> @@ -70,6 +83,8 @@ typedef struct TPMEmulator {
>>
>>       unsigned int established_flag:1;
>>       unsigned int established_flag_cached:1;
>> +
>> +    TPMBlobBuffers state_blobs;
> Suspicious addition, it shouldn't be necessary in running state. It
> could be allocated on demand? Even better if the field is removed, but
> this may not be possible with VMSTATE macros.

This is the field that will carry the external devices' state blobs.

>>   } TPMEmulator;
>>
>>
>> @@ -301,7 +316,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>>       return 0;
>>   }
>>
>> -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>> +static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
>> +                                     bool is_resume)
> Using underscore-prefixed names is discouraged. Call it tpm_emulator_init?
>
>>   {
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>       ptm_init init = {
>> @@ -309,12 +325,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>>       };
>>       ptm_res res;
>>
>> +    DPRINTF("%s   is_resume: %d", __func__, is_resume);
>> +
> extra spaces, you may also add buffersize while at it.
>
>>       if (buffersize != 0 &&
>>           tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
>>           goto err_exit;
>>       }
>>
>> -    DPRINTF("%s", __func__);
>> +    if (is_resume) {
>> +        init.u.req.init_flags = cpu_to_be32(PTM_INIT_FLAG_DELETE_VOLATILE);
>> +    }
> Since it's a flags, and for future extensibility, I would suggest |=.
>
>> +
>>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
>>                                sizeof(init)) < 0) {
>>           error_report("tpm-emulator: could not send INIT: %s",
>> @@ -333,6 +354,11 @@ err_exit:
>>       return -1;
>>   }
>>
>> +static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>> +{
>> +    return _tpm_emulator_startup_tpm(tb, buffersize, false);
>> +}
>> +
>>   static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
>>   {
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>> @@ -431,16 +457,21 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
>>   static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
>>   {
>>       Error *err = NULL;
>> +    ptm_cap caps = PTM_CAP_GET_STATEBLOB | PTM_CAP_SET_STATEBLOB |
>> +                   PTM_CAP_STOP;
>>
>> -    error_setg(&tpm_emu->migration_blocker,
>> -               "Migration disabled: TPM emulator not yet migratable");
>> -    migrate_add_blocker(tpm_emu->migration_blocker, &err);
>> -    if (err) {
>> -        error_report_err(err);
>> -        error_free(tpm_emu->migration_blocker);
>> -        tpm_emu->migration_blocker = NULL;
>> +    if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_emu, caps)) {
>> +        error_setg(&tpm_emu->migration_blocker,
>> +                   "Migration disabled: TPM emulator does not support "
>> +                   "migration");
>> +        migrate_add_blocker(tpm_emu->migration_blocker, &err);
>> +        if (err) {
>> +            error_report_err(err);
>> +            error_free(tpm_emu->migration_blocker);
>> +            tpm_emu->migration_blocker = NULL;
>>
>> -        return -1;
>> +            return -1;
>> +        }
>>       }
>>
>>       return 0;
>> @@ -569,6 +600,263 @@ static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
>>       { /* end of list */ },
>>   };
>>
>> +/*
>> + * Transfer a TPM state blob from the TPM into a provided buffer.
>> + *
>> + * @tpm_emu: TPMEmulator
>> + * @type: the type of blob to transfer
>> + * @tsb: the TPMSizeBuffer to fill with the blob
>> + * @flags: the flags to return to the caller
>> + */
>> +static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
>> +                                       uint8_t type,
>> +                                       TPMSizedBuffer *tsb,
>> +                                       uint32_t *flags)
>> +{
>> +    ptm_getstate pgs;
>> +    ptm_res res;
>> +    ssize_t n;
>> +    uint32_t totlength, length;
>> +
>> +    tpm_sized_buffer_reset(tsb);
>> +
>> +    pgs.u.req.state_flags = cpu_to_be32(PTM_STATE_FLAG_DECRYPTED);
>> +    pgs.u.req.type = cpu_to_be32(type);
>> +    pgs.u.req.offset = 0;
>> +
>> +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_STATEBLOB,
>> +                             &pgs, sizeof(pgs.u.req),
>> +                             offsetof(ptm_getstate, u.resp.data)) < 0) {
>> +        error_report("tpm-emulator: could not get state blob type %d : %s",
>> +                     type, strerror(errno));
>> +        return -1;
> (I guess some day vmstate functions should have an Error ** argument)
>
>> +    }
>> +
>> +    res = be32_to_cpu(pgs.u.resp.tpm_result);
>> +    if (res != 0 && (res & 0x800) == 0) {
>> +        error_report("tpm-emulator: Getting the stateblob (type %d) failed "
>> +                     "with a TPM error 0x%x", type, res);
>> +        return -1;
>> +    }
>> +
>> +    totlength = be32_to_cpu(pgs.u.resp.totlength);
>> +    length = be32_to_cpu(pgs.u.resp.length);
>> +    if (totlength != length) {
>> +        error_report("tpm-emulator: Expecting to read %u bytes "
>> +                     "but would get %u", totlength, length);
>> +        return -1;
>> +    }
>> +
>> +    *flags = be32_to_cpu(pgs.u.resp.state_flags);
>> +
>> +    tsb->buffer = g_malloc(totlength);
> That TPMSizedBuffer is not really useful. Use GArray or qemu Buffer ?
> (and we could drop TPMSizedBuffer).
>
>> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
>> +    if (n != totlength) {
>> +        error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
>> +                     type, strerror(errno));
>> +        return -1;
>> +    }
>> +    tsb->size = totlength;
>> +
>> +    DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
>> +            type, tsb->size, *flags);
>> +
>> +    return 0;
>> +}
>> +
>> +static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
>> +{
>> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
>> +
>> +    if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
>> +                                    &state_blobs->permanent,
>> +                                    &state_blobs->permanent_flags) < 0 ||
>> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
>> +                                    &state_blobs->volatil,
>> +                                    &state_blobs->volatil_flags) < 0 ||
>> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
>> +                                    &state_blobs->savestate,
>> +                                    &state_blobs->savestate_flags) < 0) {
>> +        goto err_exit;
>> +    }
>> +
>> +    return 0;
>> +
>> + err_exit:
>> +    tpm_sized_buffer_reset(&state_blobs->volatil);
>> +    tpm_sized_buffer_reset(&state_blobs->permanent);
>> +    tpm_sized_buffer_reset(&state_blobs->savestate);
>> +
>> +    return -1;
>> +}
>> +
>> +/*
>> + * Transfer a TPM state blob to the TPM emulator.
>> + *
>> + * @tpm_emu: TPMEmulator
>> + * @type: the type of TPM state blob to transfer
>> + * @tsb: TPMSizeBuffer containing the TPM state blob
>> + * @flags: Flags describing the (encryption) state of the TPM state blob
>> + */
>> +static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
>> +                                       uint32_t type,
>> +                                       TPMSizedBuffer *tsb,
>> +                                       uint32_t flags)
>> +{
>> +    uint32_t offset = 0;
>> +    ssize_t n;
>> +    ptm_setstate pss;
>> +    ptm_res tpm_result;
>> +
>> +    if (tsb->size == 0) {
>> +        return 0;
>> +    }
>> +
>> +    pss = (ptm_setstate) {
>> +        .u.req.state_flags = cpu_to_be32(flags),
>> +        .u.req.type = cpu_to_be32(type),
>> +        .u.req.length = cpu_to_be32(tsb->size),
>> +    };
>> +
>> +    /* write the header only */
>> +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
>> +                             offsetof(ptm_setstate, u.req.data), 0) < 0) {
>> +        error_report("tpm-emulator: could not set state blob type %d : %s",
>> +                     type, strerror(errno));
>> +        return -1;
>> +    }
>> +
>> +    /* now the body */
>> +    n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
>> +                              &tsb->buffer[offset], tsb->size);
>> +    if (n != tsb->size) {
>> +        error_report("tpm-emulator: Writing the stateblob (type %d) "
>> +                     "failed: %s", type, strerror(errno));
>> +        return -1;
>> +    }
>> +
>> +    /* now get the result */
>> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
>> +                             (uint8_t *)&pss, sizeof(pss.u.resp));
>> +    if (n != sizeof(pss.u.resp)) {
>> +        error_report("tpm-emulator: Reading response from writing stateblob "
>> +                     "(type %d) failed: %s", type, strerror(errno));
>> +        return -1;
>> +    }
>> +
>> +    tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
>> +    if (tpm_result != 0) {
>> +        error_report("tpm-emulator: Setting the stateblob (type %d) failed "
>> +                     "with a TPM error 0x%x", type, tpm_result);
>> +        return -1;
>> +    }
>> +
>> +    DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
>> +            type, tsb->size, flags);
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Set all the TPM state blobs.
>> + *
>> + * Returns a negative errno code in case of error.
>> + */
>> +static int tpm_emulator_set_state_blobs(TPMBackend *tb)
>> +{
>> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
>> +
>> +    DPRINTF("%s: %d", __func__, __LINE__);
>> +
>> +    if (tpm_emulator_stop_tpm(tb) < 0) {
>> +        return -EIO;
>> +    }
>> +
>> +    if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
>> +                                    &state_blobs->permanent,
>> +                                    state_blobs->permanent_flags) < 0 ||
>> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
>> +                                    &state_blobs->volatil,
>> +                                    state_blobs->volatil_flags) < 0 ||
>> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
>> +                                    &state_blobs->savestate,
>> +                                    state_blobs->savestate_flags) < 0) {
>> +        return -EBADMSG;
>> +    }
>> +
>> +    DPRINTF("DONE SETTING STATEBLOBS");
> UPPERCASES!
>
>> +
>> +    return 0;
>> +}
>> +
>> +static int tpm_emulator_pre_save(void *opaque)
>> +{
>> +    TPMBackend *tb = opaque;
>> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>> +
>> +    DPRINTF("%s", __func__);
>> +
>> +    tpm_backend_finish_sync(tb);
>> +
>> +    /* get the state blobs from the TPM */
>> +    return tpm_emulator_get_state_blobs(tpm_emu);
>> +}
>> +
>> +/*
>> + * Load the TPM state blobs into the TPM.
>> + *
>> + * Returns negative errno codes in case of error.
>> + */
>> +static int tpm_emulator_post_load(void *opaque,
>> +                                  int version_id __attribute__((unused)))
> unnecessary attribute
>
>> +{
>> +    TPMBackend *tb = opaque;
>> +    int ret;
>> +
>> +    ret = tpm_emulator_set_state_blobs(tb);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    if (_tpm_emulator_startup_tpm(tb, 0, true) < 0) {
>> +        return -EIO;
>> +    }
>> +
> I dislike the mix of functions returning errno and others, and the
> lack of Error **, but it's not specific to this patch. Ok.  :)
>
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_tpm_emulator = {
>> +    .name = "tpm-emulator",
>> +    .version_id = 1,
>> +    .minimum_version_id = 0,
> version_id = 1 & minimum_version_id = 0 ?
>
> It's the first version, let's have version_id = 0 (and you could
> remove minimum_version).
>
>> +    .minimum_version_id_old = 0,
> No need to specify that, no load_state_old() either
>
>> +    .pre_save = tpm_emulator_pre_save,
>> +    .post_load = tpm_emulator_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
>> +        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
>> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
>> +                                     TPMEmulator, 1, 0,
>> +                                     state_blobs.permanent.size),
>> +
>> +        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
>> +        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
>> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
>> +                                     TPMEmulator, 1, 0,
>> +                                     state_blobs.volatil.size),
>> +
>> +        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
>> +        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
>> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
>> +                                     TPMEmulator, 1, 0,
>> +                                     state_blobs.savestate.size),
>> +
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static void tpm_emulator_inst_init(Object *obj)
>>   {
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
>> @@ -577,6 +865,8 @@ static void tpm_emulator_inst_init(Object *obj)
>>       tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
>>       tpm_emu->cur_locty_number = ~0;
>>       qemu_mutex_init(&tpm_emu->mutex);
>> +
>> +    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
>>   }
>>
>>   /*
>> @@ -613,6 +903,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
>>       }
>>
>>       qemu_mutex_destroy(&tpm_emu->mutex);
>> +
>> +    vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);
>>   }
>>
>>   static void tpm_emulator_class_init(ObjectClass *klass, void *data)
>> --
>> 2.5.5
>>
>>
> looks good to me overall
>
Thanks for the review. I'll fix your concerns.


    Stefan4

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Stefan Berger]

On 03/02/2018 04:26 AM, Marc-André Lureau wrote:
> Hi Stefan
>
> On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> Extend the TPM emulator backend device with state migration support.
>>
>> The external TPM emulator 'swtpm' provides a protocol over
>> its control channel to retrieve its state blobs. We implement
>> functions for getting and setting the different state blobs.
>> In case the setting of the state blobs fails, we return a
>> negative errno code to fail the start of the VM.
>>
>> Since we have an external TPM emulator, we need to make sure
>> that we do not migrate the state for as long as it is busy
>> processing a request. We need to wait for notification that
>> the request has completed processing.
> Because qemu will have to deal with an external state, managed by the
> tpm emulator (different from other storage/nvram):
>
> - the emulator statedir must be different between source and dest? Can
> it be the same?
> - what is the story regarding versionning? Can you migrate from/to any
> version, or only the same version?
> - can the host source and destination be of different endianess?
> - how is suspend and snapshot working?
>
> There are probably other complications that I can't think of
> immediately, but David or Juan help may help avoid mistakes.
>
> It probably deserves a few explanations in docs/specs/tpm.txt.
>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--
> It would be worth to add some basic tests. Either growing our own
> tests/tpm-emu.c test emulator
>
> or checking that swtpm is present (and of required version?) to
> run/skip the test a more complete test.
>
>>   1 file changed, 302 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
>> index b787aee..da877e5 100644
>> --- a/hw/tpm/tpm_emulator.c
>> +++ b/hw/tpm/tpm_emulator.c
>> @@ -55,6 +55,19 @@
>>   #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
>>
>>   /* data structures */
>> +
>> +/* blobs from the TPM; part of VM state when migrating */
>> +typedef struct TPMBlobBuffers {
>> +    uint32_t permanent_flags;
>> +    TPMSizedBuffer permanent;
>> +
>> +    uint32_t volatil_flags;
>> +    TPMSizedBuffer volatil;
>> +
>> +    uint32_t savestate_flags;
>> +    TPMSizedBuffer savestate;
>> +} TPMBlobBuffers;
>> +
>>   typedef struct TPMEmulator {
>>       TPMBackend parent;
>>
>> @@ -70,6 +83,8 @@ typedef struct TPMEmulator {
>>
>>       unsigned int established_flag:1;
>>       unsigned int established_flag_cached:1;
>> +
>> +    TPMBlobBuffers state_blobs;
> Suspicious addition, it shouldn't be necessary in running state. It
> could be allocated on demand? Even better if the field is removed, but
> this may not be possible with VMSTATE macros.

I tried to use GArray *, but offsetof cannot be applied to it by the 
VMSTATE macros (non-constant address).

Now I am trying to use Buffer (util/buffer.c) but here we're missing 
VMSTATE macros. To support it, we would need a VMSTATE_SIZE_T() or so, 
truncating size_t to 32 bits(?). This would be for storing the size 
indicator 'size-t offset' of Buffer preceeding the actual buffer. Then 
of course the VMSTATE_VBUFFER_ALLOC_UINT32 will not work anymore with 
Buffer and we would need something like VMSTATE_QEMU_BUFFER_ALLOC().

Well, the TPMSizedBuffer fit the existing VMSTATE macros quite well and 
was simple. Another approach would be to implement SimpleSizedBuffer, 
which would basically be a renaming of TPMSizedBuffer with the existing 
few TPMSizeBuffer functions wrapping it. Or we go with the plain size_t 
length indicator and char *buffer without wrapping it in any structure - 
but ... is that better?

 > +    .pre_save = tpm_emulator_pre_save,
 > +    .post_load = tpm_emulator_post_load,
 > +    .fields = (VMStateField[]) {
 > +        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
 > +        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
 > + VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
 > +                                     TPMEmulator, 1, 0,
 > + state_blobs.permanent.size),
 > +
 > +        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
 > +        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
 > + VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
 > +                                     TPMEmulator, 1, 0,
 > + state_blobs.volatil.size),
 > +
 > +        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
 > +        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
 > + VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
 > +                                     TPMEmulator, 1, 0,
 > + state_blobs.savestate.size),
 > +
 > +        VMSTATE_END_OF_LIST()
 > +    }
 > +};
 > +

    Stefan


>>   } TPMEmulator;
>>
>>
>> @@ -301,7 +316,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>>       return 0;
>>   }
>>
>> -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>> +static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
>> +                                     bool is_resume)
> Using underscore-prefixed names is discouraged. Call it tpm_emulator_init?
>
>>   {
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>       ptm_init init = {
>> @@ -309,12 +325,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>>       };
>>       ptm_res res;
>>
>> +    DPRINTF("%s   is_resume: %d", __func__, is_resume);
>> +
> extra spaces, you may also add buffersize while at it.
>
>>       if (buffersize != 0 &&
>>           tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
>>           goto err_exit;
>>       }
>>
>> -    DPRINTF("%s", __func__);
>> +    if (is_resume) {
>> +        init.u.req.init_flags = cpu_to_be32(PTM_INIT_FLAG_DELETE_VOLATILE);
>> +    }
> Since it's a flags, and for future extensibility, I would suggest |=.
>
>> +
>>       if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
>>                                sizeof(init)) < 0) {
>>           error_report("tpm-emulator: could not send INIT: %s",
>> @@ -333,6 +354,11 @@ err_exit:
>>       return -1;
>>   }
>>
>> +static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>> +{
>> +    return _tpm_emulator_startup_tpm(tb, buffersize, false);
>> +}
>> +
>>   static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
>>   {
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>> @@ -431,16 +457,21 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
>>   static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
>>   {
>>       Error *err = NULL;
>> +    ptm_cap caps = PTM_CAP_GET_STATEBLOB | PTM_CAP_SET_STATEBLOB |
>> +                   PTM_CAP_STOP;
>>
>> -    error_setg(&tpm_emu->migration_blocker,
>> -               "Migration disabled: TPM emulator not yet migratable");
>> -    migrate_add_blocker(tpm_emu->migration_blocker, &err);
>> -    if (err) {
>> -        error_report_err(err);
>> -        error_free(tpm_emu->migration_blocker);
>> -        tpm_emu->migration_blocker = NULL;
>> +    if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_emu, caps)) {
>> +        error_setg(&tpm_emu->migration_blocker,
>> +                   "Migration disabled: TPM emulator does not support "
>> +                   "migration");
>> +        migrate_add_blocker(tpm_emu->migration_blocker, &err);
>> +        if (err) {
>> +            error_report_err(err);
>> +            error_free(tpm_emu->migration_blocker);
>> +            tpm_emu->migration_blocker = NULL;
>>
>> -        return -1;
>> +            return -1;
>> +        }
>>       }
>>
>>       return 0;
>> @@ -569,6 +600,263 @@ static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
>>       { /* end of list */ },
>>   };
>>
>> +/*
>> + * Transfer a TPM state blob from the TPM into a provided buffer.
>> + *
>> + * @tpm_emu: TPMEmulator
>> + * @type: the type of blob to transfer
>> + * @tsb: the TPMSizeBuffer to fill with the blob
>> + * @flags: the flags to return to the caller
>> + */
>> +static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
>> +                                       uint8_t type,
>> +                                       TPMSizedBuffer *tsb,
>> +                                       uint32_t *flags)
>> +{
>> +    ptm_getstate pgs;
>> +    ptm_res res;
>> +    ssize_t n;
>> +    uint32_t totlength, length;
>> +
>> +    tpm_sized_buffer_reset(tsb);
>> +
>> +    pgs.u.req.state_flags = cpu_to_be32(PTM_STATE_FLAG_DECRYPTED);
>> +    pgs.u.req.type = cpu_to_be32(type);
>> +    pgs.u.req.offset = 0;
>> +
>> +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_STATEBLOB,
>> +                             &pgs, sizeof(pgs.u.req),
>> +                             offsetof(ptm_getstate, u.resp.data)) < 0) {
>> +        error_report("tpm-emulator: could not get state blob type %d : %s",
>> +                     type, strerror(errno));
>> +        return -1;
> (I guess some day vmstate functions should have an Error ** argument)
>
>> +    }
>> +
>> +    res = be32_to_cpu(pgs.u.resp.tpm_result);
>> +    if (res != 0 && (res & 0x800) == 0) {
>> +        error_report("tpm-emulator: Getting the stateblob (type %d) failed "
>> +                     "with a TPM error 0x%x", type, res);
>> +        return -1;
>> +    }
>> +
>> +    totlength = be32_to_cpu(pgs.u.resp.totlength);
>> +    length = be32_to_cpu(pgs.u.resp.length);
>> +    if (totlength != length) {
>> +        error_report("tpm-emulator: Expecting to read %u bytes "
>> +                     "but would get %u", totlength, length);
>> +        return -1;
>> +    }
>> +
>> +    *flags = be32_to_cpu(pgs.u.resp.state_flags);
>> +
>> +    tsb->buffer = g_malloc(totlength);
> That TPMSizedBuffer is not really useful. Use GArray or qemu Buffer ?
> (and we could drop TPMSizedBuffer).
>
>> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
>> +    if (n != totlength) {
>> +        error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
>> +                     type, strerror(errno));
>> +        return -1;
>> +    }
>> +    tsb->size = totlength;
>> +
>> +    DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
>> +            type, tsb->size, *flags);
>> +
>> +    return 0;
>> +}
>> +
>> +static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
>> +{
>> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
>> +
>> +    if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
>> +                                    &state_blobs->permanent,
>> +                                    &state_blobs->permanent_flags) < 0 ||
>> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
>> +                                    &state_blobs->volatil,
>> +                                    &state_blobs->volatil_flags) < 0 ||
>> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
>> +                                    &state_blobs->savestate,
>> +                                    &state_blobs->savestate_flags) < 0) {
>> +        goto err_exit;
>> +    }
>> +
>> +    return 0;
>> +
>> + err_exit:
>> +    tpm_sized_buffer_reset(&state_blobs->volatil);
>> +    tpm_sized_buffer_reset(&state_blobs->permanent);
>> +    tpm_sized_buffer_reset(&state_blobs->savestate);
>> +
>> +    return -1;
>> +}
>> +
>> +/*
>> + * Transfer a TPM state blob to the TPM emulator.
>> + *
>> + * @tpm_emu: TPMEmulator
>> + * @type: the type of TPM state blob to transfer
>> + * @tsb: TPMSizeBuffer containing the TPM state blob
>> + * @flags: Flags describing the (encryption) state of the TPM state blob
>> + */
>> +static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
>> +                                       uint32_t type,
>> +                                       TPMSizedBuffer *tsb,
>> +                                       uint32_t flags)
>> +{
>> +    uint32_t offset = 0;
>> +    ssize_t n;
>> +    ptm_setstate pss;
>> +    ptm_res tpm_result;
>> +
>> +    if (tsb->size == 0) {
>> +        return 0;
>> +    }
>> +
>> +    pss = (ptm_setstate) {
>> +        .u.req.state_flags = cpu_to_be32(flags),
>> +        .u.req.type = cpu_to_be32(type),
>> +        .u.req.length = cpu_to_be32(tsb->size),
>> +    };
>> +
>> +    /* write the header only */
>> +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
>> +                             offsetof(ptm_setstate, u.req.data), 0) < 0) {
>> +        error_report("tpm-emulator: could not set state blob type %d : %s",
>> +                     type, strerror(errno));
>> +        return -1;
>> +    }
>> +
>> +    /* now the body */
>> +    n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
>> +                              &tsb->buffer[offset], tsb->size);
>> +    if (n != tsb->size) {
>> +        error_report("tpm-emulator: Writing the stateblob (type %d) "
>> +                     "failed: %s", type, strerror(errno));
>> +        return -1;
>> +    }
>> +
>> +    /* now get the result */
>> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
>> +                             (uint8_t *)&pss, sizeof(pss.u.resp));
>> +    if (n != sizeof(pss.u.resp)) {
>> +        error_report("tpm-emulator: Reading response from writing stateblob "
>> +                     "(type %d) failed: %s", type, strerror(errno));
>> +        return -1;
>> +    }
>> +
>> +    tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
>> +    if (tpm_result != 0) {
>> +        error_report("tpm-emulator: Setting the stateblob (type %d) failed "
>> +                     "with a TPM error 0x%x", type, tpm_result);
>> +        return -1;
>> +    }
>> +
>> +    DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
>> +            type, tsb->size, flags);
>> +
>> +    return 0;
>> +}
>> +
>> +/*
>> + * Set all the TPM state blobs.
>> + *
>> + * Returns a negative errno code in case of error.
>> + */
>> +static int tpm_emulator_set_state_blobs(TPMBackend *tb)
>> +{
>> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
>> +
>> +    DPRINTF("%s: %d", __func__, __LINE__);
>> +
>> +    if (tpm_emulator_stop_tpm(tb) < 0) {
>> +        return -EIO;
>> +    }
>> +
>> +    if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
>> +                                    &state_blobs->permanent,
>> +                                    state_blobs->permanent_flags) < 0 ||
>> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
>> +                                    &state_blobs->volatil,
>> +                                    state_blobs->volatil_flags) < 0 ||
>> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
>> +                                    &state_blobs->savestate,
>> +                                    state_blobs->savestate_flags) < 0) {
>> +        return -EBADMSG;
>> +    }
>> +
>> +    DPRINTF("DONE SETTING STATEBLOBS");
> UPPERCASES!
>
>> +
>> +    return 0;
>> +}
>> +
>> +static int tpm_emulator_pre_save(void *opaque)
>> +{
>> +    TPMBackend *tb = opaque;
>> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>> +
>> +    DPRINTF("%s", __func__);
>> +
>> +    tpm_backend_finish_sync(tb);
>> +
>> +    /* get the state blobs from the TPM */
>> +    return tpm_emulator_get_state_blobs(tpm_emu);
>> +}
>> +
>> +/*
>> + * Load the TPM state blobs into the TPM.
>> + *
>> + * Returns negative errno codes in case of error.
>> + */
>> +static int tpm_emulator_post_load(void *opaque,
>> +                                  int version_id __attribute__((unused)))
> unnecessary attribute
>
>> +{
>> +    TPMBackend *tb = opaque;
>> +    int ret;
>> +
>> +    ret = tpm_emulator_set_state_blobs(tb);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    if (_tpm_emulator_startup_tpm(tb, 0, true) < 0) {
>> +        return -EIO;
>> +    }
>> +
> I dislike the mix of functions returning errno and others, and the
> lack of Error **, but it's not specific to this patch. Ok.  :)
>
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_tpm_emulator = {
>> +    .name = "tpm-emulator",
>> +    .version_id = 1,
>> +    .minimum_version_id = 0,
> version_id = 1 & minimum_version_id = 0 ?
>
> It's the first version, let's have version_id = 0 (and you could
> remove minimum_version).
>
>> +    .minimum_version_id_old = 0,
> No need to specify that, no load_state_old() either
>
>> +    .pre_save = tpm_emulator_pre_save,
>> +    .post_load = tpm_emulator_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
>> +        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
>> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
>> +                                     TPMEmulator, 1, 0,
>> +                                     state_blobs.permanent.size),
>> +
>> +        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
>> +        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
>> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
>> +                                     TPMEmulator, 1, 0,
>> +                                     state_blobs.volatil.size),
>> +
>> +        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
>> +        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
>> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
>> +                                     TPMEmulator, 1, 0,
>> +                                     state_blobs.savestate.size),
>> +
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static void tpm_emulator_inst_init(Object *obj)
>>   {
>>       TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
>> @@ -577,6 +865,8 @@ static void tpm_emulator_inst_init(Object *obj)
>>       tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
>>       tpm_emu->cur_locty_number = ~0;
>>       qemu_mutex_init(&tpm_emu->mutex);
>> +
>> +    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
>>   }
>>
>>   /*
>> @@ -613,6 +903,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
>>       }
>>
>>       qemu_mutex_destroy(&tpm_emu->mutex);
>> +
>> +    vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);
>>   }
>>
>>   static void tpm_emulator_class_init(ObjectClass *klass, void *data)
>> --
>> 2.5.5
>>
>>
> looks good to me overall
>

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Stefan Berger]

On 03/02/2018 05:14 AM, Dr. David Alan Gilbert wrote:
> * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
>> Hi Stefan
>>
>> On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
>> <stefanb@linux.vnet.ibm.com> wrote:
>>> Extend the TPM emulator backend device with state migration support.
>>>
>>> The external TPM emulator 'swtpm' provides a protocol over
>>> its control channel to retrieve its state blobs. We implement
>>> functions for getting and setting the different state blobs.
>>> In case the setting of the state blobs fails, we return a
>>> negative errno code to fail the start of the VM.
>>>
>>> Since we have an external TPM emulator, we need to make sure
>>> that we do not migrate the state for as long as it is busy
>>> processing a request. We need to wait for notification that
>>> the request has completed processing.
>> Because qemu will have to deal with an external state, managed by the
>> tpm emulator (different from other storage/nvram):
>>
>> - the emulator statedir must be different between source and dest? Can
>> it be the same?
>> - what is the story regarding versionning? Can you migrate from/to any
>> version, or only the same version?
>> - can the host source and destination be of different endianess?
>> - how is suspend and snapshot working?
>>
>> There are probably other complications that I can't think of
>> immediately, but David or Juan help may help avoid mistakes.
> Yes, I think that just about covers it.
>
>> It probably deserves a few explanations in docs/specs/tpm.txt.
>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>>> ---
>>>   hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--
>> It would be worth to add some basic tests. Either growing our own
>> tests/tpm-emu.c test emulator
>>
>> or checking that swtpm is present (and of required version?) to
>> run/skip the test a more complete test.
>>
>>>   1 file changed, 302 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
>>> index b787aee..da877e5 100644
>>> --- a/hw/tpm/tpm_emulator.c
>>> +++ b/hw/tpm/tpm_emulator.c
>>> @@ -55,6 +55,19 @@
>>>   #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
>>>
>>>   /* data structures */
>>> +
>>> +/* blobs from the TPM; part of VM state when migrating */
>>> +typedef struct TPMBlobBuffers {
>>> +    uint32_t permanent_flags;
>>> +    TPMSizedBuffer permanent;
>>> +
>>> +    uint32_t volatil_flags;
>>> +    TPMSizedBuffer volatil;
>>> +
>>> +    uint32_t savestate_flags;
>>> +    TPMSizedBuffer savestate;
>>> +} TPMBlobBuffers;
>>> +
>>>   typedef struct TPMEmulator {
>>>       TPMBackend parent;
>>>
>>> @@ -70,6 +83,8 @@ typedef struct TPMEmulator {
>>>
>>>       unsigned int established_flag:1;
>>>       unsigned int established_flag_cached:1;
>>> +
>>> +    TPMBlobBuffers state_blobs;
>> Suspicious addition, it shouldn't be necessary in running state. It
>> could be allocated on demand? Even better if the field is removed, but
>> this may not be possible with VMSTATE macros.
>>
>>>   } TPMEmulator;
>>>
>>>
>>> @@ -301,7 +316,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>>>       return 0;
>>>   }
>>>
>>> -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>>> +static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
>>> +                                     bool is_resume)
>> Using underscore-prefixed names is discouraged. Call it tpm_emulator_init?
>>
>>>   {
>>>       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>>       ptm_init init = {
>>> @@ -309,12 +325,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>>>       };
>>>       ptm_res res;
>>>
>>> +    DPRINTF("%s   is_resume: %d", __func__, is_resume);
>>> +
>> extra spaces, you may also add buffersize while at it.
> Also it would be good to use trace_'s where possible rather than
> DPRINTFs.

Here's a branch where I am doing that now:

https://github.com/stefanberger/qemu-tpm/commits/tracing

I would leave a DEBUG_TIS in the tpm_tis.c, though.

>> That TPMSizedBuffer is not really useful. Use GArray or qemu Buffer ?
>> (and we could drop TPMSizedBuffer).
> Also, given this is an arbitrary sized chunk, this should probably use
> g_try_malloc and check the result rather than letting g_malloc assert on
> failure (especially true on the source side).

Will fix. Thanks.


>
>>> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
>>> +    if (n != totlength) {
>>> +        error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
>>> +                     type, strerror(errno));
>>> +        return -1;
>>> +    }
>>> +    tsb->size = totlength;
>>> +
>>> +    DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
>>> +            type, tsb->size, *flags);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
>>> +{
>>> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
>>> +
>>> +    if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
>>> +                                    &state_blobs->permanent,
>>> +                                    &state_blobs->permanent_flags) < 0 ||
>>> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
>>> +                                    &state_blobs->volatil,
>>> +                                    &state_blobs->volatil_flags) < 0 ||
>>> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
>>> +                                    &state_blobs->savestate,
>>> +                                    &state_blobs->savestate_flags) < 0) {
>>> +        goto err_exit;
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> + err_exit:
>>> +    tpm_sized_buffer_reset(&state_blobs->volatil);
>>> +    tpm_sized_buffer_reset(&state_blobs->permanent);
>>> +    tpm_sized_buffer_reset(&state_blobs->savestate);
>>> +
>>> +    return -1;
>>> +}
>>> +
>>> +/*
>>> + * Transfer a TPM state blob to the TPM emulator.
>>> + *
>>> + * @tpm_emu: TPMEmulator
>>> + * @type: the type of TPM state blob to transfer
>>> + * @tsb: TPMSizeBuffer containing the TPM state blob
>>> + * @flags: Flags describing the (encryption) state of the TPM state blob
>>> + */
>>> +static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
>>> +                                       uint32_t type,
>>> +                                       TPMSizedBuffer *tsb,
>>> +                                       uint32_t flags)
>>> +{
>>> +    uint32_t offset = 0;
>>> +    ssize_t n;
>>> +    ptm_setstate pss;
>>> +    ptm_res tpm_result;
>>> +
>>> +    if (tsb->size == 0) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    pss = (ptm_setstate) {
>>> +        .u.req.state_flags = cpu_to_be32(flags),
>>> +        .u.req.type = cpu_to_be32(type),
>>> +        .u.req.length = cpu_to_be32(tsb->size),
>>> +    };
>>> +
>>> +    /* write the header only */
>>> +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
>>> +                             offsetof(ptm_setstate, u.req.data), 0) < 0) {
>>> +        error_report("tpm-emulator: could not set state blob type %d : %s",
>>> +                     type, strerror(errno));
>>> +        return -1;
>>> +    }
>>> +
>>> +    /* now the body */
>>> +    n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
>>> +                              &tsb->buffer[offset], tsb->size);
>>> +    if (n != tsb->size) {
>>> +        error_report("tpm-emulator: Writing the stateblob (type %d) "
>>> +                     "failed: %s", type, strerror(errno));
>>> +        return -1;
>>> +    }
>>> +
>>> +    /* now get the result */
>>> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
>>> +                             (uint8_t *)&pss, sizeof(pss.u.resp));
>>> +    if (n != sizeof(pss.u.resp)) {
>>> +        error_report("tpm-emulator: Reading response from writing stateblob "
>>> +                     "(type %d) failed: %s", type, strerror(errno));
>>> +        return -1;
>>> +    }
>>> +
>>> +    tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
>>> +    if (tpm_result != 0) {
>>> +        error_report("tpm-emulator: Setting the stateblob (type %d) failed "
>>> +                     "with a TPM error 0x%x", type, tpm_result);
>>> +        return -1;
>>> +    }
>>> +
>>> +    DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
>>> +            type, tsb->size, flags);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/*
>>> + * Set all the TPM state blobs.
>>> + *
>>> + * Returns a negative errno code in case of error.
>>> + */
>>> +static int tpm_emulator_set_state_blobs(TPMBackend *tb)
>>> +{
>>> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
>>> +
>>> +    DPRINTF("%s: %d", __func__, __LINE__);
>>> +
>>> +    if (tpm_emulator_stop_tpm(tb) < 0) {
>>> +        return -EIO;
>>> +    }
>>> +
>>> +    if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
>>> +                                    &state_blobs->permanent,
>>> +                                    state_blobs->permanent_flags) < 0 ||
>>> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
>>> +                                    &state_blobs->volatil,
>>> +                                    state_blobs->volatil_flags) < 0 ||
>>> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
>>> +                                    &state_blobs->savestate,
>>> +                                    state_blobs->savestate_flags) < 0) {
>>> +        return -EBADMSG;
>>> +    }
>>> +
>>> +    DPRINTF("DONE SETTING STATEBLOBS");
>> UPPERCASES!
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int tpm_emulator_pre_save(void *opaque)
>>> +{
>>> +    TPMBackend *tb = opaque;
>>> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>>> +
>>> +    DPRINTF("%s", __func__);
>>> +
>>> +    tpm_backend_finish_sync(tb);
>>> +
>>> +    /* get the state blobs from the TPM */
>>> +    return tpm_emulator_get_state_blobs(tpm_emu);
>>> +}
>>> +
>>> +/*
>>> + * Load the TPM state blobs into the TPM.
>>> + *
>>> + * Returns negative errno codes in case of error.
>>> + */
>>> +static int tpm_emulator_post_load(void *opaque,
>>> +                                  int version_id __attribute__((unused)))
>> unnecessary attribute
>>
>>> +{
>>> +    TPMBackend *tb = opaque;
>>> +    int ret;
>>> +
>>> +    ret = tpm_emulator_set_state_blobs(tb);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    if (_tpm_emulator_startup_tpm(tb, 0, true) < 0) {
>>> +        return -EIO;
>>> +    }
>>> +
>> I dislike the mix of functions returning errno and others, and the
>> lack of Error **, but it's not specific to this patch. Ok.  :)
>>
>>> +    return 0;
>>> +}
>>> +
>>> +static const VMStateDescription vmstate_tpm_emulator = {
>>> +    .name = "tpm-emulator",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 0,
>> version_id = 1 & minimum_version_id = 0 ?
>>
>> It's the first version, let's have version_id = 0 (and you could
>> remove minimum_version).
>>
>>> +    .minimum_version_id_old = 0,
>> No need to specify that, no load_state_old() either
>>
>>> +    .pre_save = tpm_emulator_pre_save,
>>> +    .post_load = tpm_emulator_post_load,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
>>> +        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
>>> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
>>> +                                     TPMEmulator, 1, 0,
>>> +                                     state_blobs.permanent.size),
>>> +
>>> +        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
>>> +        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
>>> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
>>> +                                     TPMEmulator, 1, 0,
>>> +                                     state_blobs.volatil.size),
>>> +
>>> +        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
>>> +        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
>>> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
>>> +                                     TPMEmulator, 1, 0,
>>> +                                     state_blobs.savestate.size),
>>> +
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>>   static void tpm_emulator_inst_init(Object *obj)
>>>   {
>>>       TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
>>> @@ -577,6 +865,8 @@ static void tpm_emulator_inst_init(Object *obj)
>>>       tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
>>>       tpm_emu->cur_locty_number = ~0;
>>>       qemu_mutex_init(&tpm_emu->mutex);
>>> +
>>> +    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
>>>   }
>>>
>>>   /*
>>> @@ -613,6 +903,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
>>>       }
>>>
>>>       qemu_mutex_destroy(&tpm_emu->mutex);
>>> +
>>> +    vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);
> It's best to avoid the vmstate_register/unregister and just set the
> dc->vmsd pointer during class_init; see hw/virtio/virtio-balloon.c
> virtio_balloon_class_init for an example.

hw/tpm/tpm_emulator.c:909:tpm_emulator_class_init: Object 0x55f4cbc72680 
is not an instance of type device
Aborted

Can we leave it as it is ?


Thanks for the review.

    Stefan

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Dr. David Alan Gilbert]

* Stefan Berger (stefanb@linux.vnet.ibm.com) wrote:
> On 03/02/2018 05:14 AM, Dr. David Alan Gilbert wrote:
> > * Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> > > Hi Stefan
> > > 
> > > On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
> > > <stefanb@linux.vnet.ibm.com> wrote:
> > > > Extend the TPM emulator backend device with state migration support.
> > > > 
> > > > The external TPM emulator 'swtpm' provides a protocol over
> > > > its control channel to retrieve its state blobs. We implement
> > > > functions for getting and setting the different state blobs.
> > > > In case the setting of the state blobs fails, we return a
> > > > negative errno code to fail the start of the VM.
> > > > 
> > > > Since we have an external TPM emulator, we need to make sure
> > > > that we do not migrate the state for as long as it is busy
> > > > processing a request. We need to wait for notification that
> > > > the request has completed processing.
> > > Because qemu will have to deal with an external state, managed by the
> > > tpm emulator (different from other storage/nvram):
> > > 
> > > - the emulator statedir must be different between source and dest? Can
> > > it be the same?
> > > - what is the story regarding versionning? Can you migrate from/to any
> > > version, or only the same version?
> > > - can the host source and destination be of different endianess?
> > > - how is suspend and snapshot working?
> > > 
> > > There are probably other complications that I can't think of
> > > immediately, but David or Juan help may help avoid mistakes.
> > Yes, I think that just about covers it.
> > 
> > > It probably deserves a few explanations in docs/specs/tpm.txt.
> > > 
> > > > Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > ---
> > > >   hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--
> > > It would be worth to add some basic tests. Either growing our own
> > > tests/tpm-emu.c test emulator
> > > 
> > > or checking that swtpm is present (and of required version?) to
> > > run/skip the test a more complete test.
> > > 
> > > >   1 file changed, 302 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> > > > index b787aee..da877e5 100644
> > > > --- a/hw/tpm/tpm_emulator.c
> > > > +++ b/hw/tpm/tpm_emulator.c
> > > > @@ -55,6 +55,19 @@
> > > >   #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
> > > > 
> > > >   /* data structures */
> > > > +
> > > > +/* blobs from the TPM; part of VM state when migrating */
> > > > +typedef struct TPMBlobBuffers {
> > > > +    uint32_t permanent_flags;
> > > > +    TPMSizedBuffer permanent;
> > > > +
> > > > +    uint32_t volatil_flags;
> > > > +    TPMSizedBuffer volatil;
> > > > +
> > > > +    uint32_t savestate_flags;
> > > > +    TPMSizedBuffer savestate;
> > > > +} TPMBlobBuffers;
> > > > +
> > > >   typedef struct TPMEmulator {
> > > >       TPMBackend parent;
> > > > 
> > > > @@ -70,6 +83,8 @@ typedef struct TPMEmulator {
> > > > 
> > > >       unsigned int established_flag:1;
> > > >       unsigned int established_flag_cached:1;
> > > > +
> > > > +    TPMBlobBuffers state_blobs;
> > > Suspicious addition, it shouldn't be necessary in running state. It
> > > could be allocated on demand? Even better if the field is removed, but
> > > this may not be possible with VMSTATE macros.
> > > 
> > > >   } TPMEmulator;
> > > > 
> > > > 
> > > > @@ -301,7 +316,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
> > > >       return 0;
> > > >   }
> > > > 
> > > > -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> > > > +static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
> > > > +                                     bool is_resume)
> > > Using underscore-prefixed names is discouraged. Call it tpm_emulator_init?
> > > 
> > > >   {
> > > >       TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> > > >       ptm_init init = {
> > > > @@ -309,12 +325,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> > > >       };
> > > >       ptm_res res;
> > > > 
> > > > +    DPRINTF("%s   is_resume: %d", __func__, is_resume);
> > > > +
> > > extra spaces, you may also add buffersize while at it.
> > Also it would be good to use trace_'s where possible rather than
> > DPRINTFs.
> 
> Here's a branch where I am doing that now:
> 
> https://github.com/stefanberger/qemu-tpm/commits/tracing
> 
> I would leave a DEBUG_TIS in the tpm_tis.c, though.
> 
> > > That TPMSizedBuffer is not really useful. Use GArray or qemu Buffer ?
> > > (and we could drop TPMSizedBuffer).
> > Also, given this is an arbitrary sized chunk, this should probably use
> > g_try_malloc and check the result rather than letting g_malloc assert on
> > failure (especially true on the source side).
> 
> Will fix. Thanks.
> 
> 
> > 
> > > > +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
> > > > +    if (n != totlength) {
> > > > +        error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
> > > > +                     type, strerror(errno));
> > > > +        return -1;
> > > > +    }
> > > > +    tsb->size = totlength;
> > > > +
> > > > +    DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
> > > > +            type, tsb->size, *flags);
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
> > > > +{
> > > > +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
> > > > +
> > > > +    if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
> > > > +                                    &state_blobs->permanent,
> > > > +                                    &state_blobs->permanent_flags) < 0 ||
> > > > +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
> > > > +                                    &state_blobs->volatil,
> > > > +                                    &state_blobs->volatil_flags) < 0 ||
> > > > +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
> > > > +                                    &state_blobs->savestate,
> > > > +                                    &state_blobs->savestate_flags) < 0) {
> > > > +        goto err_exit;
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +
> > > > + err_exit:
> > > > +    tpm_sized_buffer_reset(&state_blobs->volatil);
> > > > +    tpm_sized_buffer_reset(&state_blobs->permanent);
> > > > +    tpm_sized_buffer_reset(&state_blobs->savestate);
> > > > +
> > > > +    return -1;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Transfer a TPM state blob to the TPM emulator.
> > > > + *
> > > > + * @tpm_emu: TPMEmulator
> > > > + * @type: the type of TPM state blob to transfer
> > > > + * @tsb: TPMSizeBuffer containing the TPM state blob
> > > > + * @flags: Flags describing the (encryption) state of the TPM state blob
> > > > + */
> > > > +static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
> > > > +                                       uint32_t type,
> > > > +                                       TPMSizedBuffer *tsb,
> > > > +                                       uint32_t flags)
> > > > +{
> > > > +    uint32_t offset = 0;
> > > > +    ssize_t n;
> > > > +    ptm_setstate pss;
> > > > +    ptm_res tpm_result;
> > > > +
> > > > +    if (tsb->size == 0) {
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    pss = (ptm_setstate) {
> > > > +        .u.req.state_flags = cpu_to_be32(flags),
> > > > +        .u.req.type = cpu_to_be32(type),
> > > > +        .u.req.length = cpu_to_be32(tsb->size),
> > > > +    };
> > > > +
> > > > +    /* write the header only */
> > > > +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
> > > > +                             offsetof(ptm_setstate, u.req.data), 0) < 0) {
> > > > +        error_report("tpm-emulator: could not set state blob type %d : %s",
> > > > +                     type, strerror(errno));
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    /* now the body */
> > > > +    n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
> > > > +                              &tsb->buffer[offset], tsb->size);
> > > > +    if (n != tsb->size) {
> > > > +        error_report("tpm-emulator: Writing the stateblob (type %d) "
> > > > +                     "failed: %s", type, strerror(errno));
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    /* now get the result */
> > > > +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
> > > > +                             (uint8_t *)&pss, sizeof(pss.u.resp));
> > > > +    if (n != sizeof(pss.u.resp)) {
> > > > +        error_report("tpm-emulator: Reading response from writing stateblob "
> > > > +                     "(type %d) failed: %s", type, strerror(errno));
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
> > > > +    if (tpm_result != 0) {
> > > > +        error_report("tpm-emulator: Setting the stateblob (type %d) failed "
> > > > +                     "with a TPM error 0x%x", type, tpm_result);
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
> > > > +            type, tsb->size, flags);
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Set all the TPM state blobs.
> > > > + *
> > > > + * Returns a negative errno code in case of error.
> > > > + */
> > > > +static int tpm_emulator_set_state_blobs(TPMBackend *tb)
> > > > +{
> > > > +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> > > > +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
> > > > +
> > > > +    DPRINTF("%s: %d", __func__, __LINE__);
> > > > +
> > > > +    if (tpm_emulator_stop_tpm(tb) < 0) {
> > > > +        return -EIO;
> > > > +    }
> > > > +
> > > > +    if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
> > > > +                                    &state_blobs->permanent,
> > > > +                                    state_blobs->permanent_flags) < 0 ||
> > > > +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
> > > > +                                    &state_blobs->volatil,
> > > > +                                    state_blobs->volatil_flags) < 0 ||
> > > > +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
> > > > +                                    &state_blobs->savestate,
> > > > +                                    state_blobs->savestate_flags) < 0) {
> > > > +        return -EBADMSG;
> > > > +    }
> > > > +
> > > > +    DPRINTF("DONE SETTING STATEBLOBS");
> > > UPPERCASES!
> > > 
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static int tpm_emulator_pre_save(void *opaque)
> > > > +{
> > > > +    TPMBackend *tb = opaque;
> > > > +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> > > > +
> > > > +    DPRINTF("%s", __func__);
> > > > +
> > > > +    tpm_backend_finish_sync(tb);
> > > > +
> > > > +    /* get the state blobs from the TPM */
> > > > +    return tpm_emulator_get_state_blobs(tpm_emu);
> > > > +}
> > > > +
> > > > +/*
> > > > + * Load the TPM state blobs into the TPM.
> > > > + *
> > > > + * Returns negative errno codes in case of error.
> > > > + */
> > > > +static int tpm_emulator_post_load(void *opaque,
> > > > +                                  int version_id __attribute__((unused)))
> > > unnecessary attribute
> > > 
> > > > +{
> > > > +    TPMBackend *tb = opaque;
> > > > +    int ret;
> > > > +
> > > > +    ret = tpm_emulator_set_state_blobs(tb);
> > > > +    if (ret < 0) {
> > > > +        return ret;
> > > > +    }
> > > > +
> > > > +    if (_tpm_emulator_startup_tpm(tb, 0, true) < 0) {
> > > > +        return -EIO;
> > > > +    }
> > > > +
> > > I dislike the mix of functions returning errno and others, and the
> > > lack of Error **, but it's not specific to this patch. Ok.  :)
> > > 
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static const VMStateDescription vmstate_tpm_emulator = {
> > > > +    .name = "tpm-emulator",
> > > > +    .version_id = 1,
> > > > +    .minimum_version_id = 0,
> > > version_id = 1 & minimum_version_id = 0 ?
> > > 
> > > It's the first version, let's have version_id = 0 (and you could
> > > remove minimum_version).
> > > 
> > > > +    .minimum_version_id_old = 0,
> > > No need to specify that, no load_state_old() either
> > > 
> > > > +    .pre_save = tpm_emulator_pre_save,
> > > > +    .post_load = tpm_emulator_post_load,
> > > > +    .fields = (VMStateField[]) {
> > > > +        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
> > > > +        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
> > > > +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
> > > > +                                     TPMEmulator, 1, 0,
> > > > +                                     state_blobs.permanent.size),
> > > > +
> > > > +        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
> > > > +        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
> > > > +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
> > > > +                                     TPMEmulator, 1, 0,
> > > > +                                     state_blobs.volatil.size),
> > > > +
> > > > +        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
> > > > +        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
> > > > +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
> > > > +                                     TPMEmulator, 1, 0,
> > > > +                                     state_blobs.savestate.size),
> > > > +
> > > > +        VMSTATE_END_OF_LIST()
> > > > +    }
> > > > +};
> > > > +
> > > >   static void tpm_emulator_inst_init(Object *obj)
> > > >   {
> > > >       TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
> > > > @@ -577,6 +865,8 @@ static void tpm_emulator_inst_init(Object *obj)
> > > >       tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
> > > >       tpm_emu->cur_locty_number = ~0;
> > > >       qemu_mutex_init(&tpm_emu->mutex);
> > > > +
> > > > +    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
> > > >   }
> > > > 
> > > >   /*
> > > > @@ -613,6 +903,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
> > > >       }
> > > > 
> > > >       qemu_mutex_destroy(&tpm_emu->mutex);
> > > > +
> > > > +    vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);
> > It's best to avoid the vmstate_register/unregister and just set the
> > dc->vmsd pointer during class_init; see hw/virtio/virtio-balloon.c
> > virtio_balloon_class_init for an example.
> 
> hw/tpm/tpm_emulator.c:909:tpm_emulator_class_init: Object 0x55f4cbc72680 is
> not an instance of type device
> Aborted
> 
> Can we leave it as it is ?

Oh, not a device, hmmm that's odd; yeh probably best then.

Dave

> 
> Thanks for the review.
> 
>    Stefan
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Stefan Berger]

On 03/05/2018 07:43 AM, Dr. David Alan Gilbert wrote:
> * Stefan Berger (stefanb@linux.vnet.ibm.com) wrote:
>> On 03/02/2018 05:14 AM, Dr. David Alan Gilbert wrote:
>>
>> hw/tpm/tpm_emulator.c:909:tpm_emulator_class_init: Object 0x55f4cbc72680 is
>> not an instance of type device
>> Aborted
>>
>> Can we leave it as it is ?
> Oh, not a device, hmmm that's odd; yeh probably best then.

At least it doesn't *need* to be a device that an OS could talk to via 
MMIO or so. That's the TIS or CRB hardware interfaces in this case. If 
we have to convert this 'backend' to a device for some reason, I'd do 
it, but rather leave it as it is if not.

    Stefan

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Stefan Berger]

On 03/02/2018 04:26 AM, Marc-André Lureau wrote:
>
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_tpm_emulator = {
>> +    .name = "tpm-emulator",
>> +    .version_id = 1,
>> +    .minimum_version_id = 0,
> version_id = 1 & minimum_version_id = 0 ?
>
> It's the first version, let's have version_id = 0 (and you could
> remove minimum_version).

It doesn't work...  I cannot use version_id 0 in tpm_tis or in 
tpm_emulator. For tpm_tis I get the error 'Missing section footer' upon 
VM resume and for some weird reason it doesn't allocate the blob buffers 
when reading them in tpm_emulator. I get a NULL pointer instead. So, we 
are better off with version_id 1 here... The CRB seems to work as is.

    Stefan

Re: [Qemu-devel] [PATCH v4 1/2] tpm: extend TPM emulator with state migration support

[Marc-André Lureau]

Hi

On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Extend the TPM emulator backend device with state migration support.
>
> The external TPM emulator 'swtpm' provides a protocol over
> its control channel to retrieve its state blobs. We implement
> functions for getting and setting the different state blobs.
> In case the setting of the state blobs fails, we return a
> negative errno code to fail the start of the VM.
>
> Since we have an external TPM emulator, we need to make sure
> that we do not migrate the state for as long as it is busy
> processing a request. We need to wait for notification that
> the request has completed processing.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---

With this squashed:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 6d6158deea..ec9c25989b 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -894,6 +894,7 @@ static void tpm_emulator_shutdown(TPMEmulator *tpm_emu)
 static void tpm_emulator_inst_finalize(Object *obj)
 {
     TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
+    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;

     tpm_emulator_shutdown(tpm_emu);

@@ -908,6 +909,10 @@ static void tpm_emulator_inst_finalize(Object *obj)
         error_free(tpm_emu->migration_blocker);
     }

+    tpm_sized_buffer_reset(&state_blobs->volatil);
+    tpm_sized_buffer_reset(&state_blobs->permanent);
+    tpm_sized_buffer_reset(&state_blobs->savestate);
+
     qemu_mutex_destroy(&tpm_emu->mutex);

     vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);


>  hw/tpm/tpm_emulator.c | 312 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 302 insertions(+), 10 deletions(-)
>
> diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
> index b787aee..da877e5 100644
> --- a/hw/tpm/tpm_emulator.c
> +++ b/hw/tpm/tpm_emulator.c
> @@ -55,6 +55,19 @@
>  #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
>
>  /* data structures */
> +
> +/* blobs from the TPM; part of VM state when migrating */
> +typedef struct TPMBlobBuffers {
> +    uint32_t permanent_flags;
> +    TPMSizedBuffer permanent;
> +
> +    uint32_t volatil_flags;
> +    TPMSizedBuffer volatil;
> +
> +    uint32_t savestate_flags;
> +    TPMSizedBuffer savestate;
> +} TPMBlobBuffers;
> +
>  typedef struct TPMEmulator {
>      TPMBackend parent;
>
> @@ -70,6 +83,8 @@ typedef struct TPMEmulator {
>
>      unsigned int established_flag:1;
>      unsigned int established_flag_cached:1;
> +
> +    TPMBlobBuffers state_blobs;
>  } TPMEmulator;
>
>
> @@ -301,7 +316,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
>      return 0;
>  }
>
> -static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> +static int _tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize,
> +                                     bool is_resume)
>  {
>      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
>      ptm_init init = {
> @@ -309,12 +325,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
>      };
>      ptm_res res;
>
> +    DPRINTF("%s   is_resume: %d", __func__, is_resume);
> +
>      if (buffersize != 0 &&
>          tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
>          goto err_exit;
>      }
>
> -    DPRINTF("%s", __func__);
> +    if (is_resume) {
> +        init.u.req.init_flags = cpu_to_be32(PTM_INIT_FLAG_DELETE_VOLATILE);
> +    }
> +
>      if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
>                               sizeof(init)) < 0) {
>          error_report("tpm-emulator: could not send INIT: %s",
> @@ -333,6 +354,11 @@ err_exit:
>      return -1;
>  }
>
> +static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
> +{
> +    return _tpm_emulator_startup_tpm(tb, buffersize, false);
> +}
> +
>  static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
>  {
>      TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> @@ -431,16 +457,21 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
>  static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
>  {
>      Error *err = NULL;
> +    ptm_cap caps = PTM_CAP_GET_STATEBLOB | PTM_CAP_SET_STATEBLOB |
> +                   PTM_CAP_STOP;
>
> -    error_setg(&tpm_emu->migration_blocker,
> -               "Migration disabled: TPM emulator not yet migratable");
> -    migrate_add_blocker(tpm_emu->migration_blocker, &err);
> -    if (err) {
> -        error_report_err(err);
> -        error_free(tpm_emu->migration_blocker);
> -        tpm_emu->migration_blocker = NULL;
> +    if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_emu, caps)) {
> +        error_setg(&tpm_emu->migration_blocker,
> +                   "Migration disabled: TPM emulator does not support "
> +                   "migration");
> +        migrate_add_blocker(tpm_emu->migration_blocker, &err);
> +        if (err) {
> +            error_report_err(err);
> +            error_free(tpm_emu->migration_blocker);
> +            tpm_emu->migration_blocker = NULL;
>
> -        return -1;
> +            return -1;
> +        }
>      }
>
>      return 0;
> @@ -569,6 +600,263 @@ static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
>      { /* end of list */ },
>  };
>
> +/*
> + * Transfer a TPM state blob from the TPM into a provided buffer.
> + *
> + * @tpm_emu: TPMEmulator
> + * @type: the type of blob to transfer
> + * @tsb: the TPMSizeBuffer to fill with the blob
> + * @flags: the flags to return to the caller
> + */
> +static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
> +                                       uint8_t type,
> +                                       TPMSizedBuffer *tsb,
> +                                       uint32_t *flags)
> +{
> +    ptm_getstate pgs;
> +    ptm_res res;
> +    ssize_t n;
> +    uint32_t totlength, length;
> +
> +    tpm_sized_buffer_reset(tsb);
> +
> +    pgs.u.req.state_flags = cpu_to_be32(PTM_STATE_FLAG_DECRYPTED);
> +    pgs.u.req.type = cpu_to_be32(type);
> +    pgs.u.req.offset = 0;
> +
> +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_STATEBLOB,
> +                             &pgs, sizeof(pgs.u.req),
> +                             offsetof(ptm_getstate, u.resp.data)) < 0) {
> +        error_report("tpm-emulator: could not get state blob type %d : %s",
> +                     type, strerror(errno));
> +        return -1;
> +    }
> +
> +    res = be32_to_cpu(pgs.u.resp.tpm_result);
> +    if (res != 0 && (res & 0x800) == 0) {
> +        error_report("tpm-emulator: Getting the stateblob (type %d) failed "
> +                     "with a TPM error 0x%x", type, res);
> +        return -1;
> +    }
> +
> +    totlength = be32_to_cpu(pgs.u.resp.totlength);
> +    length = be32_to_cpu(pgs.u.resp.length);
> +    if (totlength != length) {
> +        error_report("tpm-emulator: Expecting to read %u bytes "
> +                     "but would get %u", totlength, length);
> +        return -1;
> +    }
> +
> +    *flags = be32_to_cpu(pgs.u.resp.state_flags);
> +
> +    tsb->buffer = g_malloc(totlength);
> +
> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr, tsb->buffer, totlength);
> +    if (n != totlength) {
> +        error_report("tpm-emulator: Could not read stateblob (type %d) : %s",
> +                     type, strerror(errno));
> +        return -1;
> +    }
> +    tsb->size = totlength;
> +
> +    DPRINTF("got state blob type %d, %d bytes, flags 0x%08x\n",
> +            type, tsb->size, *flags);
> +
> +    return 0;
> +}
> +
> +static int tpm_emulator_get_state_blobs(TPMEmulator *tpm_emu)
> +{
> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
> +
> +    if (tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
> +                                    &state_blobs->permanent,
> +                                    &state_blobs->permanent_flags) < 0 ||
> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
> +                                    &state_blobs->volatil,
> +                                    &state_blobs->volatil_flags) < 0 ||
> +        tpm_emulator_get_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
> +                                    &state_blobs->savestate,
> +                                    &state_blobs->savestate_flags) < 0) {
> +        goto err_exit;
> +    }
> +
> +    return 0;
> +
> + err_exit:
> +    tpm_sized_buffer_reset(&state_blobs->volatil);
> +    tpm_sized_buffer_reset(&state_blobs->permanent);
> +    tpm_sized_buffer_reset(&state_blobs->savestate);
> +
> +    return -1;
> +}
> +
> +/*
> + * Transfer a TPM state blob to the TPM emulator.
> + *
> + * @tpm_emu: TPMEmulator
> + * @type: the type of TPM state blob to transfer
> + * @tsb: TPMSizeBuffer containing the TPM state blob
> + * @flags: Flags describing the (encryption) state of the TPM state blob
> + */
> +static int tpm_emulator_set_state_blob(TPMEmulator *tpm_emu,
> +                                       uint32_t type,
> +                                       TPMSizedBuffer *tsb,
> +                                       uint32_t flags)
> +{
> +    uint32_t offset = 0;
> +    ssize_t n;
> +    ptm_setstate pss;
> +    ptm_res tpm_result;
> +
> +    if (tsb->size == 0) {
> +        return 0;
> +    }
> +
> +    pss = (ptm_setstate) {
> +        .u.req.state_flags = cpu_to_be32(flags),
> +        .u.req.type = cpu_to_be32(type),
> +        .u.req.length = cpu_to_be32(tsb->size),
> +    };
> +
> +    /* write the header only */
> +    if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_STATEBLOB, &pss,
> +                             offsetof(ptm_setstate, u.req.data), 0) < 0) {
> +        error_report("tpm-emulator: could not set state blob type %d : %s",
> +                     type, strerror(errno));
> +        return -1;
> +    }
> +
> +    /* now the body */
> +    n = qemu_chr_fe_write_all(&tpm_emu->ctrl_chr,
> +                              &tsb->buffer[offset], tsb->size);
> +    if (n != tsb->size) {
> +        error_report("tpm-emulator: Writing the stateblob (type %d) "
> +                     "failed: %s", type, strerror(errno));
> +        return -1;
> +    }
> +
> +    /* now get the result */
> +    n = qemu_chr_fe_read_all(&tpm_emu->ctrl_chr,
> +                             (uint8_t *)&pss, sizeof(pss.u.resp));
> +    if (n != sizeof(pss.u.resp)) {
> +        error_report("tpm-emulator: Reading response from writing stateblob "
> +                     "(type %d) failed: %s", type, strerror(errno));
> +        return -1;
> +    }
> +
> +    tpm_result = be32_to_cpu(pss.u.resp.tpm_result);
> +    if (tpm_result != 0) {
> +        error_report("tpm-emulator: Setting the stateblob (type %d) failed "
> +                     "with a TPM error 0x%x", type, tpm_result);
> +        return -1;
> +    }
> +
> +    DPRINTF("set the state blob type %d, %d bytes, flags 0x%08x\n",
> +            type, tsb->size, flags);
> +
> +    return 0;
> +}
> +
> +/*
> + * Set all the TPM state blobs.
> + *
> + * Returns a negative errno code in case of error.
> + */
> +static int tpm_emulator_set_state_blobs(TPMBackend *tb)
> +{
> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> +    TPMBlobBuffers *state_blobs = &tpm_emu->state_blobs;
> +
> +    DPRINTF("%s: %d", __func__, __LINE__);
> +
> +    if (tpm_emulator_stop_tpm(tb) < 0) {
> +        return -EIO;
> +    }
> +
> +    if (tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_PERMANENT,
> +                                    &state_blobs->permanent,
> +                                    state_blobs->permanent_flags) < 0 ||
> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_VOLATILE,
> +                                    &state_blobs->volatil,
> +                                    state_blobs->volatil_flags) < 0 ||
> +        tpm_emulator_set_state_blob(tpm_emu, PTM_BLOB_TYPE_SAVESTATE,
> +                                    &state_blobs->savestate,
> +                                    state_blobs->savestate_flags) < 0) {
> +        return -EBADMSG;
> +    }
> +
> +    DPRINTF("DONE SETTING STATEBLOBS");
> +
> +    return 0;
> +}
> +
> +static int tpm_emulator_pre_save(void *opaque)
> +{
> +    TPMBackend *tb = opaque;
> +    TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
> +
> +    DPRINTF("%s", __func__);
> +
> +    tpm_backend_finish_sync(tb);
> +
> +    /* get the state blobs from the TPM */
> +    return tpm_emulator_get_state_blobs(tpm_emu);
> +}
> +
> +/*
> + * Load the TPM state blobs into the TPM.
> + *
> + * Returns negative errno codes in case of error.
> + */
> +static int tpm_emulator_post_load(void *opaque,
> +                                  int version_id __attribute__((unused)))
> +{
> +    TPMBackend *tb = opaque;
> +    int ret;
> +
> +    ret = tpm_emulator_set_state_blobs(tb);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    if (_tpm_emulator_startup_tpm(tb, 0, true) < 0) {
> +        return -EIO;
> +    }
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_tpm_emulator = {
> +    .name = "tpm-emulator",
> +    .version_id = 1,
> +    .minimum_version_id = 0,
> +    .minimum_version_id_old = 0,
> +    .pre_save = tpm_emulator_pre_save,
> +    .post_load = tpm_emulator_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(state_blobs.permanent_flags, TPMEmulator),
> +        VMSTATE_UINT32(state_blobs.permanent.size, TPMEmulator),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.permanent.buffer,
> +                                     TPMEmulator, 1, 0,
> +                                     state_blobs.permanent.size),
> +
> +        VMSTATE_UINT32(state_blobs.volatil_flags, TPMEmulator),
> +        VMSTATE_UINT32(state_blobs.volatil.size, TPMEmulator),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.volatil.buffer,
> +                                     TPMEmulator, 1, 0,
> +                                     state_blobs.volatil.size),
> +
> +        VMSTATE_UINT32(state_blobs.savestate_flags, TPMEmulator),
> +        VMSTATE_UINT32(state_blobs.savestate.size, TPMEmulator),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(state_blobs.savestate.buffer,
> +                                     TPMEmulator, 1, 0,
> +                                     state_blobs.savestate.size),
> +
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void tpm_emulator_inst_init(Object *obj)
>  {
>      TPMEmulator *tpm_emu = TPM_EMULATOR(obj);
> @@ -577,6 +865,8 @@ static void tpm_emulator_inst_init(Object *obj)
>      tpm_emu->options = g_new0(TPMEmulatorOptions, 1);
>      tpm_emu->cur_locty_number = ~0;
>      qemu_mutex_init(&tpm_emu->mutex);
> +
> +    vmstate_register(NULL, -1, &vmstate_tpm_emulator, obj);
>  }
>
>  /*
> @@ -613,6 +903,8 @@ static void tpm_emulator_inst_finalize(Object *obj)
>      }
>
>      qemu_mutex_destroy(&tpm_emu->mutex);
> +
> +    vmstate_unregister(NULL, &vmstate_tpm_emulator, obj);
>  }
>
>  static void tpm_emulator_class_init(ObjectClass *klass, void *data)
> --
> 2.5.5
>

Re: [Qemu-devel] [PATCH v4 2/2] tpm: extend TPM TIS with state migration support

[Marc-André Lureau]

Hi

On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> Extend the TPM TIS interface with state migration support.
>
> We need to synchronize with the backend thread to make sure that a command
> being processed by the external TPM emulator has completed and its
> response been received.
>
> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> ---
>  hw/tpm/tpm_tis.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index 834eef7..5016d28 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -902,9 +902,61 @@ static void tpm_tis_reset(DeviceState *dev)
>      tpm_backend_startup_tpm(s->be_driver, s->be_buffer_size);
>  }
>
> +/* persistent state handling */
> +
> +static int tpm_tis_pre_save(void *opaque)
> +{
> +    TPMState *s = opaque;
> +    uint8_t locty = s->active_locty;
> +
> +    DPRINTF("tpm_tis: suspend: locty = %d : rw_offset = %u\n",
> +            locty, s->rw_offset);
> +#ifdef DEBUG_TIS
> +    tpm_tis_dump_state(opaque, 0);
> +#endif
> +
> +    /*
> +     * Synchronize with backend completion.
> +     */
> +    tpm_backend_finish_sync(s->be_driver);
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_locty = {
> +    .name = "loc",
> +    .version_id = 1,
> +    .minimum_version_id = 0,
> +    .minimum_version_id_old = 0,

I don't understand the problem there is leaving all the version fields
to 0, just like CRB.

> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT32(state, TPMLocality),
> +        VMSTATE_UINT32(inte, TPMLocality),
> +        VMSTATE_UINT32(ints, TPMLocality),
> +        VMSTATE_UINT8(access, TPMLocality),
> +        VMSTATE_UINT32(sts, TPMLocality),
> +        VMSTATE_UINT32(iface_id, TPMLocality),
> +        VMSTATE_END_OF_LIST(),
> +    }
> +};
> +
>  static const VMStateDescription vmstate_tpm_tis = {
>      .name = "tpm",
> -    .unmigratable = 1,
> +    .version_id = 1,
> +    .minimum_version_id = 0,
> +    .minimum_version_id_old = 0,

same

If you remove the version fields: Reviewed-by: Marc-André Lureau
<marcandre.lureau@redhat.com>



> +    .pre_save  = tpm_tis_pre_save,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BUFFER(buffer, TPMState),
> +        VMSTATE_UINT16(rw_offset, TPMState),
> +        VMSTATE_UINT8(active_locty, TPMState),
> +        VMSTATE_UINT8(aborting_locty, TPMState),
> +        VMSTATE_UINT8(next_locty, TPMState),
> +
> +        VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 1,
> +                             vmstate_locty, TPMLocality),
> +
> +        VMSTATE_END_OF_LIST()
> +    }
>  };
>
>  static Property tpm_tis_properties[] = {
> --
> 2.5.5
>

Re: [Qemu-devel] [PATCH v4 2/2] tpm: extend TPM TIS with state migration support

[Stefan Berger]

On 03/28/2018 11:41 AM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> +
>> +static const VMStateDescription vmstate_locty = {
>> +    .name = "loc",
>> +    .version_id = 1,
>> +    .minimum_version_id = 0,
>> +    .minimum_version_id_old = 0,
> I don't understand the problem there is leaving all the version fields
> to 0, just like CRB.
>
>> +    .fields      = (VMStateField[]) {
>> +        VMSTATE_UINT32(state, TPMLocality),
>> +        VMSTATE_UINT32(inte, TPMLocality),
>> +        VMSTATE_UINT32(ints, TPMLocality),
>> +        VMSTATE_UINT8(access, TPMLocality),
>> +        VMSTATE_UINT32(sts, TPMLocality),
>> +        VMSTATE_UINT32(iface_id, TPMLocality),
>> +        VMSTATE_END_OF_LIST(),
>> +    }
>> +};
>> +
>>   static const VMStateDescription vmstate_tpm_tis = {
>>       .name = "tpm",
>> -    .unmigratable = 1,
>> +    .version_id = 1,
>> +    .minimum_version_id = 0,
>> +    .minimum_version_id_old = 0,
> same
>
> If you remove the version fields: Reviewed-by: Marc-André Lureau
> <marcandre.lureau@redhat.com>

This is the error I got when setting .version_id = 0 (on both) and doing 
a localhost migration

qemu-system-x86_64: Missing section footer for tpm-tis
qemu-system-x86_64: load of migration failed: Invalid argument

It must have something to do with the nesting invoked by 
VMSTATE_STRUCT_ARRAY(loc,...) below.



>
>
>
>> +    .pre_save  = tpm_tis_pre_save,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_BUFFER(buffer, TPMState),
>> +        VMSTATE_UINT16(rw_offset, TPMState),
>> +        VMSTATE_UINT8(active_locty, TPMState),
>> +        VMSTATE_UINT8(aborting_locty, TPMState),
>> +        VMSTATE_UINT8(next_locty, TPMState),
>> +
>> +        VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 1,
>> +                             vmstate_locty, TPMLocality),
>> +
>> +        VMSTATE_END_OF_LIST()
>> +    }
>>   };
>>
>>   static Property tpm_tis_properties[] = {
>> --
>> 2.5.5
>>

Re: [Qemu-devel] [PATCH v4 2/2] tpm: extend TPM TIS with state migration support

[Marc-André Lureau]

Hi

On Thu, Mar 29, 2018 at 1:56 AM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> On 03/28/2018 11:41 AM, Marc-André Lureau wrote:
>>
>> Hi
>>
>> On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
>> <stefanb@linux.vnet.ibm.com> wrote:
>>>
>>> +
>>> +static const VMStateDescription vmstate_locty = {
>>> +    .name = "loc",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 0,
>>> +    .minimum_version_id_old = 0,
>>
>> I don't understand the problem there is leaving all the version fields
>> to 0, just like CRB.
>>
>>> +    .fields      = (VMStateField[]) {
>>> +        VMSTATE_UINT32(state, TPMLocality),
>>> +        VMSTATE_UINT32(inte, TPMLocality),
>>> +        VMSTATE_UINT32(ints, TPMLocality),
>>> +        VMSTATE_UINT8(access, TPMLocality),
>>> +        VMSTATE_UINT32(sts, TPMLocality),
>>> +        VMSTATE_UINT32(iface_id, TPMLocality),
>>> +        VMSTATE_END_OF_LIST(),
>>> +    }
>>> +};
>>> +
>>>   static const VMStateDescription vmstate_tpm_tis = {
>>>       .name = "tpm",
>>> -    .unmigratable = 1,
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 0,
>>> +    .minimum_version_id_old = 0,
>>
>> same
>>
>> If you remove the version fields: Reviewed-by: Marc-André Lureau
>> <marcandre.lureau@redhat.com>
>
>
> This is the error I got when setting .version_id = 0 (on both) and doing a
> localhost migration
>
> qemu-system-x86_64: Missing section footer for tpm-tis
> qemu-system-x86_64: load of migration failed: Invalid argument

It's clearly not the most friendly error message, but I debugged it,
you just have to specify the right version for VMSTATE_STRUCT_ARRAY:
0.
        VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 0,
                             vmstate_locty, TPMLocality),

Then it all works with version 0 (or default value)

thanks
>
> It must have something to do with the nesting invoked by
> VMSTATE_STRUCT_ARRAY(loc,...) below.
>
>
>
>
>>
>>
>>
>>> +    .pre_save  = tpm_tis_pre_save,
>>> +    .fields = (VMStateField[]) {
>>> +        VMSTATE_BUFFER(buffer, TPMState),
>>> +        VMSTATE_UINT16(rw_offset, TPMState),
>>> +        VMSTATE_UINT8(active_locty, TPMState),
>>> +        VMSTATE_UINT8(aborting_locty, TPMState),
>>> +        VMSTATE_UINT8(next_locty, TPMState),
>>> +
>>> +        VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 1,
>>> +                             vmstate_locty, TPMLocality),
>>> +
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>>   };
>>>
>>>   static Property tpm_tis_properties[] = {
>>> --
>>> 2.5.5
>>>
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v4 2/2] tpm: extend TPM TIS with state migration support

[Stefan Berger]

On 03/29/2018 01:07 PM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Mar 29, 2018 at 1:56 AM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> On 03/28/2018 11:41 AM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
>>> <stefanb@linux.vnet.ibm.com> wrote:
>>>> +
>>>> +static const VMStateDescription vmstate_locty = {
>>>> +    .name = "loc",
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 0,
>>>> +    .minimum_version_id_old = 0,
>>> I don't understand the problem there is leaving all the version fields
>>> to 0, just like CRB.
>>>
>>>> +    .fields      = (VMStateField[]) {
>>>> +        VMSTATE_UINT32(state, TPMLocality),
>>>> +        VMSTATE_UINT32(inte, TPMLocality),
>>>> +        VMSTATE_UINT32(ints, TPMLocality),
>>>> +        VMSTATE_UINT8(access, TPMLocality),
>>>> +        VMSTATE_UINT32(sts, TPMLocality),
>>>> +        VMSTATE_UINT32(iface_id, TPMLocality),
>>>> +        VMSTATE_END_OF_LIST(),
>>>> +    }
>>>> +};
>>>> +
>>>>    static const VMStateDescription vmstate_tpm_tis = {
>>>>        .name = "tpm",
>>>> -    .unmigratable = 1,
>>>> +    .version_id = 1,
>>>> +    .minimum_version_id = 0,
>>>> +    .minimum_version_id_old = 0,
>>> same
>>>
>>> If you remove the version fields: Reviewed-by: Marc-André Lureau
>>> <marcandre.lureau@redhat.com>
>>
>> This is the error I got when setting .version_id = 0 (on both) and doing a
>> localhost migration
>>
>> qemu-system-x86_64: Missing section footer for tpm-tis
>> qemu-system-x86_64: load of migration failed: Invalid argument
> It's clearly not the most friendly error message, but I debugged it,
> you just have to specify the right version for VMSTATE_STRUCT_ARRAY:
> 0.
>          VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 0,
>                               vmstate_locty, TPMLocality),
>
> Then it all works with version 0 (or default value)
>
> thanks
Thanks.