diff mbox series

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

Message ID 20230321053001.3886666-4-ninad@linux.ibm.com
State New
Headers show
Series Add support for TPM devices over I2C bus | expand

Commit Message

Ninad Palsule March 21, 2023, 5:30 a.m. UTC
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

Comments

Stefan Berger March 22, 2023, 1:10 a.m. UTC | #1
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)
Stefan Berger March 22, 2023, 1:30 a.m. UTC | #2
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
Ninad Palsule March 22, 2023, 11:26 a.m. UTC | #3
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
Ninad Palsule March 22, 2023, 11:28 a.m. UTC | #4
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
Stefan Berger March 22, 2023, 11:50 a.m. UTC | #5
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
>
Stefan Berger March 22, 2023, 1:04 p.m. UTC | #6
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
Ninad Palsule March 22, 2023, 5:01 p.m. UTC | #7
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
>>
Ninad Palsule March 23, 2023, 12:43 a.m. UTC | #8
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
diff mbox series

Patch

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)