From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:42250) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzB8M-00083G-Fn for qemu-devel@nongnu.org; Thu, 01 Sep 2011 13:32:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QzB8L-0008EU-2T for qemu-devel@nongnu.org; Thu, 01 Sep 2011 13:32:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3188) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QzB8K-0008E6-Q6 for qemu-devel@nongnu.org; Thu, 01 Sep 2011 13:32:57 -0400 Date: Thu, 1 Sep 2011 20:20:38 +0300 From: "Michael S. Tsirkin" Message-ID: <20110901172037.GE10989@redhat.com> References: <20110831143551.127339744@linux.vnet.ibm.com> <20110831143618.248943092@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110831143618.248943092@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH V8 03/14] Add persistent state handling to TPM TIS frontend driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Berger Cc: chrisw@redhat.com, anbang.ruan@cs.ox.ac.uk, qemu-devel@nongnu.org, rrelyea@redhat.com, alevy@redhat.com, andreas.niederl@iaik.tugraz.at, serge@hallyn.com On Wed, Aug 31, 2011 at 10:35:54AM -0400, Stefan Berger wrote: > This patch adds support for handling of persistent state to the TPM TIS > frontend. > > The currently used buffer is determined (can only be in currently active > locality and either be a read or a write buffer) and only that buffer's content > is stored. The reverse is done when the state is restored from disk > where the buffer's content are copied into the currently used buffer. > > To keep compatibility with existing Xen implementation the VMStateDescription > was adapted to be compatible with existing state. For that I am adding Andreas > Niederl as an author to the file. > > v5: > - removing qdev.no_user=1 > > v4: > - main thread releases the 'state' lock while periodically calling the > backends function that may request it to write data into block storage. > > v3: > - all functions prefixed with tis_ > - while the main thread is waiting for an outstanding TPM command to finish, > it periodically does some work (writes data to the block storage) > > Signed-off-by: Stefan Berger > > --- > hw/tpm_tis.c | 166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 166 insertions(+) > > Index: qemu-git/hw/tpm_tis.c > =================================================================== > --- qemu-git.orig/hw/tpm_tis.c > +++ qemu-git/hw/tpm_tis.c > @@ -6,6 +6,8 @@ > * Author: Stefan Berger > * David Safford > * > + * Xen 4 support: Andrease Niederl > + * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License as > * published by the Free Software Foundation, version 2 of the > @@ -839,3 +841,167 @@ static int tis_init(ISADevice *dev) > err_exit: > return -1; > } > + > +/* persistent state handling */ > + > +static void tis_pre_save(void *opaque) > +{ > + TPMState *s = opaque; > + uint8_t locty = s->active_locty; > + > + qemu_mutex_lock(&s->state_lock); > + > + /* wait for outstanding requests to complete */ > + if (IS_VALID_LOCTY(locty) && s->loc[locty].state == STATE_EXECUTION) { > + if (!s->be_driver->ops->job_for_main_thread) { > + qemu_cond_wait(&s->from_tpm_cond, &s->state_lock); > + } else { > + while (s->loc[locty].state == STATE_EXECUTION) { > + qemu_mutex_unlock(&s->state_lock); > + > + s->be_driver->ops->job_for_main_thread(NULL); > + usleep(10000); > + > + qemu_mutex_lock(&s->state_lock); > + } > + } > + } > + > +#ifdef DEBUG_TIS_SR > + fprintf(stderr, > + "tpm_tis: suspend: locty 0 : r_offset = %d, w_offset = %d\n", > + s->loc[0].r_offset, s->loc[0].w_offset); > + if (s->loc[0].r_offset) { > + tis_dump_state(opaque, 0); > + } > +#endif > + > + qemu_mutex_unlock(&s->state_lock); > + > + /* copy current active read or write buffer into the buffer > + written to disk */ > + if (IS_VALID_LOCTY(locty)) { > + switch (s->loc[locty].state) { > + case STATE_RECEPTION: > + memcpy(s->buf, > + s->loc[locty].w_buffer.buffer, > + MIN(sizeof(s->buf), > + s->loc[locty].w_buffer.size)); > + s->offset = s->loc[locty].w_offset; > + break; > + case STATE_COMPLETION: > + memcpy(s->buf, > + s->loc[locty].r_buffer.buffer, > + MIN(sizeof(s->buf), > + s->loc[locty].r_buffer.size)); > + s->offset = s->loc[locty].r_offset; > + break; > + default: > + /* leak nothing */ > + memset(s->buf, 0x0, sizeof(s->buf)); > + break; > + } > + } > + > + s->be_driver->ops->save_volatile_data(); > +} > + > + > +static int tis_post_load(void *opaque, > + int version_id __attribute__((unused))) > +{ > + TPMState *s = opaque; > + > + uint8_t locty = s->active_locty; > + > + if (IS_VALID_LOCTY(locty)) { > + switch (s->loc[locty].state) { > + case STATE_RECEPTION: > + memcpy(s->loc[locty].w_buffer.buffer, > + s->buf, > + MIN(sizeof(s->buf), > + s->loc[locty].w_buffer.size)); > + s->loc[locty].w_offset = s->offset; > + break; > + case STATE_COMPLETION: > + memcpy(s->loc[locty].r_buffer.buffer, > + s->buf, > + MIN(sizeof(s->buf), > + s->loc[locty].r_buffer.size)); > + s->loc[locty].r_offset = s->offset; > + break; > + default: > + break; > + } > + } Should this do something with interrupts as well? > + > +#ifdef DEBUG_TIS_SR > + fprintf(stderr, > + "tpm_tis: resume : locty 0 : r_offset = %d, w_offset = %d\n", > + s->loc[0].r_offset, s->loc[0].w_offset); > +#endif > + > + return s->be_driver->ops->load_volatile_data(s); > +} > + > + > +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_UINT8(sts, TPMLocality), > + VMSTATE_END_OF_LIST(), > + } > +}; > + > + > +static const VMStateDescription vmstate_tis = { > + .name = "tpm", > + .version_id = 1, > + .minimum_version_id = 0, > + .minimum_version_id_old = 0, > + .pre_save = tis_pre_save, > + .post_load = tis_post_load, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(irq_num, TPMState), > + VMSTATE_UINT32(offset, TPMState), > + VMSTATE_BUFFER(buf, TPMState), > + VMSTATE_UINT8(active_locty, TPMState), > + VMSTATE_UINT8(aborting_locty, TPMState), > + VMSTATE_UINT8(next_locty, TPMState), Is irq_num guest modifiable? If yes post load should do something with it? If not, why are we migrating it? > + > + VMSTATE_STRUCT_ARRAY(loc, TPMState, NUM_LOCALITIES, 1, > + vmstate_locty, TPMLocality), > + > + VMSTATE_END_OF_LIST() > + } > +}; > + > + > +static ISADeviceInfo tis_device_info = { > + .init = tis_init, > + .qdev.name = "tpm-tis", > + .qdev.size = sizeof(TPMState), > + .qdev.vmsd = &vmstate_tis, > + .qdev.reset = tis_reset, > + .qdev.props = (Property[]) { > + DEFINE_PROP_UINT32("irq", TPMState, > + irq_num, TPM_TIS_IRQ), > + DEFINE_PROP_STRING("tpmdev", TPMState, backend), > + DEFINE_PROP_END_OF_LIST(), > + }, > +}; > + > + > +static void tis_register_device(void) > +{ > + isa_qdev_register(&tis_device_info); > +} > + > +device_init(tis_register_device) > So this is a qdev device. Why do we need a new flag to set it up then?