Re: [Qemu-devel] [PATCH V15 1/7] Support for TPM command line options

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

On 03/27/2012 05:35 PM, Anthony Liguori wrote:
> On 03/27/2012 03:24 PM, Stefan Berger wrote:
>> This patch adds support for TPM command line options.
>> The command line options supported here are
>>
[...]
>> Monitor support for 'info tpm' has been added. It for example prints the
>> following:
>>
>> (qemu) info tpm
>> TPM devices:
>>   tpm0: model=tpm-tis
>>    \ tpm0: type=passthrough,path=/dev/tpm0
>>
>> Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
>> ---
[...]
>>
>> --- /dev/null
>> +++ b/hw/tpm_tis.h
>> @@ -0,0 +1,81 @@
>> +/*
>> + * tpm_tis.c - QEMU's TPM TIS interface emulator
>> + *
>> + * Copyright (C) 2006,2010,2011 IBM Corporation
>> + *
>> + * Authors:
>> + *  Stefan Berger<stefanb@us.ibm.com>
>> + *  David Safford<safford@us.ibm.com>
>> + *
>> + * 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
>> + * License.
>
> Or later please.  We're sticking with GPLv2 as our effective license 
> but asking that all new code is GPLv2 or later.

Forgot that one. Fixed.
>
>> + *
>> + *
>> + * Implementation of the TIS interface according to specs found at
>> + * http://www.trustedcomputiggroup.org
>> + *
>> + */
>> +#ifndef HW_TPM_TIS_H
>> +#define HW_TPM_TIS_H
>> +
>> +#include "isa.h"
>> +#include "qemu-thread.h"
>
> This shouldn't be needed anymore.

Indeed. Removed.+
>> +typedef struct TPMTISState {
>> +    uint32_t offset;
>> +    uint8_t buf[TPM_TIS_BUFFER_MAX];
>> +
>> +    uint8_t active_locty;
>> +    uint8_t aborting_locty;
>> +    uint8_t next_locty;
>> +
>> +    TPMLocality loc[TPM_TIS_NUM_LOCALITIES];
>> +
>> +    qemu_irq irq;
>> +    uint32_t irq_num;
>
> I'm a bit confused here.  If TPMTISState has an irq, shouldn't it be a 
> DeviceState?
>

So I guess you would expect the irq to be part of TPMState?diff --git 
a/qapi-schema.json b/qapi-schema.json

>> index 0d11d6e..4ad6d29 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1701,3 +1701,32 @@
>>   # Since: 1.1
>>   ##
>>   { 'command': 'xen-save-devices-state', 'data': {'filename': 'str'} }
>> +
>> +##
>> +# @TPMInfo:
>> +#
>> +# Information about the TPM
>> +#
>> +# @model: The TPM frontend model, i.e., tpm-tis
>> +#
>> +# @id: The ID of the TPM
>> +#
>> +# @type: The type of TPM backend, i.e., passthrough
>> +#
>> +# @parameters: Additional parameters of the TPM backend device
>
> This is in some sort of key=value format?  Why not specify those 
> parameters properly in the schema as optional items?
>

Yes, it's in key=value format and it is optional. So, I fixed this now 
with #optional and '*parameters'. Should it make
a difference in the code, except for the auto-generated code?
+
>> +/* overall state of the TPM interface */
>> +typedef struct TPMState {
>> +    ISADevice busdev;
>> +    MemoryRegion mmio;
>> +
>> +    union {
>> +        TPMTISState tis;
>> +    } s;
>> +
>> +    uint8_t     command_locty;
>> +    TPMLocality *cmd_locty;
>> +
>> +    char *backend;
>> +    TPMBackend *be_driver;
>> +} TPMState;
>
> I'm a bit confused at what the relationship between TPMTISState and 
> TPMSTate.

The rational is that TPMState could accomodate different types of 
front-ends in the union with the part in the union being private to the 
front-end. Right now there is only a TIS emulator, there could be a 
virtio (with the restriction that it would have to support TPMLocality). 
Obviously there is a backend driver necessary for this split between 
front- and backend to work, so this is what the 'char *backend' and 
TPMBackend *be_driver are good for. cmd_locty is shared between front 
and backend and is set by the frontend and read by the backend.

Regards,
    Stefan