* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi Stefan
>
> On Thu, Mar 1, 2018 at 8:59 PM, Stefan Berger
> <stef...@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 <stef...@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 / dgilb...@redhat.com / Manchester, UK
