Re: [PATCH v4 2/3] tpm: Extend common APIs to support TPM TIS I2C

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

On 3/25/23 00:37, 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>
> ---
> V2:
> 
> Incorporated Stephen's comments.
> 
> - Removed checksum enable and checksum get registers.
> - Added checksum calculation function which can be called from
>    i2c layer.
> 
> ---
> V3:
> Incorporated review comments from Cedric and Stefan.
> 
> - Pass locality to the checksum calculation function and cleanup
> - Moved I2C related definations in the acpi/tpm.h
> 
> ---
> V4:
> 
> Incorporated review comments by Stefan
> 
> - Remove the check for locality while calculating checksum
> - Use bswap16 instead of cpu_ti_be16.
> - Rename TPM_I2C register by dropping _TIS_ from it.
> ---
>   hw/tpm/tpm_tis.h        |  3 +++
>   hw/tpm/tpm_tis_common.c | 28 ++++++++++++++++++++++++++++
>   include/hw/acpi/tpm.h   | 28 ++++++++++++++++++++++++++++
>   3 files changed, 59 insertions(+)
> 
> diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
> index f6b5872ba6..6f29a508dd 100644
> --- a/hw/tpm/tpm_tis.h
> +++ b/hw/tpm/tpm_tis.h
> @@ -86,5 +86,8 @@ 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);
> +uint16_t tpm_tis_get_checksum(TPMState *s);
>   
>   #endif /* TPM_TPM_TIS_H */
> diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
> index 503be2a541..c6d139943e 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"
> @@ -447,6 +449,23 @@ 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);
> +}
> +
> +/*
> + * Calculate current data buffer checksum
> + */
> +uint16_t tpm_tis_get_checksum(TPMState *s)
> +{
> +    return bswap16(crc_ccitt(0, s->buffer, s->rw_offset));
> +}
> +
>   /*
>    * Write a value to a register of the TIS interface
>    * See specs pages 33-63 for description of the registers
> @@ -767,6 +786,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..4f2424e2fe 100644
> --- a/include/hw/acpi/tpm.h
> +++ b/include/hw/acpi/tpm.h
> @@ -93,6 +93,7 @@
>   #define TPM_TIS_CAP_DATA_TRANSFER_64B    (3 << 9)
>   #define TPM_TIS_CAP_DATA_TRANSFER_LEGACY (0 << 9)
>   #define TPM_TIS_CAP_BURST_COUNT_DYNAMIC  (0 << 8)
> +#define TPM_TIS_CAP_BURST_COUNT_STATIC   (1 << 8)
>   #define TPM_TIS_CAP_INTERRUPT_LOW_LEVEL  (1 << 4) /* support is mandatory */
>   #define TPM_TIS_CAPABILITIES_SUPPORTED1_3 \
>       (TPM_TIS_CAP_INTERRUPT_LOW_LEVEL | \
> @@ -209,6 +210,33 @@ REG32(CRB_DATA_BUFFER, 0x80)
>   #define TPM_PPI_FUNC_ALLOWED_USR_NOT_REQ (4 << 0)
>   #define TPM_PPI_FUNC_MASK                (7 << 0)
>   
> +/* TPM TIS I2C registers */
> +#define TPM_I2C_REG_LOC_SEL              0x00
> +#define TPM_I2C_REG_ACCESS               0x04
> +#define TPM_I2C_REG_INT_ENABLE           0x08
> +#define TPM_I2C_REG_INT_CAPABILITY       0x14
> +#define TPM_I2C_REG_STS                  0x18
> +#define TPM_I2C_REG_DATA_FIFO            0x24
> +#define TPM_I2C_REG_INTF_CAPABILITY      0x30
> +#define TPM_I2C_REG_I2C_DEV_ADDRESS      0x38
> +#define TPM_I2C_REG_DATA_CSUM_ENABLE     0x40
> +#define TPM_I2C_REG_DATA_CSUM_GET        0x44
> +#define TPM_I2C_REG_DID_VID              0x48
> +#define TPM_I2C_REG_RID                  0x4c
> +#define TPM_I2C_REG_UNKNOWN              0xff
> +
> +/* I2C specific interface capabilities */
> +#define TPM_I2C_CAP_INTERFACE_TYPE     (0x2 << 0)       /* FIFO interface */
> +#define TPM_I2C_CAP_INTERFACE_VER      (0x0 << 4)       /* TCG I2C intf 1.0 */
> +#define TPM_I2C_CAP_TPM2_FAMILY        (0x1 << 7)       /* TPM 2.0 family. */
> +#define TPM_I2C_CAP_DEV_ADDR_CHANGE    (0x0 << 27)      /* No dev addr chng */
> +#define TPM_I2C_CAP_BURST_COUNT_STATIC (0x1 << 29)      /* Burst count static */
> +#define TPM_I2C_CAP_LOCALITY_CAP       (0x1 << 25)      /* 0-5 locality */
> +#define TPM_I2C_CAP_BUS_SPEED          (3   << 21)      /* std and fast mode */
> +
> +/* TPM_STS mask for read bits 31:26 must be zero */
> +#define TPM_I2C_STS_READ_MASK          0x03ffffff
> +

I think you should define a bit here for the TPM_DATA_CSUM_ENABLE register and used instead of 'true'.

    Stefan

>   void tpm_build_ppi_acpi(TPMIf *tpm, Aml *dev);
>   
>   #endif /* CONFIG_TPM */