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

[Ninad Palsule]

On 3/24/23 9:26 AM, Stefan Berger wrote:
> On 3/23/23 23:02, 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
> > ---
> >   hw/tpm/tpm_tis.h        |  3 +++
> >   hw/tpm/tpm_tis_common.c | 32 ++++++++++++++++++++++++++++++++
> >   include/hw/acpi/tpm.h   | 28 ++++++++++++++++++++++++++++
> >   3 files changed, 63 insertions(+)
> >
> > diff --git a/hw/tpm/tpm_tis.h b/hw/tpm/tpm_tis.h
> > index f6b5872ba6..784898ce34 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, uint8_t locty);
> >     #endif /* TPM_TPM_TIS_H */
> > diff --git a/hw/tpm/tpm_tis_common.c b/hw/tpm/tpm_tis_common.c
> > index 503be2a541..84f3e2259f 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,27 @@ 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, uint8_t locty)
> > +{
> > +    if (TPM_TIS_IS_VALID_LOCTY(locty) && (s->active_locty == locty)) {
> > +        return cpu_to_be16(crc_ccitt(0, s->buffer, s->rw_offset));
>
> Actually, this whole function should only be the following so you can 
> have the result in 'native' byte order:
>
> return crc_ccitt(0, s->buffer, s->rw_offset);
>
> Per I2C spec table 11 (section 6.6) the locality plays no role here. 
> My bad.
Ok, Removed the locality.

But still caller will have to make sure that data is read or written in 
the TIS before calling checksum function otherwise the buffer in TIS 
will be empty and result will be 0.


Thanks for the review.

Ninad

> > +    } else {
> > +        return 0;
> > +    }
> > +}
> > +
> >   /*
> >    * Write a value to a register of the TIS interface
> >    * See specs pages 33-63 for description of the registers
> > @@ -767,6 +790,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..82430605c7 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_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_I2C_DEV_ADDRESS  0x38
> > +#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
> > +
> > +/* 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
> > +
> >   void tpm_build_ppi_acpi(TPMIf *tpm, Aml *dev);
> >     #endif /* CONFIG_TPM */