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

[Markus Armbruster]

Stefan Berger <address@hidden> 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 <address@hidden>
>
> ---
>  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.

> +{
> +    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."