Re: [PATCH 2/3] Add support for TPM devices over I2C bus

[Ninad Palsule <ninad@linux.vnet.ibm.com>]

On 3/21/23 6:54 PM, Stefan Berger wrote:
>
>
> On 3/21/23 01:30, Ninad Palsule wrote:
>> Qemu already supports devices attached to ISA and sysbus. This drop adds
>> support for the I2C bus attached TPM devices.
>>
>> This commit includes changes for the common code.
>> - Added support for the new checksum registers which are required for
>>    the I2C support. The checksum calculation is handled in the qemu
>>    common code.
>> - Added wrapper function for read and write data so that I2C code can
>>    call it without MMIO interface.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   hw/tpm/tpm_tis.h        |  2 ++
>>   hw/tpm/tpm_tis_common.c | 33 +++++++++++++++++++++++++++++++++
>>   include/hw/acpi/tpm.h   |  2 ++
>>   3 files changed, 37 insertions(+)
>>
>> diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
>> index f6b5872ba6..16b7baddd8 100644
>> --- a/hw/tpm/tpm_tis.h
>> +++ b/hw/tpm/tpm_tis.h
>> @@ -86,5 +86,7 @@ int tpm_tis_pre_save(TPMState *s);
>>   void tpm_tis_reset(TPMState *s);
>>   enum TPMVersion tpm_tis_get_tpm_version(TPMState *s);
>>   void tpm_tis_request_completed(TPMState *s, int ret);
>> +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size);
>> +void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, 
>> uint32_t size);
>>     #endif /* TPM_TPM_TIS_H */
>> diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
>> index 503be2a541..3c82f63179 100644
>> --- a/hw/tpm/tpm_tis_common.c
>> +++ b/hw/tpm/tpm_tis_common.c
>> @@ -26,6 +26,8 @@
>>   #include "hw/irq.h"
>>   #include "hw/isa/isa.h"
>>   #include "qapi/error.h"
>> +#include "qemu/bswap.h"
>> +#include "qemu/crc-ccitt.h"
>>   #include "qemu/module.h"
>>     #include "hw/acpi/tpm.h"
>> @@ -422,6 +424,9 @@ static uint64_t tpm_tis_mmio_read(void *opaque, 
>> hwaddr addr,
>>               shift = 0; /* no more adjustments */
>>           }
>>           break;
>> +    case TPM_TIS_REG_DATA_CSUM_GET:
>> +        val = bswap16(crc_ccitt(0, s->buffer, s->rw_offset));
>
> Should this not rather be cpu_to_be16() so that it would also work on 
> a big endian host (assuming you tested this on a little e endian host)?


Changed code to use cpu_to_be16. Yes, I did not run on big endian host.

>
>> +        break;
>>       case TPM_TIS_REG_INTERFACE_ID:
>>           val = s->loc[locty].iface_id;
>>           break;
>> @@ -447,6 +452,15 @@ static uint64_t tpm_tis_mmio_read(void *opaque, 
>> hwaddr addr,
>>       return val;
>>   }
>>   +/*
>> + * A wrapper read function so that it can be directly called without
>> + * mmio.
>> + */
>> +uint32_t tpm_tis_read_data(TPMState *s, hwaddr addr, unsigned size)
>> +{
>> +    return tpm_tis_mmio_read(s, addr, size);
>> +}
>> +
>>   /*
>>    * Write a value to a register of the TIS interface
>>    * See specs pages 33-63 for description of the registers
>> @@ -600,6 +614,15 @@ static void tpm_tis_mmio_write(void *opaque, 
>> hwaddr addr,
>>       case TPM_TIS_REG_INT_VECTOR:
>>           /* hard wired -- ignore */
>>           break;
>> +    case TPM_TIS_REG_DATA_CSUM_ENABLE:
>> +        /*
>> +         * Checksum implemented by common code so no need to set
>> +         * any flags.
>> +         */
>> +        break;
>> +    case TPM_TIS_REG_DATA_CSUM_GET:
>> +        /* This is readonly register so ignore */
>> +        break;
>>       case TPM_TIS_REG_INT_STATUS:
>>           if (s->active_locty != locty) {
>>               break;
>> @@ -703,6 +726,7 @@ static void tpm_tis_mmio_write(void *opaque, 
>> hwaddr addr,
>>           break;
>>       case TPM_TIS_REG_DATA_FIFO:
>>       case TPM_TIS_REG_DATA_XFIFO ... TPM_TIS_REG_DATA_XFIFO_END:
>> +
>
> you can remove this one
Sorry, I am not clear what you are asking me to remove.
>
>>           /* data fifo */
>>           if (s->active_locty != locty) {
>>               break;
>> @@ -767,6 +791,15 @@ static void tpm_tis_mmio_write(void *opaque, 
>> hwaddr addr,
>>       }
>>   }
>>   +/*
>> + * A wrapper write function so that it can be directly called without
>> + * mmio.
>> + */
>> +void tpm_tis_write_data(TPMState *s, hwaddr addr, uint64_t val, 
>> uint32_t size)
>> +{
>> +    tpm_tis_mmio_write(s, addr, val, size);
>> +}'
>> +
>>   const MemoryRegionOps tpm_tis_memory_ops = {
>>       .read = tpm_tis_mmio_read,
>>       .write = tpm_tis_mmio_write,
>> diff --git a/include/hw/acpi/tpm.h b/include/hw/acpi/tpm.h
>> index 559ba6906c..db12c002f4 100644
>> --- a/include/hw/acpi/tpm.h
>> +++ b/include/hw/acpi/tpm.h
>> @@ -40,6 +40,8 @@
>>   #define TPM_TIS_REG_STS                   0x18
>>   #define TPM_TIS_REG_DATA_FIFO             0x24
>>   #define TPM_TIS_REG_INTERFACE_ID          0x30
>> +#define TPM_TIS_REG_DATA_CSUM_ENABLE      0x40
>> +#define TPM_TIS_REG_DATA_CSUM_GET         0x44
>>   #define TPM_TIS_REG_DATA_XFIFO            0x80
>>   #define TPM_TIS_REG_DATA_XFIFO_END        0xbc
>>   #define TPM_TIS_REG_DID_VID               0xf00
>
> Looks good.


Thank you for the review!

Ninad Palsule