Patchwork [V8,03/14] Add persistent state handling to TPM TIS frontend driver

login
register
mail settings
Submitter Stefan Berger
Date Aug. 31, 2011, 2:35 p.m.
Message ID <20110831143618.248943092@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/112556/
State New
Headers show

Comments

Stefan Berger - Aug. 31, 2011, 2:35 p.m.
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 <stefanb@linux.vnet.ibm.com>

---
 hw/tpm_tis.c |  166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)
Michael S. Tsirkin - Sept. 1, 2011, 5:20 p.m.
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 <stefanb@linux.vnet.ibm.com>
> 
> ---
>  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 <stefanb@us.ibm.com>
>   *         David Safford <safford@us.ibm.com>
>   *
> + * Xen 4 support: Andrease Niederl <andreas.niederl@iaik.tugraz.at>
> + *
>   * 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?
Stefan Berger - Sept. 2, 2011, 1:12 a.m.
On 09/01/2011 01:20 PM, Michael S. Tsirkin wrote:
> 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<stefanb@linux.vnet.ibm.com>
>>
>> ---
>>   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<stefanb@us.ibm.com>
>>    *         David Safford<safford@us.ibm.com>
>>    *
>> + * Xen 4 support: Andrease Niederl<andreas.niederl@iaik.tugraz.at>
>> + *
>>    * 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?
Even if the last action the TIS emulator was doing before the VM 
suspended completely was to reveive the last outstanding response then 
the tis_raise_irq() function was called in tis_tpm_receive_cb()  and 
along with that qemu_irq_raise(s->irq) was executed. Presumably it's not 
necessary to raise this same IRQ again immediately after resuming but 
this IRQ was restored as part of restoring the interrupt device model's 
state and the OS (still) sees the IRQ as pending.


>> +
>> +#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?
It's hard-wired to IRQ 5.
> If yes post load should do something with it?
> If not, why are we migrating it?
True. I'll remove it from the migrated state.
>> +
>> +        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?
>
>
Which flag are you referring to?

    Stefan
Paul Moore - Sept. 9, 2011, 9:13 p.m.
On Wednesday, August 31, 2011 10:35:54 AM Stefan Berger wrote:
> 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 <stefanb@us.ibm.com>
>   *         David Safford <safford@us.ibm.com>
>   *
> + * Xen 4 support: Andrease Niederl <andreas.niederl@iaik.tugraz.at>
> + *
>   * 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;

Is it safe to read s->active_locty without the state_lock?  I'm not sure at 
this point but I saw it being protected by the lock elsewhere ...

If the state_lock does not protect all of the structure, it might be nice to 
add some comments in the structure declaration explaining what fields are 
protected by the state_lock and which are not.

> +    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);

Hmm, this may be right, but it looks dangerous to me; can the active_locty 
change while the state_lock is dropped?  What about loc[locty].state?

> +            }
> +        }
> +    }
> +
> +#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) {

More concerns about loc[locty].state without the state_lock.

> +        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;

Same thing, just different fields ...

> +        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;

Again ...

> +        break;
> +        default:
> +            /* leak nothing */
> +            memset(s->buf, 0x0, sizeof(s->buf));

Maybe?

> +        break;
> +        }
> +    }
> +
> +    s->be_driver->ops->save_volatile_data();
> +}
Stefan Berger - Sept. 11, 2011, 4:45 p.m.
On 09/09/2011 05:13 PM, Paul Moore wrote:
> On Wednesday, August 31, 2011 10:35:54 AM Stefan Berger wrote:
>> 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<stefanb@us.ibm.com>
>>    *         David Safford<safford@us.ibm.com>
>>    *
>> + * Xen 4 support: Andrease Niederl<andreas.niederl@iaik.tugraz.at>
>> + *
>>    * 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;
> Is it safe to read s->active_locty without the state_lock?  I'm not sure at
> this point but I saw it being protected by the lock elsewhere ...
It cannot change anymore since no vCPU is in the TPM TIS emulation layer 
anymore but all we're doing is wait for the last outstanding command to 
be returned to use from the TPM thread.
I don't mind putting this reading into the critical section, though, 
just to have it be consistent.

> If the state_lock does not protect all of the structure, it might be nice to
> add some comments in the structure declaration explaining what fields are
> protected by the state_lock and which are not.
>
>> +    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);
> Hmm, this may be right, but it looks dangerous to me; can the active_locty
> change while the state_lock is dropped?  What about loc[locty].state?
This is correct since at this time the VM is not executing anymore, so 
no vCPU can be in the TPM TIS emulation code anymore, but we're waiting 
for the last outstanding TPM command finish processing in the TPM thread 
(to have it's response 'caught' and stored as part of the TPM TIS 
state). The locking is against the thread at this point that may change 
the .state variable, although I don't think it would be necessary to 
hold the lock there at all except for in the case where the condition is 
being waited for in the other else branch.
>> +            }
>> +        }
>> +    }
>> +
>> +#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) {
> More concerns about loc[locty].state without the state_lock.
>
The section you are quoting here is further down in the same function 
that prepares the TPM TIS for state serialization before final 
migration/suspend. At this point we have caught the last outstanding 
response from the TPM thread and that thread will not process any more 
commands at this point (queuing of commands it not possible with TPM TIS 
but strictly sending a single request to  it, have it processed, getting 
that response -- so the thread will be idle). Also since no more vCPU is 
in the TPM TIS emulation layer the state cannot change anymore. Again, 
also here I can have the critical section extended over this area.
>> +        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;
> Same thing, just different fields ...
>
>> +        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;
> Again ...
Ok, I can move that single qemu_mutex_unlock(&s->state_lock) above to 
after the switch() though I don't think it is necessary in this case due 
the state the emulation is in. Though I agree that the code 'looks' more 
correct.
>> +        break;
>> +        default:
>> +            /* leak nothing */
>> +            memset(s->buf, 0x0, sizeof(s->buf));
> Maybe?
>
What do you mean?
This command just makes sure that no previous response still stored in 
the TPM TIS buffer is being stored as part of the TPM TIS state 
serialization.

Thanks for the review.

    Stefan
>> +        break;
>> +        }
>> +    }
>> +
>> +    s->be_driver->ops->save_volatile_data();
>> +}
Paul Moore - Sept. 12, 2011, 9:16 p.m.
On Sunday, September 11, 2011 12:45:05 PM Stefan Berger wrote:
> On 09/09/2011 05:13 PM, Paul Moore wrote:
> > On Wednesday, August 31, 2011 10:35:54 AM Stefan Berger wrote:
> >> 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<stefanb@us.ibm.com>
> >>    *         David Safford<safford@us.ibm.com>
> >>    *
> >> 
> >> + * Xen 4 support: Andrease Niederl<andreas.niederl@iaik.tugraz.at>
> >> + *
> >> 
> >>    * 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;
> > 
> > Is it safe to read s->active_locty without the state_lock?  I'm not sure
> > at this point but I saw it being protected by the lock elsewhere ...
> It cannot change anymore since no vCPU is in the TPM TIS emulation layer
> anymore but all we're doing is wait for the last outstanding command to
> be returned to use from the TPM thread.
> I don't mind putting this reading into the critical section, though,
> just to have it be consistent.

[Dropping the rest of the comments since they all cover the same issue]

I'm a big fan of consistency, especially when it comes to locking; 
inconsistent lock usage can lead to confusion and that is almost never good. 

If we need a lock here because there is the potential for an outstanding TPM 
command, then I vote for locking in this function just as you would in any 
other.  However, if we really don't need locks here because the outstanding 
TPM command will have _no_ effect on the TPMState or any related structure, 
then just do away with the lock completely and make of note of it in the 
function explaining why.
Stefan Berger - Sept. 12, 2011, 11:37 p.m.
On 09/12/2011 05:16 PM, Paul Moore wrote:
> On Sunday, September 11, 2011 12:45:05 PM Stefan Berger wrote:
>> On 09/09/2011 05:13 PM, Paul Moore wrote:
>>> On Wednesday, August 31, 2011 10:35:54 AM Stefan Berger wrote:
>>>> 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<stefanb@us.ibm.com>
>>>>     *         David Safford<safford@us.ibm.com>
>>>>     *
>>>>
>>>> + * Xen 4 support: Andrease Niederl<andreas.niederl@iaik.tugraz.at>
>>>> + *
>>>>
>>>>     * 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;
>>> Is it safe to read s->active_locty without the state_lock?  I'm not sure
>>> at this point but I saw it being protected by the lock elsewhere ...
>> It cannot change anymore since no vCPU is in the TPM TIS emulation layer
>> anymore but all we're doing is wait for the last outstanding command to
>> be returned to use from the TPM thread.
>> I don't mind putting this reading into the critical section, though,
>> just to have it be consistent.
> [Dropping the rest of the comments since they all cover the same issue]
>
> I'm a big fan of consistency, especially when it comes to locking;
> inconsistent lock usage can lead to confusion and that is almost never good.
>
> If we need a lock here because there is the potential for an outstanding TPM
> command, then I vote for locking in this function just as you would in any
> other.  However, if we really don't need locks here because the outstanding
> TPM command will have _no_ effect on the TPMState or any related structure,
> then just do away with the lock completely and make of note of it in the
> function explaining why.
>
Let's give the consistency argument the favor and extend the locking 
over those parts that usually also get locked.

Regards,
     Stefan
Paul Moore - Sept. 13, 2011, 12:13 p.m.
On Monday, September 12, 2011 07:37:25 PM Stefan Berger wrote:
> On 09/12/2011 05:16 PM, Paul Moore wrote:
> > On Sunday, September 11, 2011 12:45:05 PM Stefan Berger wrote:
> >> On 09/09/2011 05:13 PM, Paul Moore wrote:
> >>> On Wednesday, August 31, 2011 10:35:54 AM Stefan Berger wrote:
> >>>> 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<stefanb@us.ibm.com>
> >>>>     *         David Safford<safford@us.ibm.com>
> >>>>     *
> >>>> 
> >>>> + * Xen 4 support: Andrease
> >>>> Niederl<andreas.niederl@iaik.tugraz.at>
> >>>> + *
> >>>> 
> >>>>     * 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;
> >>> 
> >>> Is it safe to read s->active_locty without the state_lock?  I'm not
> >>> sure at this point but I saw it being protected by the lock
> >>> elsewhere ...>> 
> >> It cannot change anymore since no vCPU is in the TPM TIS emulation
> >> layer
> >> anymore but all we're doing is wait for the last outstanding command
> >> to
> >> be returned to use from the TPM thread.
> >> I don't mind putting this reading into the critical section, though,
> >> just to have it be consistent.
> > 
> > [Dropping the rest of the comments since they all cover the same issue]
> > 
> > I'm a big fan of consistency, especially when it comes to locking;
> > inconsistent lock usage can lead to confusion and that is almost never
> > good.
> > 
> > If we need a lock here because there is the potential for an outstanding
> > TPM command, then I vote for locking in this function just as you would
> > in any other.  However, if we really don't need locks here because the
> > outstanding TPM command will have _no_ effect on the TPMState or any
> > related structure, then just do away with the lock completely and make
> > of note of it in the function explaining why.
> 
> Let's give the consistency argument the favor and extend the locking
> over those parts that usually also get locked.

Great, thanks.

Patch

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 <stefanb@us.ibm.com>
  *         David Safford <safford@us.ibm.com>
  *
+ * Xen 4 support: Andrease Niederl <andreas.niederl@iaik.tugraz.at>
+ *
  * 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;
+        }
+    }
+
+#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),
+
+        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)