Re: [Qemu-devel] [PATCH V8 03/14] Add persistent state handling to TPM TIS frontend driver

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

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