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.