[PATCH 0/3] Add support for TPM devices over I2C bus

[PATCH 0/3] Add support for TPM devices over I2C bus

[Ninad Palsule]

This drop adds support for the TPM devices attached to the I2C bus. It
only supports the TPM2 protocol. You need to run it with the external
TPM emulator like swtpm. I have tested it with swtpm.

I have refered to the work done by zhdaniel@meta.com but at the core
level out implementation is different.
https://github.com/theopolis/qemu/commit/2e2e57cde9e419c36af8071bb85392ad1ed70966 

Based-on: $MESSAGE_ID

Ninad Palsule (3):
  Add support for TPM devices over I2C bus
  Add support for TPM devices over I2C bus
  Add support for TPM devices over I2C bus

 docs/specs/tpm.rst      |   5 +-
 hw/tpm/meson.build      |   1 +
 hw/tpm/tpm_tis.h        |   2 +
 hw/tpm/tpm_tis_common.c |  33 ++++
 hw/tpm/tpm_tis_i2c.c    | 342 ++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/tpm.h   |   2 +
 include/sysemu/tpm.h    |   3 +
 7 files changed, 387 insertions(+), 1 deletion(-)
 create mode 100644 hw/tpm/tpm_tis_i2c.c

-- 
2.37.2

[PATCH 1/3] Add support for TPM devices over I2C bus

[Ninad Palsule]

This is a documentation change for I2C TPM device support.

Qemu already supports devices attached to ISA and sysbus.
This drop adds support for the I2C bus attached TPM devices.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 docs/specs/tpm.rst | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
index 535912a92b..79a79f0640 100644
--- a/docs/specs/tpm.rst
+++ b/docs/specs/tpm.rst
@@ -21,11 +21,14 @@ QEMU files related to TPM TIS interface:
  - ``hw/tpm/tpm_tis_common.c``
  - ``hw/tpm/tpm_tis_isa.c``
  - ``hw/tpm/tpm_tis_sysbus.c``
+ - ``hw/tpm/tpm_tis_i2c.c``
  - ``hw/tpm/tpm_tis.h``
 
 Both an ISA device and a sysbus device are available. The former is
 used with pc/q35 machine while the latter can be instantiated in the
-Arm virt machine.
+Arm virt machine. An I2C device support is also added which can be
+instantiated in the arm based emulation machine. I2C model only supports
+TPM2 protocol.
 
 CRB interface
 -------------
-- 
2.37.2

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

[Stefan Berger]

On 3/21/23 01:29, Ninad Palsule wrote:
> This is a documentation change for I2C TPM device support.
> 
> Qemu already supports devices attached to ISA and sysbus.
> This drop adds support for the I2C bus attached TPM devices.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>   docs/specs/tpm.rst | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
> index 535912a92b..79a79f0640 100644
> --- a/docs/specs/tpm.rst
> +++ b/docs/specs/tpm.rst
> @@ -21,11 +21,14 @@ QEMU files related to TPM TIS interface:
>    - ``hw/tpm/tpm_tis_common.c``
>    - ``hw/tpm/tpm_tis_isa.c``
>    - ``hw/tpm/tpm_tis_sysbus.c``
> + - ``hw/tpm/tpm_tis_i2c.c``
>    - ``hw/tpm/tpm_tis.h``
>   
>   Both an ISA device and a sysbus device are available. The former is
>   used with pc/q35 machine while the latter can be instantiated in the
> -Arm virt machine.
> +Arm virt machine. An I2C device support is also added which can be
> +instantiated in the arm based emulation machine. I2C model only supports
> +TPM2 protocol.


An I2C device is also supported for the Arm virt machine. This device only supports the TPM 2 protocol.

    Stefan

>   
>   CRB interface
>   -------------

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

[Ninad Palsule]

On 3/21/23 6:35 PM, Stefan Berger wrote:
>
>
> On 3/21/23 01:29, Ninad Palsule wrote:
>> This is a documentation change for I2C TPM device support.
>>
>> Qemu already supports devices attached to ISA and sysbus.
>> This drop adds support for the I2C bus attached TPM devices.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   docs/specs/tpm.rst | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/specs/tpm.rst b/docs/specs/tpm.rst
>> index 535912a92b..79a79f0640 100644
>> --- a/docs/specs/tpm.rst
>> +++ b/docs/specs/tpm.rst
>> @@ -21,11 +21,14 @@ QEMU files related to TPM TIS interface:
>>    - ``hw/tpm/tpm_tis_common.c``
>>    - ``hw/tpm/tpm_tis_isa.c``
>>    - ``hw/tpm/tpm_tis_sysbus.c``
>> + - ``hw/tpm/tpm_tis_i2c.c``
>>    - ``hw/tpm/tpm_tis.h``
>>     Both an ISA device and a sysbus device are available. The former is
>>   used with pc/q35 machine while the latter can be instantiated in the
>> -Arm virt machine.
>> +Arm virt machine. An I2C device support is also added which can be
>> +instantiated in the arm based emulation machine. I2C model only 
>> supports
>> +TPM2 protocol.
>
>
> An I2C device is also supported for the Arm virt machine. This device 
> only supports the TPM 2 protocol.
>
>    Stefan
>
Updated the document.

Thank you for the review!


Ninad Palsule


>>     CRB interface
>>   -------------

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

[Ninad Palsule]

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));
+        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:
+
         /* 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
-- 
2.37.2

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

[Stefan Berger]

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)?

> +        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

>           /* 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.

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

[Ninad Palsule]

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

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

[Stefan Berger]

On 3/22/23 07:18, Ninad Palsule wrote:
> 
> On 3/21/23 6:54 PM, Stefan Berger wrote:


>>> @@ -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.

You added an empty line here.

    Stefan

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

[Ninad Palsule]

On 3/22/23 6:24 AM, Stefan Berger wrote:
>
>
> On 3/22/23 07:18, Ninad Palsule wrote:
>>
>> On 3/21/23 6:54 PM, Stefan Berger wrote:
>
>
>>>> @@ -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.
>
> You added an empty line here.
>
>    Stefan


Ah,  Fixed it.

Thank you for the review.

Ninad Palsule

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

[Stefan Berger]

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.

> @@ -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.
> +         */

Can you intercept handling this register on the I2C layer and add a byte for its value so that it can be set correctly? We do want to be able to write bit 0 to it to enable it and allow reading of bit 0 to see what the state is. I don't want this byte of state on the TIS layer since this creates state incompatibilities.

And for getting the checksum value it should be also handled on the I2C layer and ask tpm_tis_common.c to run crc_ccitt(0, s->buffer, s->rw_offset) via a function call.

    Stefan

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

[Ninad Palsule]

On 3/22/23 7:05 AM, 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.
>
>> @@ -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.
>> +         */
>
> Can you intercept handling this register on the I2C layer and add a 
> byte for its value so that it can be set correctly? We do want to be 
> able to write bit 0 to it to enable it and allow reading of bit 0 to 
> see what the state is. I don't want this byte of state on the TIS 
> layer since this creates state incompatibilities.
>
> And for getting the checksum value it should be also handled on the 
> I2C layer and ask tpm_tis_common.c to run crc_ccitt(0, s->buffer, 
> s->rw_offset) via a function call.
>
>    Stefan


Sure, Fix it by handling it in I2C and removed newly defined registers 
from TIS layer.

Thanks for the review.

Ninad Palsule

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

[Ninad Palsule]

Qemu already supports devices attached to ISA and sysbus. This drop adds
support for the I2C bus attached TPM devices. I2C model only supports
TPM2 protocol.

This commit includes changes for the common code.
- Added I2C emulation model. Logic was added in the model to temporarily
  cache the data as I2C interface works per byte basis.
- New tpm type "tpm-tis-i2c" added for I2C support. User specify this
  string on command line.

Testing:
  TPM I2C device modulte is tested using SWTPM (software based TPM
  package). The qemu used the rainier machine and it was connected to
  swtpm over the socket interface.

  The command to start swtpm is as follows:
  $ swtpm socket --tpmstate dir=/tmp/mytpm1    \
                 --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock  \
                 --tpm2 --log level=100

  The command to start qemu is as follows:
  $ qemu-system-arm -M rainier-bmc -nographic \
            -kernel ${IMAGEPATH}/fitImage-linux.bin \
            -dtb ${IMAGEPATH}/aspeed-bmc-ibm-rainier.dtb \
            -initrd ${IMAGEPATH}/obmc-phosphor-initramfs.rootfs.cpio.xz \
            -drive file=${IMAGEPATH}/obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2 \
            -net nic -net user,hostfwd=:127.0.0.1:2222-:22,hostfwd=:127.0.0.1:2443-:443 \
            -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
            -tpmdev emulator,id=tpm0,chardev=chrtpm \
            -device tpm-tis-i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e

  Note: Currently you need to specify the I2C bus and device address on
        command line. In future we can add a device at board level.

Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
---
 hw/tpm/meson.build   |   1 +
 hw/tpm/tpm_tis_i2c.c | 342 +++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/tpm.h |   3 +
 3 files changed, 346 insertions(+)
 create mode 100644 hw/tpm/tpm_tis_i2c.c

diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
index 7abc2d794a..76fe3cb098 100644
--- a/hw/tpm/meson.build
+++ b/hw/tpm/meson.build
@@ -1,6 +1,7 @@
 softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
+softmmu_ss.add(when: 'CONFIG_TPM_TIS_I2C', if_true: files('tpm_tis_i2c.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_ppi.c'))
 softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_ppi.c'))
diff --git a/hw/tpm/tpm_tis_i2c.c b/hw/tpm/tpm_tis_i2c.c
new file mode 100644
index 0000000000..3c45af4140
--- /dev/null
+++ b/hw/tpm/tpm_tis_i2c.c
@@ -0,0 +1,342 @@
+/*
+ * tpm_tis_i2c.c - QEMU's TPM TIS I2C Device
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ * Implementation of the TIS interface according to specs found at
+ * http://www.trustedcomputinggroup.org. This implementation currently
+ * supports version 1.3, 21 March 2013
+ * In the developers menu choose the PC Client section then find the TIS
+ * specification.
+ *
+ * TPM TIS for TPM 2 implementation following TCG PC Client Platform
+ * TPM Profile (PTP) Specification, Familiy 2.0, Revision 00.43
+ */
+
+#include "qemu/osdep.h"
+#include "hw/i2c/i2c.h"
+#include "hw/qdev-properties.h"
+#include "hw/acpi/tpm.h"
+#include "migration/vmstate.h"
+#include "tpm_prop.h"
+#include "tpm_tis.h"
+#include "qom/object.h"
+#include "block/aio.h"
+#include "qemu/main-loop.h"
+
+/* TPM TIS I2C registers */
+#define TPM_TIS_I2C_REG_LOC_SEL          0x00
+#define TPM_TIS_I2C_REG_ACCESS           0x04
+#define TPM_TIS_I2C_REG_INT_ENABLE       0x08
+#define TPM_TIS_I2C_REG_INT_CAPABILITY   0x14
+#define TPM_TIS_I2C_REG_STS              0x18
+#define TPM_TIS_I2C_REG_DATA_FIFO        0x24
+#define TPM_TIS_I2C_REG_INTF_CAPABILITY  0x30
+#define TPM_TIS_I2C_REG_DATA_CSUM_ENABLE 0x40
+#define TPM_TIS_I2C_REG_DATA_CSUM_GET    0x44
+#define TPM_TIS_I2C_REG_DID_VID          0x48
+#define TPM_TIS_I2C_REG_RID              0x4c
+#define TPM_TIS_I2C_REG_UNKNOWN          0xff
+
+/* Operations */
+#define OP_SEND   1
+#define OP_RECV   2
+
+typedef struct TPMStateI2C {
+    /*< private >*/
+    I2CSlave parent_obj;
+
+    int      offset;     /* offset in to data[] */
+    int      size;       /* Size of the current reg data */
+    uint8_t  operation;  /* OP_SEND & OP_RECV */
+    uint8_t  data[4096]; /* Data */
+
+    /*< public >*/
+    TPMState state; /* not a QOM object */
+
+} TPMStateI2C;
+
+DECLARE_INSTANCE_CHECKER(TPMStateI2C, TPM_TIS_I2C,
+                         TYPE_TPM_TIS_I2C)
+
+static const VMStateDescription vmstate_tpm_tis_i2c = {
+    .name = "tpm",
+    .unmigratable = 1,
+};
+
+/* Register map */
+typedef struct reg_map {
+    uint16_t  i2c_reg;    /* I2C register */
+    uint16_t  tis_reg;    /* TIS register */
+    uint32_t  data_size;  /* data size expected */
+} i2c_reg_map;
+
+#define TPM_I2C_MAP_COUNT 11
+
+/*
+ * The register values in the common code is different than the latest
+ * register numbers as per the spec hence add the conversion map
+ */
+i2c_reg_map tpm_tis_reg_map[] = {
+    { TPM_TIS_I2C_REG_LOC_SEL,          TPM_TIS_REG_ACCESS,           1, },
+    { TPM_TIS_I2C_REG_ACCESS,           TPM_TIS_REG_ACCESS,           1, },
+    { TPM_TIS_I2C_REG_INT_ENABLE,       TPM_TIS_REG_INT_ENABLE,       4, },
+    { TPM_TIS_I2C_REG_INT_CAPABILITY,   TPM_TIS_REG_INT_VECTOR,       4, },
+    { TPM_TIS_I2C_REG_STS,              TPM_TIS_REG_STS,              4, },
+    { TPM_TIS_I2C_REG_DATA_FIFO,        TPM_TIS_REG_DATA_FIFO,        0, },
+    { TPM_TIS_I2C_REG_INTF_CAPABILITY,  TPM_TIS_REG_INTF_CAPABILITY,  4, },
+    { TPM_TIS_I2C_REG_DATA_CSUM_ENABLE, TPM_TIS_REG_DATA_CSUM_ENABLE, 1, },
+    { TPM_TIS_I2C_REG_DATA_CSUM_GET,    TPM_TIS_REG_DATA_CSUM_GET,    2, },
+    { TPM_TIS_I2C_REG_DID_VID,          TPM_TIS_REG_DID_VID,          4, },
+    { TPM_TIS_I2C_REG_RID,              TPM_TIS_REG_RID,              1, },
+};
+
+static inline uint16_t tpm_tis_i2c_to_tis_reg(uint64_t i2c_reg, int *size)
+{
+    uint16_t tis_reg = TPM_TIS_I2C_REG_UNKNOWN;
+    i2c_reg_map *reg_map;
+    int i;
+
+    for (i = 0; i < TPM_I2C_MAP_COUNT; i++) {
+        reg_map = &tpm_tis_reg_map[i];
+        if (reg_map->i2c_reg == i2c_reg) {
+            tis_reg = reg_map->tis_reg;
+            *size = reg_map->data_size;
+            break;
+        }
+    }
+
+    assert(tis_reg != TPM_TIS_I2C_REG_UNKNOWN);
+    return tis_reg;
+}
+
+/* Initialize the cached data */
+static inline void tpm_tis_i2c_init_cache(TPMStateI2C *i2cst)
+{
+    /* Clear operation and offset */
+    i2cst->operation = 0;
+    i2cst->offset = 0;
+    i2cst->size = 0;
+
+    return;
+}
+
+/* Send data to TPM */
+static inline void tpm_tis_i2c_tpm_send(TPMStateI2C *i2cst)
+{
+    if ((i2cst->operation == OP_SEND) && (i2cst->offset > 1)) {
+        uint16_t tis_reg;
+        uint32_t data;
+        int      i;
+
+        tis_reg = tpm_tis_i2c_to_tis_reg(i2cst->data[0], &i2cst->size);
+
+        /* Index 0 is always a register */
+        for (i = 1; i < i2cst->offset; i++) {
+            data = (i2cst->data[i] & 0xff);
+            tpm_tis_write_data(&i2cst->state, tis_reg, data, 1);
+        }
+
+        tpm_tis_i2c_init_cache(i2cst);
+    }
+
+    return;
+}
+
+/* Callback from TPM to indicate that response is copied */
+static void tpm_tis_i2c_request_completed(TPMIf *ti, int ret)
+{
+    TPMStateI2C *i2cst = TPM_TIS_I2C(ti);
+    TPMState *s = &i2cst->state;
+
+    /* Inform the common code. */
+    tpm_tis_request_completed(s, ret);
+}
+
+static enum TPMVersion tpm_tis_i2c_get_tpm_version(TPMIf *ti)
+{
+    TPMStateI2C *i2cst = TPM_TIS_I2C(ti);
+    TPMState *s = &i2cst->state;
+
+    return tpm_tis_get_tpm_version(s);
+}
+
+static int tpm_tis_i2c_event(I2CSlave *i2c, enum i2c_event event)
+{
+    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
+    int ret = 0;
+
+    switch (event) {
+    case I2C_START_RECV:
+        break;
+    case I2C_START_SEND:
+        tpm_tis_i2c_init_cache(i2cst);
+        break;
+    case I2C_FINISH:
+        if (i2cst->operation == OP_SEND) {
+            tpm_tis_i2c_tpm_send(i2cst);
+        } else {
+            tpm_tis_i2c_init_cache(i2cst);
+        }
+        break;
+    default:
+        break;
+    }
+
+    return ret;
+}
+
+/* If data is for FIFO then it is received from tpm_tis_common buffer
+ * otherwise it will be handled using single call to common code and
+ * cached in the local buffer.
+ */
+static uint8_t tpm_tis_i2c_recv(I2CSlave *i2c)
+{
+    int ret = 0;
+    int i, j;
+    uint32_t addr;
+    uint32_t data_read;
+    uint16_t i2c_reg;
+    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
+    TPMState *s = &i2cst->state;
+
+    if (i2cst->operation == OP_RECV) {
+
+        /* Special handling for FIFO */
+        if (i2cst->data[0] == TPM_TIS_I2C_REG_DATA_FIFO) {
+            i2c_reg = i2cst->data[0];
+            addr = tpm_tis_i2c_to_tis_reg(i2c_reg, &i2cst->size);
+            data_read = tpm_tis_read_data(s, addr, 1);
+            ret = (data_read & 0xff);
+        } else
+            ret = i2cst->data[i2cst->offset++];
+
+    } else if ((i2cst->operation == OP_SEND) && (i2cst->offset < 2)) {
+        i2c_reg = i2cst->data[0];
+
+        i2cst->operation = OP_RECV;
+        i2cst->offset = 0;
+
+        addr = tpm_tis_i2c_to_tis_reg(i2c_reg, &i2cst->size);
+
+        /* Special handling for FIFO register */
+        if (i2c_reg == TPM_TIS_I2C_REG_DATA_FIFO) {
+            data_read = tpm_tis_read_data(s, addr, 1);
+            ret = (data_read & 0xff);
+        } else {
+            /*
+             * Save the data in the data field. Save it in the little
+             * endian format.
+             */
+            for (i = 0; i < i2cst->size;) {
+                data_read = tpm_tis_read_data(s, addr, 4);
+                for (j = 0; j < 4; j++) {
+                    i2cst->data[i++] = (data_read & 0xff);
+                    data_read >>= 8;
+                }
+            }
+
+            /* Return first byte with this call */
+            ret = i2cst->data[i2cst->offset++];
+        }
+    } else
+        i2cst->operation = OP_RECV;
+
+    return ret;
+}
+
+/*
+ * Send function only remembers data in the buffer and then calls
+ * TPM TIS common code during FINISH event.
+ */
+static int tpm_tis_i2c_send(I2CSlave *i2c, uint8_t data)
+{
+    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
+
+    /* Remember data locally */
+    i2cst->operation = OP_SEND;
+    i2cst->data[i2cst->offset++] = data;
+
+    return 0;
+}
+
+static Property tpm_tis_i2c_properties[] = {
+    DEFINE_PROP_UINT32("irq", TPMStateI2C, state.irq_num, TPM_TIS_IRQ),
+    DEFINE_PROP_TPMBE("tpmdev", TPMStateI2C, state.be_driver),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void tpm_tis_i2c_realizefn(DeviceState *dev, Error **errp)
+{
+    TPMStateI2C *i2cst = TPM_TIS_I2C(dev);
+    TPMState *s = &i2cst->state;
+
+    if (!tpm_find()) {
+        error_setg(errp, "at most one TPM device is permitted");
+        return;
+    }
+
+    /* Get the backend pointer. It is not initialized propery during
+     * device_class_set_props
+     */
+    s->be_driver = qemu_find_tpm_be("tpm0");
+
+    if (!s->be_driver) {
+        error_setg(errp, "'tpmdev' property is required");
+        return;
+    }
+    if (s->irq_num > 15) {
+        error_setg(errp, "IRQ %d is outside valid range of 0 to 15",
+                   s->irq_num);
+        return;
+    }
+}
+
+static void tpm_tis_i2c_reset(DeviceState *dev)
+{
+    TPMStateI2C *i2cst = TPM_TIS_I2C(dev);
+    TPMState *s = &i2cst->state;
+
+    tpm_tis_i2c_init_cache(i2cst);
+
+    return tpm_tis_reset(s);
+}
+
+static void tpm_tis_i2c_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+    TPMIfClass *tc = TPM_IF_CLASS(klass);
+
+    dc->realize = tpm_tis_i2c_realizefn;
+    dc->reset = tpm_tis_i2c_reset;
+    dc->vmsd = &vmstate_tpm_tis_i2c;
+    device_class_set_props(dc, tpm_tis_i2c_properties);
+
+    k->event = tpm_tis_i2c_event;
+    k->recv = tpm_tis_i2c_recv;
+    k->send = tpm_tis_i2c_send;
+
+    tc->model = TPM_MODEL_TPM_TIS;
+    tc->request_completed = tpm_tis_i2c_request_completed;
+    tc->get_version = tpm_tis_i2c_get_tpm_version;
+}
+
+static const TypeInfo tpm_tis_i2c_info = {
+    .name          = TYPE_TPM_TIS_I2C,
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(TPMStateI2C),
+    .class_init    = tpm_tis_i2c_class_init,
+        .interfaces = (InterfaceInfo[]) {
+        { TYPE_TPM_IF },
+        { }
+    }
+};
+
+static void tpm_tis_i2c_register_types(void)
+{
+    type_register_static(&tpm_tis_i2c_info);
+}
+
+type_init(tpm_tis_i2c_register_types)
diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
index fb40e30ff6..66e3b45f30 100644
--- a/include/sysemu/tpm.h
+++ b/include/sysemu/tpm.h
@@ -48,6 +48,7 @@ struct TPMIfClass {
 #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
 #define TYPE_TPM_CRB                "tpm-crb"
 #define TYPE_TPM_SPAPR              "tpm-spapr"
+#define TYPE_TPM_TIS_I2C            "tpm-tis-i2c"
 
 #define TPM_IS_TIS_ISA(chr)                         \
     object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
@@ -57,6 +58,8 @@ struct TPMIfClass {
     object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
 #define TPM_IS_SPAPR(chr)                           \
     object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
+#define TPM_IS_TIS_I2C(chr)                      \
+    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_I2C)
 
 /* returns NULL unless there is exactly one TPM device */
 static inline TPMIf *tpm_find(void)
-- 
2.37.2

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

[Stefan Berger]

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. I2C model only supports
> TPM2 protocol.
> 
> This commit includes changes for the common code.
> - Added I2C emulation model. Logic was added in the model to temporarily
>    cache the data as I2C interface works per byte basis.
> - New tpm type "tpm-tis-i2c" added for I2C support. User specify this
>    string on command line.
> 
> Testing:
>    TPM I2C device modulte is tested using SWTPM (software based TPM
>    package). The qemu used the rainier machine and it was connected to
>    swtpm over the socket interface.
> 
>    The command to start swtpm is as follows:
>    $ swtpm socket --tpmstate dir=/tmp/mytpm1    \
>                   --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock  \
>                   --tpm2 --log level=100
> 
>    The command to start qemu is as follows:
>    $ qemu-system-arm -M rainier-bmc -nographic \
>              -kernel ${IMAGEPATH}/fitImage-linux.bin \
>              -dtb ${IMAGEPATH}/aspeed-bmc-ibm-rainier.dtb \
>              -initrd ${IMAGEPATH}/obmc-phosphor-initramfs.rootfs.cpio.xz \
>              -drive file=${IMAGEPATH}/obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2 \
>              -net nic -net user,hostfwd=:127.0.0.1:2222-:22,hostfwd=:127.0.0.1:2443-:443 \
>              -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
>              -tpmdev emulator,id=tpm0,chardev=chrtpm \
>              -device tpm-tis-i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e


Please add this command line example also to the documentation.

When you run scripts/checkpatch.pl over this patch it reports the following relevant complaints:

WARNING: Block comments use a leading /* on a separate line
#255: FILE: hw/tpm/tpm_tis_i2c.c:190:
+/* If data is for FIFO then it is received from tpm_tis_common buffer

WARNING: Block comments use a leading /* on a separate line
#345: FILE: hw/tpm/tpm_tis_i2c.c:280:
+    /* Get the backend pointer. It is not initialized propery during



> 
>    Note: Currently you need to specify the I2C bus and device address on
>          command line. In future we can add a device at board level.
> 
> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
> ---
>   hw/tpm/meson.build   |   1 +
>   hw/tpm/tpm_tis_i2c.c | 342 +++++++++++++++++++++++++++++++++++++++++++
>   include/sysemu/tpm.h |   3 +
>   3 files changed, 346 insertions(+)
>   create mode 100644 hw/tpm/tpm_tis_i2c.c
> 
> diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
> index 7abc2d794a..76fe3cb098 100644
> --- a/hw/tpm/meson.build
> +++ b/hw/tpm/meson.build
> @@ -1,6 +1,7 @@
>   softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_tis_common.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: files('tpm_tis_isa.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: files('tpm_tis_sysbus.c'))
> +softmmu_ss.add(when: 'CONFIG_TPM_TIS_I2C', if_true: files('tpm_tis_i2c.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_ppi.c'))
>   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_ppi.c'))
> diff --git a/hw/tpm/tpm_tis_i2c.c b/hw/tpm/tpm_tis_i2c.c
> new file mode 100644
> index 0000000000..3c45af4140
> --- /dev/null
> +++ b/hw/tpm/tpm_tis_i2c.c
> @@ -0,0 +1,342 @@
> +/*
> + * tpm_tis_i2c.c - QEMU's TPM TIS I2C Device
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + * Implementation of the TIS interface according to specs found at
> + * http://www.trustedcomputinggroup.org. This implementation currently
> + * supports version 1.3, 21 March 2013
> + * In the developers menu choose the PC Client section then find the TIS
> + * specification.
> + *
> + * TPM TIS for TPM 2 implementation following TCG PC Client Platform
> + * TPM Profile (PTP) Specification, Familiy 2.0, Revision 00.43
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/acpi/tpm.h"
> +#include "migration/vmstate.h"
> +#include "tpm_prop.h"
> +#include "tpm_tis.h"
> +#include "qom/object.h"
> +#include "block/aio.h"
> +#include "qemu/main-loop.h"
> +
> +/* TPM TIS I2C registers */
> +#define TPM_TIS_I2C_REG_LOC_SEL          0x00
> +#define TPM_TIS_I2C_REG_ACCESS           0x04
> +#define TPM_TIS_I2C_REG_INT_ENABLE       0x08
> +#define TPM_TIS_I2C_REG_INT_CAPABILITY   0x14
> +#define TPM_TIS_I2C_REG_STS              0x18
> +#define TPM_TIS_I2C_REG_DATA_FIFO        0x24
> +#define TPM_TIS_I2C_REG_INTF_CAPABILITY  0x30
> +#define TPM_TIS_I2C_REG_DATA_CSUM_ENABLE 0x40
> +#define TPM_TIS_I2C_REG_DATA_CSUM_GET    0x44
> +#define TPM_TIS_I2C_REG_DID_VID          0x48
> +#define TPM_TIS_I2C_REG_RID              0x4c
> +#define TPM_TIS_I2C_REG_UNKNOWN          0xff
> +
> +/* Operations */
> +#define OP_SEND   1
> +#define OP_RECV   2
> +
> +typedef struct TPMStateI2C {
> +    /*< private >*/
> +    I2CSlave parent_obj;
> +
> +    int      offset;     /* offset in to data[] */
> +    int      size;       /* Size of the current reg data */
> +    uint8_t  operation;  /* OP_SEND & OP_RECV */
> +    uint8_t  data[4096]; /* Data */
> +
> +    /*< public >*/
> +    TPMState state; /* not a QOM object */
> +
> +} TPMStateI2C;
> +
> +DECLARE_INSTANCE_CHECKER(TPMStateI2C, TPM_TIS_I2C,
> +                         TYPE_TPM_TIS_I2C)
> +
> +static const VMStateDescription vmstate_tpm_tis_i2c = {
> +    .name = "tpm",
> +    .unmigratable = 1,

Is this just temporary? You offset + size + operation and data would have to be written out plus probably all the regular tis fields.

> +};
> +
> +/* Register map */
> +typedef struct reg_map {
> +    uint16_t  i2c_reg;    /* I2C register */
> +    uint16_t  tis_reg;    /* TIS register */
> +    uint32_t  data_size;  /* data size expected */
> +} i2c_reg_map;
> +
> +#define TPM_I2C_MAP_COUNT 11
> +
> +/*
> + * The register values in the common code is different than the latest
> + * register numbers as per the spec hence add the conversion map
> + */
> +i2c_reg_map tpm_tis_reg_map[] = {

static const i2c_reg_map tpm_tis_reg

> +    { TPM_TIS_I2C_REG_LOC_SEL,          TPM_TIS_REG_ACCESS,           1, },
> +    { TPM_TIS_I2C_REG_ACCESS,           TPM_TIS_REG_ACCESS,           1, },
> +    { TPM_TIS_I2C_REG_INT_ENABLE,       TPM_TIS_REG_INT_ENABLE,       4, },
> +    { TPM_TIS_I2C_REG_INT_CAPABILITY,   TPM_TIS_REG_INT_VECTOR,       4, },
> +    { TPM_TIS_I2C_REG_STS,              TPM_TIS_REG_STS,              4, },
> +    { TPM_TIS_I2C_REG_DATA_FIFO,        TPM_TIS_REG_DATA_FIFO,        0, },
> +    { TPM_TIS_I2C_REG_INTF_CAPABILITY,  TPM_TIS_REG_INTF_CAPABILITY,  4, },
> +    { TPM_TIS_I2C_REG_DATA_CSUM_ENABLE, TPM_TIS_REG_DATA_CSUM_ENABLE, 1, },
> +    { TPM_TIS_I2C_REG_DATA_CSUM_GET,    TPM_TIS_REG_DATA_CSUM_GET,    2, },
> +    { TPM_TIS_I2C_REG_DID_VID,          TPM_TIS_REG_DID_VID,          4, },
> +    { TPM_TIS_I2C_REG_RID,              TPM_TIS_REG_RID,              1, },
> +};
> +
> +static inline uint16_t tpm_tis_i2c_to_tis_reg(uint64_t i2c_reg, int *size)
> +{
> +    uint16_t tis_reg = TPM_TIS_I2C_REG_UNKNOWN;
> +    i2c_reg_map *reg_map;
> +    int i;
> +
> +    for (i = 0; i < TPM_I2C_MAP_COUNT; i++) {

..; i < ARRAY_SIZE(tpm_tis_reg_map); ...

Then you can drop TPM_I2c_MAP_COUNT.

> +        reg_map = &tpm_tis_reg_map[i];
> +        if (reg_map->i2c_reg == i2c_reg) {
> +            tis_reg = reg_map->tis_reg;
> +            *size = reg_map->data_size;
> +            break;
> +        }
> +    }
> +
> +    assert(tis_reg != TPM_TIS_I2C_REG_UNKNOWN);
> +    return tis_reg;
> +}
> +
> +/* Initialize the cached data */
> +static inline void tpm_tis_i2c_init_cache(TPMStateI2C *i2cst)
> +{
> +    /* Clear operation and offset */
> +    i2cst->operation = 0;
> +    i2cst->offset = 0;
> +    i2cst->size = 0;
> +
> +    return;
> +}
> +
> +/* Send data to TPM */
> +static inline void tpm_tis_i2c_tpm_send(TPMStateI2C *i2cst)
> +{
> +    if ((i2cst->operation == OP_SEND) && (i2cst->offset > 1)) {
> +        uint16_t tis_reg;
> +        uint32_t data;
> +        int      i;
You can move those 3 variable decls outside the if statement.

> +
> +        tis_reg = tpm_tis_i2c_to_tis_reg(i2cst->data[0], &i2cst->size);
> +
> +        /* Index 0 is always a register */
> +        for (i = 1; i < i2cst->offset; i++) {
> +            data = (i2cst->data[i] & 0xff);


' & 0xff' shouldn't be necessary since data is unsigned byte.

> +            tpm_tis_write_data(&i2cst->state, tis_reg, data, 1);
> +        }
> +
> +        tpm_tis_i2c_init_cache(i2cst);
> +> +    }> +    return;
> +}
> +
> +/* Callback from TPM to indicate that response is copied */
> +static void tpm_tis_i2c_request_completed(TPMIf *ti, int ret)
> +{
> +    TPMStateI2C *i2cst = TPM_TIS_I2C(ti);
> +    TPMState *s = &i2cst->state;
> +
> +    /* Inform the common code. */
> +    tpm_tis_request_completed(s, ret);
> +}
> +
> +static enum TPMVersion tpm_tis_i2c_get_tpm_version(TPMIf *ti)
> +{
> +    TPMStateI2C *i2cst = TPM_TIS_I2C(ti);
> +    TPMState *s = &i2cst->state;
> +
> +    return tpm_tis_get_tpm_version(s);
> +}
> +
> +static int tpm_tis_i2c_event(I2CSlave *i2c, enum i2c_event event)
> +{
> +    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
> +    int ret = 0;
> +
> +    switch (event) {
> +    case I2C_START_RECV:
> +        break;
> +    case I2C_START_SEND:
> +        tpm_tis_i2c_init_cache(i2cst);
> +        break;
> +    case I2C_FINISH:
> +        if (i2cst->operation == OP_SEND) {
> +            tpm_tis_i2c_tpm_send(i2cst);
> +        } else {
> +            tpm_tis_i2c_init_cache(i2cst);
> +        }
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +/* If data is for FIFO then it is received from tpm_tis_common buffer
> + * otherwise it will be handled using single call to common code and
> + * cached in the local buffer.
> + */
> +static uint8_t tpm_tis_i2c_recv(I2CSlave *i2c)
> +{
> +    int ret = 0;
> +    int i, j;
> +    uint32_t addr;
> +    uint32_t data_read;
> +    uint16_t i2c_reg;
> +    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
> +    TPMState *s = &i2cst->state;
> +
> +    if (i2cst->operation == OP_RECV) {
> +
> +        /* Special handling for FIFO */
> +        if (i2cst->data[0] == TPM_TIS_I2C_REG_DATA_FIFO) {
> +            i2c_reg = i2cst->data[0];
> +            addr = tpm_tis_i2c_to_tis_reg(i2c_reg, &i2cst->size);

why not just use TPM_TIS_I2C_REG_DATA_FIFO ? no need for i2c_reg here...


> +            data_read = tpm_tis_read_data(s, addr, 1);
> +            ret = (data_read & 0xff);
> +        } else
> +            ret = i2cst->data[i2cst->offset++];

Do you need to check for access beyond the buffer here?

> +
> +    } else if ((i2cst->operation == OP_SEND) && (i2cst->offset < 2)) {
> +        i2c_reg = i2cst->data[0];
> +
> +        i2cst->operation = OP_RECV;
> +        i2cst->offset = 0;
> +
> +        addr = tpm_tis_i2c_to_tis_reg(i2c_reg, &i2cst->size);
> +
> +        /* Special handling for FIFO register */
> +        if (i2c_reg == TPM_TIS_I2C_REG_DATA_FIFO) {
> +            data_read = tpm_tis_read_data(s, addr, 1);
> +            ret = (data_read & 0xff);
> +        } else {
> +            /*
> +             * Save the data in the data field. Save it in the little
> +             * endian format.
> +             */
> +            for (i = 0; i < i2cst->size;) {
> +                data_read = tpm_tis_read_data(s, addr, 4);
> +                for (j = 0; j < 4; j++) {
> +                    i2cst->data[i++] = (data_read & 0xff);


Where do you ensure that you never write beyond the size of the data buffer?

> +                    data_read >>= 8;
> +                }
> +            }
> +
> +            /* Return first byte with this call */
> +            ret = i2cst->data[i2cst->offset++];

Same comment as above regarding access beyond boundaries.

> +        }
> +    } else
> +        i2cst->operation = OP_RECV;

I am surprised that the checkpatch tool didn't complain about it but afaik this else branch should alsoe have { } -- one more case above like this.

> +
> +    return ret;
> +}
> +
> +/*
> + * Send function only remembers data in the buffer and then calls
> + * TPM TIS common code during FINISH event.
> + */
> +static int tpm_tis_i2c_send(I2CSlave *i2c, uint8_t data)
> +{
> +    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
> +
> +    /* Remember data locally */
> +    i2cst->operation = OP_SEND;
> +    i2cst->data[i2cst->offset++] = data;

Boundary check ?

> +
> +    return 0;
> +}
> +
> +static Property tpm_tis_i2c_properties[] = {
> +    DEFINE_PROP_UINT32("irq", TPMStateI2C, state.irq_num, TPM_TIS_IRQ),
> +    DEFINE_PROP_TPMBE("tpmdev", TPMStateI2C, state.be_driver),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void tpm_tis_i2c_realizefn(DeviceState *dev, Error **errp)
> +{
> +    TPMStateI2C *i2cst = TPM_TIS_I2C(dev);
> +    TPMState *s = &i2cst->state;
> +
> +    if (!tpm_find()) {
> +        error_setg(errp, "at most one TPM device is permitted");
> +        return;
> +    }
> +
> +    /* Get the backend pointer. It is not initialized propery during
> +     * device_class_set_props
> +     */
> +    s->be_driver = qemu_find_tpm_be("tpm0");
> +
> +    if (!s->be_driver) {
> +        error_setg(errp, "'tpmdev' property is required");
> +        return;
> +    }
> +    if (s->irq_num > 15) {
> +        error_setg(errp, "IRQ %d is outside valid range of 0 to 15",
> +                   s->irq_num);
> +        return;
> +    }
> +}
> +
> +static void tpm_tis_i2c_reset(DeviceState *dev)
> +{
> +    TPMStateI2C *i2cst = TPM_TIS_I2C(dev);
> +    TPMState *s = &i2cst->state;
> +
> +    tpm_tis_i2c_init_cache(i2cst);
> +
> +    return tpm_tis_reset(s);
> +}
> +
> +static void tpm_tis_i2c_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
> +    TPMIfClass *tc = TPM_IF_CLASS(klass);
> +
> +    dc->realize = tpm_tis_i2c_realizefn;
> +    dc->reset = tpm_tis_i2c_reset;
> +    dc->vmsd = &vmstate_tpm_tis_i2c;
> +    device_class_set_props(dc, tpm_tis_i2c_properties);
> +
> +    k->event = tpm_tis_i2c_event;
> +    k->recv = tpm_tis_i2c_recv;
> +    k->send = tpm_tis_i2c_send;
> +
> +    tc->model = TPM_MODEL_TPM_TIS;
> +    tc->request_completed = tpm_tis_i2c_request_completed;
> +    tc->get_version = tpm_tis_i2c_get_tpm_version;
> +}
> +
> +static const TypeInfo tpm_tis_i2c_info = {
> +    .name          = TYPE_TPM_TIS_I2C,
> +    .parent        = TYPE_I2C_SLAVE,
> +    .instance_size = sizeof(TPMStateI2C),
> +    .class_init    = tpm_tis_i2c_class_init,
> +        .interfaces = (InterfaceInfo[]) {
> +        { TYPE_TPM_IF },
> +        { }
> +    }
> +};
> +
> +static void tpm_tis_i2c_register_types(void)
> +{
> +    type_register_static(&tpm_tis_i2c_info);
> +}
> +
> +type_init(tpm_tis_i2c_register_types)
> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
> index fb40e30ff6..66e3b45f30 100644
> --- a/include/sysemu/tpm.h
> +++ b/include/sysemu/tpm.h
> @@ -48,6 +48,7 @@ struct TPMIfClass {
>   #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
>   #define TYPE_TPM_CRB                "tpm-crb"
>   #define TYPE_TPM_SPAPR              "tpm-spapr"
> +#define TYPE_TPM_TIS_I2C            "tpm-tis-i2c"
>   
>   #define TPM_IS_TIS_ISA(chr)                         \
>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
> @@ -57,6 +58,8 @@ struct TPMIfClass {
>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
>   #define TPM_IS_SPAPR(chr)                           \
>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
> +#define TPM_IS_TIS_I2C(chr)                      \
> +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_I2C)
>   
>   /* returns NULL unless there is exactly one TPM device */
>   static inline TPMIf *tpm_find(void)

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

[Ninad Palsule]

On 3/21/23 8:10 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. I2C model only supports
>> TPM2 protocol.
>>
>> This commit includes changes for the common code.
>> - Added I2C emulation model. Logic was added in the model to temporarily
>>    cache the data as I2C interface works per byte basis.
>> - New tpm type "tpm-tis-i2c" added for I2C support. User specify this
>>    string on command line.
>>
>> Testing:
>>    TPM I2C device modulte is tested using SWTPM (software based TPM
>>    package). The qemu used the rainier machine and it was connected to
>>    swtpm over the socket interface.
>>
>>    The command to start swtpm is as follows:
>>    $ swtpm socket --tpmstate dir=/tmp/mytpm1    \
>>                   --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock  \
>>                   --tpm2 --log level=100
>>
>>    The command to start qemu is as follows:
>>    $ qemu-system-arm -M rainier-bmc -nographic \
>>              -kernel ${IMAGEPATH}/fitImage-linux.bin \
>>              -dtb ${IMAGEPATH}/aspeed-bmc-ibm-rainier.dtb \
>>              -initrd 
>> ${IMAGEPATH}/obmc-phosphor-initramfs.rootfs.cpio.xz \
>>              -drive 
>> file=${IMAGEPATH}/obmc-phosphor-image.rootfs.wic.qcow2,if=sd,index=2 \
>>              -net nic -net 
>> user,hostfwd=:127.0.0.1:2222-:22,hostfwd=:127.0.0.1:2443-:443 \
>>              -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
>>              -tpmdev emulator,id=tpm0,chardev=chrtpm \
>>              -device 
>> tpm-tis-i2c,tpmdev=tpm0,bus=aspeed.i2c.bus.12,address=0x2e
>
>
> Please add this command line example also to the documentation.
Added rainier-bmc command in the documentation. The swtpm command is 
already in the document.
>
> When you run scripts/checkpatch.pl over this patch it reports the 
> following relevant complaints:
>
> WARNING: Block comments use a leading /* on a separate line
> #255: FILE: hw/tpm/tpm_tis_i2c.c:190:
> +/* If data is for FIFO then it is received from tpm_tis_common buffer
>
> WARNING: Block comments use a leading /* on a separate line
> #345: FILE: hw/tpm/tpm_tis_i2c.c:280:
> +    /* Get the backend pointer. It is not initialized propery during
>
>
Sorry about that. Fixed it.
>
>>
>>    Note: Currently you need to specify the I2C bus and device address on
>>          command line. In future we can add a device at board level.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   hw/tpm/meson.build   |   1 +
>>   hw/tpm/tpm_tis_i2c.c | 342 +++++++++++++++++++++++++++++++++++++++++++
>>   include/sysemu/tpm.h |   3 +
>>   3 files changed, 346 insertions(+)
>>   create mode 100644 hw/tpm/tpm_tis_i2c.c
>>
>> diff --git a/hw/tpm/meson.build b/hw/tpm/meson.build
>> index 7abc2d794a..76fe3cb098 100644
>> --- a/hw/tpm/meson.build
>> +++ b/hw/tpm/meson.build
>> @@ -1,6 +1,7 @@
>>   softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: 
>> files('tpm_tis_common.c'))
>>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_ISA', if_true: 
>> files('tpm_tis_isa.c'))
>>   softmmu_ss.add(when: 'CONFIG_TPM_TIS_SYSBUS', if_true: 
>> files('tpm_tis_sysbus.c'))
>> +softmmu_ss.add(when: 'CONFIG_TPM_TIS_I2C', if_true: 
>> files('tpm_tis_i2c.c'))
>>   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_crb.c'))
>>   softmmu_ss.add(when: 'CONFIG_TPM_TIS', if_true: files('tpm_ppi.c'))
>>   softmmu_ss.add(when: 'CONFIG_TPM_CRB', if_true: files('tpm_ppi.c'))
>> diff --git a/hw/tpm/tpm_tis_i2c.c b/hw/tpm/tpm_tis_i2c.c
>> new file mode 100644
>> index 0000000000..3c45af4140
>> --- /dev/null
>> +++ b/hw/tpm/tpm_tis_i2c.c
>> @@ -0,0 +1,342 @@
>> +/*
>> + * tpm_tis_i2c.c - QEMU's TPM TIS I2C Device
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 
>> or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + * Implementation of the TIS interface according to specs found at
>> + * http://www.trustedcomputinggroup.org. This implementation currently
>> + * supports version 1.3, 21 March 2013
>> + * In the developers menu choose the PC Client section then find the 
>> TIS
>> + * specification.
>> + *
>> + * TPM TIS for TPM 2 implementation following TCG PC Client Platform
>> + * TPM Profile (PTP) Specification, Familiy 2.0, Revision 00.43
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/i2c/i2c.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/acpi/tpm.h"
>> +#include "migration/vmstate.h"
>> +#include "tpm_prop.h"
>> +#include "tpm_tis.h"
>> +#include "qom/object.h"
>> +#include "block/aio.h"
>> +#include "qemu/main-loop.h"
>> +
>> +/* TPM TIS I2C registers */
>> +#define TPM_TIS_I2C_REG_LOC_SEL          0x00
>> +#define TPM_TIS_I2C_REG_ACCESS           0x04
>> +#define TPM_TIS_I2C_REG_INT_ENABLE       0x08
>> +#define TPM_TIS_I2C_REG_INT_CAPABILITY   0x14
>> +#define TPM_TIS_I2C_REG_STS              0x18
>> +#define TPM_TIS_I2C_REG_DATA_FIFO        0x24
>> +#define TPM_TIS_I2C_REG_INTF_CAPABILITY  0x30
>> +#define TPM_TIS_I2C_REG_DATA_CSUM_ENABLE 0x40
>> +#define TPM_TIS_I2C_REG_DATA_CSUM_GET    0x44
>> +#define TPM_TIS_I2C_REG_DID_VID          0x48
>> +#define TPM_TIS_I2C_REG_RID              0x4c
>> +#define TPM_TIS_I2C_REG_UNKNOWN          0xff
>> +
>> +/* Operations */
>> +#define OP_SEND   1
>> +#define OP_RECV   2
>> +
>> +typedef struct TPMStateI2C {
>> +    /*< private >*/
>> +    I2CSlave parent_obj;
>> +
>> +    int      offset;     /* offset in to data[] */
>> +    int      size;       /* Size of the current reg data */
>> +    uint8_t  operation;  /* OP_SEND & OP_RECV */
>> +    uint8_t  data[4096]; /* Data */
>> +
>> +    /*< public >*/
>> +    TPMState state; /* not a QOM object */
>> +
>> +} TPMStateI2C;
>> +
>> +DECLARE_INSTANCE_CHECKER(TPMStateI2C, TPM_TIS_I2C,
>> +                         TYPE_TPM_TIS_I2C)
>> +
>> +static const VMStateDescription vmstate_tpm_tis_i2c = {
>> +    .name = "tpm",
>> +    .unmigratable = 1,
>
> Is this just temporary? You offset + size + operation and data would 
> have to be written out plus probably all the regular tis fields.
>
Yes, This is temporary. I removed it.
>> +};
>> +
>> +/* Register map */
>> +typedef struct reg_map {
>> +    uint16_t  i2c_reg;    /* I2C register */
>> +    uint16_t  tis_reg;    /* TIS register */
>> +    uint32_t  data_size;  /* data size expected */
>> +} i2c_reg_map;
>> +
>> +#define TPM_I2C_MAP_COUNT 11
>> +
>> +/*
>> + * The register values in the common code is different than the latest
>> + * register numbers as per the spec hence add the conversion map
>> + */
>> +i2c_reg_map tpm_tis_reg_map[] = {
>
> static const i2c_reg_map tpm_tis_reg
Fixed the code.
>
>> +    { TPM_TIS_I2C_REG_LOC_SEL, TPM_TIS_REG_ACCESS,           1, },
>> +    { TPM_TIS_I2C_REG_ACCESS, TPM_TIS_REG_ACCESS,           1, },
>> +    { TPM_TIS_I2C_REG_INT_ENABLE, TPM_TIS_REG_INT_ENABLE,       4, },
>> +    { TPM_TIS_I2C_REG_INT_CAPABILITY, TPM_TIS_REG_INT_VECTOR,       
>> 4, },
>> +    { TPM_TIS_I2C_REG_STS, TPM_TIS_REG_STS,              4, },
>> +    { TPM_TIS_I2C_REG_DATA_FIFO, TPM_TIS_REG_DATA_FIFO,        0, },
>> +    { TPM_TIS_I2C_REG_INTF_CAPABILITY, TPM_TIS_REG_INTF_CAPABILITY,  
>> 4, },
>> +    { TPM_TIS_I2C_REG_DATA_CSUM_ENABLE, 
>> TPM_TIS_REG_DATA_CSUM_ENABLE, 1, },
>> +    { TPM_TIS_I2C_REG_DATA_CSUM_GET, TPM_TIS_REG_DATA_CSUM_GET,    
>> 2, },
>> +    { TPM_TIS_I2C_REG_DID_VID, TPM_TIS_REG_DID_VID,          4, },
>> +    { TPM_TIS_I2C_REG_RID, TPM_TIS_REG_RID,              1, },
>> +};
>> +
>> +static inline uint16_t tpm_tis_i2c_to_tis_reg(uint64_t i2c_reg, int 
>> *size)
>> +{
>> +    uint16_t tis_reg = TPM_TIS_I2C_REG_UNKNOWN;
>> +    i2c_reg_map *reg_map;
>> +    int i;
>> +
>> +    for (i = 0; i < TPM_I2C_MAP_COUNT; i++) {
>
> ..; i < ARRAY_SIZE(tpm_tis_reg_map); ...
>
> Then you can drop TPM_I2c_MAP_COUNT.
Fixed the code and removed TPM_I2C_MAP_COUNT.
>
>> +        reg_map = &tpm_tis_reg_map[i];
>> +        if (reg_map->i2c_reg == i2c_reg) {
>> +            tis_reg = reg_map->tis_reg;
>> +            *size = reg_map->data_size;
>> +            break;
>> +        }
>> +    }
>> +
>> +    assert(tis_reg != TPM_TIS_I2C_REG_UNKNOWN);
>> +    return tis_reg;
>> +}
>> +
>> +/* Initialize the cached data */
>> +static inline void tpm_tis_i2c_init_cache(TPMStateI2C *i2cst)
>> +{
>> +    /* Clear operation and offset */
>> +    i2cst->operation = 0;
>> +    i2cst->offset = 0;
>> +    i2cst->size = 0;
>> +
>> +    return;
>> +}
>> +
>> +/* Send data to TPM */
>> +static inline void tpm_tis_i2c_tpm_send(TPMStateI2C *i2cst)
>> +{
>> +    if ((i2cst->operation == OP_SEND) && (i2cst->offset > 1)) {
>> +        uint16_t tis_reg;
>> +        uint32_t data;
>> +        int      i;
> You can move those 3 variable decls outside the if statement.
Done.
>
>> +
>> +        tis_reg = tpm_tis_i2c_to_tis_reg(i2cst->data[0], &i2cst->size);
>> +
>> +        /* Index 0 is always a register */
>> +        for (i = 1; i < i2cst->offset; i++) {
>> +            data = (i2cst->data[i] & 0xff);
>
>
> ' & 0xff' shouldn't be necessary since data is unsigned byte.
Removed.
>
>> + tpm_tis_write_data(&i2cst->state, tis_reg, data, 1);
>> +        }
>> +
>> +        tpm_tis_i2c_init_cache(i2cst);
>> +> +    }> +    return;
>> +}
>> +
>> +/* Callback from TPM to indicate that response is copied */
>> +static void tpm_tis_i2c_request_completed(TPMIf *ti, int ret)
>> +{
>> +    TPMStateI2C *i2cst = TPM_TIS_I2C(ti);
>> +    TPMState *s = &i2cst->state;
>> +
>> +    /* Inform the common code. */
>> +    tpm_tis_request_completed(s, ret);
>> +}
>> +
>> +static enum TPMVersion tpm_tis_i2c_get_tpm_version(TPMIf *ti)
>> +{
>> +    TPMStateI2C *i2cst = TPM_TIS_I2C(ti);
>> +    TPMState *s = &i2cst->state;
>> +
>> +    return tpm_tis_get_tpm_version(s);
>> +}
>> +
>> +static int tpm_tis_i2c_event(I2CSlave *i2c, enum i2c_event event)
>> +{
>> +    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
>> +    int ret = 0;
>> +
>> +    switch (event) {
>> +    case I2C_START_RECV:
>> +        break;
>> +    case I2C_START_SEND:
>> +        tpm_tis_i2c_init_cache(i2cst);
>> +        break;
>> +    case I2C_FINISH:
>> +        if (i2cst->operation == OP_SEND) {
>> +            tpm_tis_i2c_tpm_send(i2cst);
>> +        } else {
>> +            tpm_tis_i2c_init_cache(i2cst);
>> +        }
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +/* If data is for FIFO then it is received from tpm_tis_common buffer
>> + * otherwise it will be handled using single call to common code and
>> + * cached in the local buffer.
>> + */
>> +static uint8_t tpm_tis_i2c_recv(I2CSlave *i2c)
>> +{
>> +    int ret = 0;
>> +    int i, j;
>> +    uint32_t addr;
>> +    uint32_t data_read;
>> +    uint16_t i2c_reg;
>> +    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
>> +    TPMState *s = &i2cst->state;
>> +
>> +    if (i2cst->operation == OP_RECV) {
>> +
>> +        /* Special handling for FIFO */
>> +        if (i2cst->data[0] == TPM_TIS_I2C_REG_DATA_FIFO) {
>> +            i2c_reg = i2cst->data[0];
>> +            addr = tpm_tis_i2c_to_tis_reg(i2c_reg, &i2cst->size);
>
> why not just use TPM_TIS_I2C_REG_DATA_FIFO ? no need for i2c_reg here...
>
Yes, Removed i2c_reg.
>
>> +            data_read = tpm_tis_read_data(s, addr, 1);
>> +            ret = (data_read & 0xff);
>> +        } else
>> +            ret = i2cst->data[i2cst->offset++];
>
> Do you need to check for access beyond the buffer here?
Added boundary check.
>
>> +
>> +    } else if ((i2cst->operation == OP_SEND) && (i2cst->offset < 2)) {
>> +        i2c_reg = i2cst->data[0];
>> +
>> +        i2cst->operation = OP_RECV;
>> +        i2cst->offset = 0;
>> +
>> +        addr = tpm_tis_i2c_to_tis_reg(i2c_reg, &i2cst->size);
>> +
>> +        /* Special handling for FIFO register */
>> +        if (i2c_reg == TPM_TIS_I2C_REG_DATA_FIFO) {
>> +            data_read = tpm_tis_read_data(s, addr, 1);
>> +            ret = (data_read & 0xff);
>> +        } else {
>> +            /*
>> +             * Save the data in the data field. Save it in the little
>> +             * endian format.
>> +             */
>> +            for (i = 0; i < i2cst->size;) {
>> +                data_read = tpm_tis_read_data(s, addr, 4);
>> +                for (j = 0; j < 4; j++) {
>> +                    i2cst->data[i++] = (data_read & 0xff);
>
>
> Where do you ensure that you never write beyond the size of the data 
> buffer?
Here boundary check is not required as data[] buffer size is 4096 and 
i2cst->size will not be more than 4 bytes. We go to this code only first 
time.
>
>> +                    data_read >>= 8;
>> +                }
>> +            }
>> +
>> +            /* Return first byte with this call */
>> +            ret = i2cst->data[i2cst->offset++];
>
> Same comment as above regarding access beyond boundaries.
The boundary check is not required as i2cst->offset is guaranteed to be 
0 as just initialized and data buffer is 4096. We go to this code only 
first recv call.
>
>> +        }
>> +    } else
>> +        i2cst->operation = OP_RECV;
>
> I am surprised that the checkpatch tool didn't complain about it but 
> afaik this else branch should alsoe have { } -- one more case above 
> like this.

OK, I fixed all places.

>
>> +
>> +    return ret;
>> +}
>> +
>> +/*
>> + * Send function only remembers data in the buffer and then calls
>> + * TPM TIS common code during FINISH event.
>> + */
>> +static int tpm_tis_i2c_send(I2CSlave *i2c, uint8_t data)
>> +{
>> +    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
>> +
>> +    /* Remember data locally */
>> +    i2cst->operation = OP_SEND;
>> +    i2cst->data[i2cst->offset++] = data;
>
> Boundary check ?

Added boundary check.

>
>> +
>> +    return 0;
>> +}
>> +
>> +static Property tpm_tis_i2c_properties[] = {
>> +    DEFINE_PROP_UINT32("irq", TPMStateI2C, state.irq_num, TPM_TIS_IRQ),
>> +    DEFINE_PROP_TPMBE("tpmdev", TPMStateI2C, state.be_driver),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void tpm_tis_i2c_realizefn(DeviceState *dev, Error **errp)
>> +{
>> +    TPMStateI2C *i2cst = TPM_TIS_I2C(dev);
>> +    TPMState *s = &i2cst->state;
>> +
>> +    if (!tpm_find()) {
>> +        error_setg(errp, "at most one TPM device is permitted");
>> +        return;
>> +    }
>> +
>> +    /* Get the backend pointer. It is not initialized propery during
>> +     * device_class_set_props
>> +     */
>> +    s->be_driver = qemu_find_tpm_be("tpm0");
>> +
>> +    if (!s->be_driver) {
>> +        error_setg(errp, "'tpmdev' property is required");
>> +        return;
>> +    }
>> +    if (s->irq_num > 15) {
>> +        error_setg(errp, "IRQ %d is outside valid range of 0 to 15",
>> +                   s->irq_num);
>> +        return;
>> +    }
>> +}
>> +
>> +static void tpm_tis_i2c_reset(DeviceState *dev)
>> +{
>> +    TPMStateI2C *i2cst = TPM_TIS_I2C(dev);
>> +    TPMState *s = &i2cst->state;
>> +
>> +    tpm_tis_i2c_init_cache(i2cst);
>> +
>> +    return tpm_tis_reset(s);
>> +}
>> +
>> +static void tpm_tis_i2c_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
>> +    TPMIfClass *tc = TPM_IF_CLASS(klass);
>> +
>> +    dc->realize = tpm_tis_i2c_realizefn;
>> +    dc->reset = tpm_tis_i2c_reset;
>> +    dc->vmsd = &vmstate_tpm_tis_i2c;
>> +    device_class_set_props(dc, tpm_tis_i2c_properties);
>> +
>> +    k->event = tpm_tis_i2c_event;
>> +    k->recv = tpm_tis_i2c_recv;
>> +    k->send = tpm_tis_i2c_send;
>> +
>> +    tc->model = TPM_MODEL_TPM_TIS;
>> +    tc->request_completed = tpm_tis_i2c_request_completed;
>> +    tc->get_version = tpm_tis_i2c_get_tpm_version;
>> +}
>> +
>> +static const TypeInfo tpm_tis_i2c_info = {
>> +    .name          = TYPE_TPM_TIS_I2C,
>> +    .parent        = TYPE_I2C_SLAVE,
>> +    .instance_size = sizeof(TPMStateI2C),
>> +    .class_init    = tpm_tis_i2c_class_init,
>> +        .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_TPM_IF },
>> +        { }
>> +    }
>> +};
>> +
>> +static void tpm_tis_i2c_register_types(void)
>> +{
>> +    type_register_static(&tpm_tis_i2c_info);
>> +}
>> +
>> +type_init(tpm_tis_i2c_register_types)
>> diff --git a/include/sysemu/tpm.h b/include/sysemu/tpm.h
>> index fb40e30ff6..66e3b45f30 100644
>> --- a/include/sysemu/tpm.h
>> +++ b/include/sysemu/tpm.h
>> @@ -48,6 +48,7 @@ struct TPMIfClass {
>>   #define TYPE_TPM_TIS_SYSBUS         "tpm-tis-device"
>>   #define TYPE_TPM_CRB                "tpm-crb"
>>   #define TYPE_TPM_SPAPR              "tpm-spapr"
>> +#define TYPE_TPM_TIS_I2C            "tpm-tis-i2c"
>>     #define TPM_IS_TIS_ISA(chr)                         \
>>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_ISA)
>> @@ -57,6 +58,8 @@ struct TPMIfClass {
>>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_CRB)
>>   #define TPM_IS_SPAPR(chr)                           \
>>       object_dynamic_cast(OBJECT(chr), TYPE_TPM_SPAPR)
>> +#define TPM_IS_TIS_I2C(chr)                      \
>> +    object_dynamic_cast(OBJECT(chr), TYPE_TPM_TIS_I2C)
>>     /* returns NULL unless there is exactly one TPM device */
>>   static inline TPMIf *tpm_find(void)


Thank you for the review!

Ninad Palsule

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

[Stefan Berger]

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. I2C model only supports
> TPM2 protocol.
> 

> +
> +/* Send data to TPM */
> +static inline void tpm_tis_i2c_tpm_send(TPMStateI2C *i2cst)
> +{
> +    if ((i2cst->operation == OP_SEND) && (i2cst->offset > 1)) {
> +        uint16_t tis_reg;
> +        uint32_t data;
> +        int      i;
> +
> +        tis_reg = tpm_tis_i2c_to_tis_reg(i2cst->data[0], &i2cst->size);
> +
> +        /* Index 0 is always a register */
> +        for (i = 1; i < i2cst->offset; i++) {
> +            data = (i2cst->data[i] & 0xff);
> +            tpm_tis_write_data(&i2cst->state, tis_reg, data, 1);
> +        }


I think there should be tpm_tis_set_data_buffer function that you can call rather than transferring the data byte-by-byte.

Thanks for the series!

   Stefan

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

[Ninad Palsule]

On 3/21/23 8:30 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. I2C model only supports
>> TPM2 protocol.
>>
>
>> +
>> +/* Send data to TPM */
>> +static inline void tpm_tis_i2c_tpm_send(TPMStateI2C *i2cst)
>> +{
>> +    if ((i2cst->operation == OP_SEND) && (i2cst->offset > 1)) {
>> +        uint16_t tis_reg;
>> +        uint32_t data;
>> +        int      i;
>> +
>> +        tis_reg = tpm_tis_i2c_to_tis_reg(i2cst->data[0], &i2cst->size);
>> +
>> +        /* Index 0 is always a register */
>> +        for (i = 1; i < i2cst->offset; i++) {
>> +            data = (i2cst->data[i] & 0xff);
>> +            tpm_tis_write_data(&i2cst->state, tis_reg, data, 1);
>> +        }
>
>
> I think there should be tpm_tis_set_data_buffer function that you can 
> call rather than transferring the data byte-by-byte.
>
> Thanks for the series!
>
>   Stefan

I thought about it but the FIFO case performs multiple operations hence 
I did not want to change it. Currently there is no function to set data 
buffer in the common code.

Thanks for the review!

Ninad Palsule

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

[Stefan Berger]

On 3/22/23 07:28, Ninad Palsule wrote:
> 
> On 3/21/23 8:30 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. I2C model only supports
>>> TPM2 protocol.
>>>
>>
>>> +
>>> +/* Send data to TPM */
>>> +static inline void tpm_tis_i2c_tpm_send(TPMStateI2C *i2cst)
>>> +{
>>> +    if ((i2cst->operation == OP_SEND) && (i2cst->offset > 1)) {
>>> +        uint16_t tis_reg;
>>> +        uint32_t data;
>>> +        int      i;
>>> +
>>> +        tis_reg = tpm_tis_i2c_to_tis_reg(i2cst->data[0], &i2cst->size);
>>> +
>>> +        /* Index 0 is always a register */
>>> +        for (i = 1; i < i2cst->offset; i++) {
>>> +            data = (i2cst->data[i] & 0xff);
>>> +            tpm_tis_write_data(&i2cst->state, tis_reg, data, 1);
>>> +        }
>>
>>
>> I think there should be tpm_tis_set_data_buffer function that you can call rather than transferring the data byte-by-byte.
>>
>> Thanks for the series!
>>
>>   Stefan
> 
> I thought about it but the FIFO case performs multiple operations hence I did not want to change it. Currently there is no function to set data buffer in the common code.

It may not be correct to transfer it in one go, either. I just printed the I2C specs and I am going to look at them now.
When one writes TPM command data to the TIS the STS register has its TPM_TIS_STS_VALID bit set and TPM_TIS_STS_EXPECT bit reset once the command is complete. This would imply that you should not have a holding area for the command bytes but pass them on to the TIS immediately to get the effect of the STS register...

    Stefan


> 
> Thanks for the review!
> 
> Ninad Palsule
> 

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

[Stefan Berger]

On 3/22/23 07:50, Stefan Berger wrote:
> 
> 
> On 3/22/23 07:28, Ninad Palsule wrote:
>>
>> On 3/21/23 8:30 PM, Stefan Berger wrote:
>>>

>>>
>>> I think there should be tpm_tis_set_data_buffer function that you can call rather than transferring the data byte-by-byte.
>>>
>>> Thanks for the series!
>>>
>>>   Stefan
>>
>> I thought about it but the FIFO case performs multiple operations hence I did not want to change it. Currently there is no function to set data buffer in the common code.
> 
> It may not be correct to transfer it in one go, either. I just printed the I2C specs and I am going to look at them now.
> When one writes TPM command data to the TIS the STS register has its TPM_TIS_STS_VALID bit set and TPM_TIS_STS_EXPECT bit reset once the command is complete. This would imply that you should not have a holding area for the command bytes but pass them on to the TIS immediately to get the effect of the STS register...

Regarding the registers defined for the I2C: You can pass the data onto the TIS but you should mask out input flags that are not defined for I2C and if the return value has flags not defined for I2C you should also mask those out as well. This applies to the TPM_INT_ENABLE & TPM_STS registers on read and write and to the TPM_INT_CAPABILITY on read. Also you should implement support for TPM_I2C_INTERACE_CAPABILITY on the I2C layer and return sensible values for the defined bits. The TPM_I2C_DEVICE_ADDRESS register should be handled probably assuming fixed address support only.

Ideally there would be a test case similar to this one here https://github.com/qemu/qemu/blob/master/tests/qtest/tpm-tis-util.c . However, I am not sure how easy it is to talk to I2C without a driver for it.

   Stefan

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

[Ninad Palsule]

On 3/22/23 8:04 AM, Stefan Berger wrote:
>
>
> On 3/22/23 07:50, Stefan Berger wrote:
>>
>>
>> On 3/22/23 07:28, Ninad Palsule wrote:
>>>
>>> On 3/21/23 8:30 PM, Stefan Berger wrote:
>>>>
>
>>>>
>>>> I think there should be tpm_tis_set_data_buffer function that you 
>>>> can call rather than transferring the data byte-by-byte.
>>>>
>>>> Thanks for the series!
>>>>
>>>>   Stefan
>>>
>>> I thought about it but the FIFO case performs multiple operations 
>>> hence I did not want to change it. Currently there is no function to 
>>> set data buffer in the common code.
>>
>> It may not be correct to transfer it in one go, either. I just 
>> printed the I2C specs and I am going to look at them now.
>> When one writes TPM command data to the TIS the STS register has its 
>> TPM_TIS_STS_VALID bit set and TPM_TIS_STS_EXPECT bit reset once the 
>> command is complete. This would imply that you should not have a 
>> holding area for the command bytes but pass them on to the TIS 
>> immediately to get the effect of the STS register...
>
> Regarding the registers defined for the I2C: You can pass the data 
> onto the TIS but you should mask out input flags that are not defined 
> for I2C and if the return value has flags not defined for I2C you 
> should also mask those out as well. This applies to the TPM_INT_ENABLE 
> & TPM_STS registers on read and write and to the TPM_INT_CAPABILITY on 
> read. Also you should implement support for 
> TPM_I2C_INTERACE_CAPABILITY on the I2C layer and return sensible 
> values for the defined bits. The TPM_I2C_DEVICE_ADDRESS register 
> should be handled probably assuming fixed address support only.
>
Good catch.

- Added capability conversion for TPM_I2C_INTERFACE_CAPABILITY.

- Added clearing of bits in TPM_STS register.

- Adde check to reject TPM_I2C_DEVICE_ADDRESS register.

- No changes are required for TPM_INT_ENABLE and TPM_INT_CAPABILITY as 
they have same bits between TPM TIS and TPM I2C.


> Ideally there would be a test case similar to this one here 
> https://github.com/qemu/qemu/blob/master/tests/qtest/tpm-tis-util.c . 
> However, I am not sure how easy it is to talk to I2C without a driver 
> for it.
Ok, Thanks.
>
>   Stefan


Thanks for the review!

Ninad Palsule

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

[Ninad Palsule]

On 3/22/23 6:50 AM, Stefan Berger wrote:
>
>
> On 3/22/23 07:28, Ninad Palsule wrote:
>>
>> On 3/21/23 8:30 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. I2C model only supports
>>>> TPM2 protocol.
>>>>
>>>
>>>> +
>>>> +/* Send data to TPM */
>>>> +static inline void tpm_tis_i2c_tpm_send(TPMStateI2C *i2cst)
>>>> +{
>>>> +    if ((i2cst->operation == OP_SEND) && (i2cst->offset > 1)) {
>>>> +        uint16_t tis_reg;
>>>> +        uint32_t data;
>>>> +        int      i;
>>>> +
>>>> +        tis_reg = tpm_tis_i2c_to_tis_reg(i2cst->data[0], 
>>>> &i2cst->size);
>>>> +
>>>> +        /* Index 0 is always a register */
>>>> +        for (i = 1; i < i2cst->offset; i++) {
>>>> +            data = (i2cst->data[i] & 0xff);
>>>> +            tpm_tis_write_data(&i2cst->state, tis_reg, data, 1);
>>>> +        }
>>>
>>>
>>> I think there should be tpm_tis_set_data_buffer function that you 
>>> can call rather than transferring the data byte-by-byte.
>>>
>>> Thanks for the series!
>>>
>>>   Stefan
>>
>> I thought about it but the FIFO case performs multiple operations 
>> hence I did not want to change it. Currently there is no function to 
>> set data buffer in the common code.
>
> It may not be correct to transfer it in one go, either. I just printed 
> the I2C specs and I am going to look at them now.
> When one writes TPM command data to the TIS the STS register has its 
> TPM_TIS_STS_VALID bit set and TPM_TIS_STS_EXPECT bit reset once the 
> command is complete. This would imply that you should not have a 
> holding area for the command bytes but pass them on to the TIS 
> immediately to get the effect of the STS register...
>
>    Stefan
>
Yes, I had issue related to STS status while reading but did not see any 
issue while writing but now I have changed it to _send too so there is 
no holding area for FIFO data in the I2C.
>
>>
>> Thanks for the review!
>>
>> Ninad Palsule
>>