Re: [Qemu-devel] [PATCH V4 02/10] Add TPM (frontend) hardware interface (TPM TIS) to Qemu

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

On 05/18/2011 03:23 AM, Markus Armbruster wrote:
> Stefan Berger<stefanb@linux.vnet.ibm.com>  writes:
>
>> This patch adds the main code of the TPM frontend driver, the TPM TIS
>> interface, to Qemu. The code is largely based on my previous implementation
>> for Xen but has been significantly extended to meet the standard's
>> requirements, such as the support for changing of localities and all the
>> functionality of the available flags.
>>
>> Communication with the backend (i.e., for Xen or the libtpms-based one)
>> is cleanly separated through an interface which the backend driver needs
>> to implement.
>>
>> The TPM TIS driver's backend was previously chosen in the code added
>> to arch_init. The frontend holds a pointer to the chosen backend (interface).
>>
>> Communication with the backend is largely based on signals and conditions.
>> Whenever the frontend has collected a complete packet, it will signal
>> the backend, which then starts processing the command. Once the result
>> has been returned, the backend invokes a callback function
>> (tis_tpm_receive_cb()).
>>
>> The one tricky part is support for VM suspend while the TPM is processing
>> a command. In this case the frontend driver is waiting for the backend
>> to return the result of the last command before shutting down. It waits
>> on a condition for a signal from the backend, which is delivered in
>> tis_tpm_receive_cb().
>>
>> Testing the proper functioning of the different flags and localities
>> cannot be done from user space when running in Linux for example, since
>> access to the address space of the TPM TIS interface is not possible. Also
>> the Linux driver itself does not exercise all functionality. So, for
>> testing there is a fairly extensive test suite as part of the SeaBIOS patches
>> since from within the BIOS one can have full access to all the TPM's registers.
>>
>> v3:
>>    - prefixing functions with tis_
>>    - added a function to the backend interface 'early_startup_tpm' that
>>      allows to detect the presence of the block storage and gracefully fails
>>      Qemu if it's not available. This works with migration using shared
>>      storage but doesn't support migration with block storage migration.
>>      For encyrypted QCoW2 and in case of a snapshot resue the late_startup_tpm
>>      interface function is called
>>
>> Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
>>
>> ---
>>   hw/tpm_tis.c |  871 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 871 insertions(+)
>>
>> Index: qemu-git/hw/tpm_tis.c
>> ===================================================================
>> --- /dev/null
>> +++ qemu-git/hw/tpm_tis.c
> [...]
>> +static void tis_reset(DeviceState *d)
>> +{
>> +    TPMState *s = container_of(d, TPMState, busdev.qdev);
>> +    tis_s_reset(s);
>> +}
>> +
>> +
>> +static int tis_init(ISADevice *dev)
> Function not used, sure this compiles with -Werror?  If not, it needs
> fixing.
>
> As far as I can see, the use is added in 03/10, where you add the
> missing qdev parts.  Makes it hard to review for qdev sanity, so I did
> *not* do that.
>
I tried to split up the patches in smaller parts and had to cut 
'somewhere'.
Without additional compile option it does not report any errors.

>> +{
>> +    TPMState *s = DO_UPCAST(TPMState, busdev, dev);
>> +    int iomemtype, rc;
>> +
>> +    qemu_mutex_init(&s->state_lock);
>> +    qemu_cond_init(&s->from_tpm_cond);
>> +    qemu_cond_init(&s->to_tpm_cond);
>> +
>> +    if (active_be->init(s, tis_tpm_receive_cb)) {
>> +        goto err_exit;
>> +    }
>> +
>> +    isa_init_irq(dev,&s->irq, s->irq_num);
>> +
>> +    iomemtype = cpu_register_io_memory(tis_readfn, tis_writefn, s,
>> +                                       DEVICE_LITTLE_ENDIAN);
>> +    cpu_register_physical_memory(TIS_ADDR_BASE, 0x1000 * NUM_LOCALITIES,
>> +                                 iomemtype);
>> +
>> +    /*
>> +     * startup the TPM backend early to detect problems early
>> +     */
>> +    rc = tis_do_early_startup_tpm(s);
>> +    if (rc != 0&&  rc != -ENOKEY) {
>> +        fprintf(stderr,"tpm_tis: Fatal error accessing TPM's block storage.\n");
>> +        goto err_exit;
>> +    }
>> +
>> +    return 0;
>> +
>> + err_exit:
>> +    return -1;
>> +}
>> +
> Please fix "new blank line at EOF."
Will do.

Thanks.
    Stefan