Re: [PATCH v7 3/3] tpm: Add support for TPM device over I2C bus

[Ninad Palsule]

Hi Stefan,


On 3/27/23 8:40 AM, Stefan Berger wrote:
> On 3/26/23 18:44, 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.
>
> > --- /dev/null
> > +++ b/hw/tpm/tpm_tis_i2c.c
> > @@ -0,0 +1,540 @@
> > +/*
> > + * 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
> > + *
> > + * TPM I2C implementation follows TCG TPM I2c Interface specification,
> > + * Family 2.0, Level 00, Revision 1.00
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/acpi/tpm.h"
> > +#include "migration/vmstate.h"
> > +#include "tpm_prop.h"
> > +#include "qemu/log.h"
> > +#include "trace.h"
> > +#include "tpm_tis.h"
> > +
> > +/* TPM_STS mask for read bits 31:26 must be zero */
> > +#define TPM_I2C_STS_READ_MASK          0x03ffffff
> > +
> > +/* TPM_I2C_INT_ENABLE mask */
> > +#define TPM_I2C_INT_ENABLE_MASK   (TPM_TIS_INT_ENABLED | \
> > +                                   TPM_TIS_INT_DATA_AVAILABLE | \
> > +                                   TPM_TIS_INT_STS_VALID | \
> > +                                   TPM_TIS_INT_LOCALITY_CHANGED | \
> > +                                   TPM_TIS_INT_COMMAND_READY)
>
> Since we cannot test interrupts with Linux since the driver doesn't 
> support it,
> can you set this mask to 0 and move it into the public header file. 
> Please also
> apply the mask to vthe alue returned from reading to the 
> TPM_INT_CAPABILITY register.
OK, done.
> > +
> > +/* Operations */
> > +#define OP_SEND   1
> > +#define OP_RECV   2
> > +
> > +typedef struct TPMStateI2C {
> > +    /*< private >*/
> > +    I2CSlave    parent_obj;
> > +
> > +    uint8_t     offset;       /* offset into data[] */
> > +    uint8_t     operation;    /* OP_SEND & OP_RECV */
> > +    uint8_t     data[5];      /* Data */
> > +
> > +    /* i2c registers */
> > +    uint8_t     loc_sel;      /* Current locality */
> > +    uint8_t     csum_enable;  /* Is checksum enabled */
> > +
> > +    /* Derived from the above */
> > +    const char *reg_name;     /* Register name */
> > +    uint32_t    tis_addr;     /* Converted tis address including 
> > locty */
> > +
> > +    /*< public >*/
> > +    TPMState    state; /* not a QOM object */
> > +
> > +} TPMStateI2C;
> > +
> > +DECLARE_INSTANCE_CHECKER(TPMStateI2C, TPM_TIS_I2C,
> > +                         TYPE_TPM_TIS_I2C)
> > +
> > +/* Prototype */
> > +static inline void tpm_tis_i2c_to_tis_reg(TPMStateI2C *i2cst, 
> > uint8_t i2c_reg);
> > +
> > +/* Register map */
> > +typedef struct regMap {
> > +    uint8_t   i2c_reg;    /* I2C register */
> > +    uint16_t  tis_reg;    /* TIS register */
> > +    const char *reg_name; /* Register name */
> > +} I2CRegMap;
> > +
> > +/*
> > + * The register values in the common code is different than the latest
> > + * register numbers as per the spec hence add the conversion map
> > + */
> > +static const I2CRegMap tpm_tis_reg_map[] = {
> > +    /*
> > +     * These registers are sent to TIS layer. The register with UNKNOWN
> > +     * mapping are not sent to TIS layer and handled in I2c layer.
> > +     * NOTE: Adding frequently used registers at the start
> > +     */
> > +    { TPM_I2C_REG_DATA_FIFO, TPM_TIS_REG_DATA_FIFO,       
> > "FIFO",      },
> > +    { TPM_I2C_REG_STS, TPM_TIS_REG_STS,             "STS",       },
> > +    { TPM_I2C_REG_DATA_CSUM_GET, TPM_I2C_REG_UNKNOWN,         
> > "CSUM_GET",  },
> > +    { TPM_I2C_REG_LOC_SEL, TPM_I2C_REG_UNKNOWN,         "LOC_SEL",   },
> > +    { TPM_I2C_REG_ACCESS, TPM_TIS_REG_ACCESS,          "ACCESS",    },
> > +    { TPM_I2C_REG_INT_ENABLE,       TPM_TIS_REG_INT_ENABLE, 
> > "INTR_ENABLE",},
> > +    { TPM_I2C_REG_INT_CAPABILITY, TPM_TIS_REG_INT_ENABLE,      
> > "INTR_CAP",  },
>
>
> This mapping here is wrong. It should map to TPM_TIS_REG_INT_CAPABILITY.
OK, Now I handle this register in the I2C so changed to mapping to UNKNOWN
> > +    { TPM_I2C_REG_INTF_CAPABILITY, TPM_TIS_REG_INTF_CAPABILITY, 
> > "INTF_CAP",  },
> > +    { TPM_I2C_REG_DID_VID, TPM_TIS_REG_DID_VID,         "DID_VID",   },
> > +    { TPM_I2C_REG_RID, TPM_TIS_REG_RID,             "RID",       },
> > +    { TPM_I2C_REG_I2C_DEV_ADDRESS,  TPM_I2C_REG_UNKNOWN, 
> > "DEV_ADDRESS",},
> > +    { TPM_I2C_REG_DATA_CSUM_ENABLE, TPM_I2C_REG_UNKNOWN, 
> > "CSUM_ENABLE",},
> > +};
> > +
>
>
> > +/*
> > + * 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);
> > +
> > +    /* Reject non-supported registers. */
> > +    if (i2cst->offset == 0) {
> > +        /* Convert I2C register to TIS register */
> > +        tpm_tis_i2c_to_tis_reg(i2cst, data);
> > +        if (i2cst->tis_addr == 0xffffffff) {
> > +            return 0xffffffff;
> > +        }
> > +
> > +        trace_tpm_tis_i2c_send_reg(i2cst->reg_name, data);
> > +
> > +        /* We do not support device address change */
> > +        if (data == TPM_I2C_REG_I2C_DEV_ADDRESS) {
> > +            qemu_log_mask(LOG_UNIMP, "%s: Device address change "
> > +                          "is not supported.\n", __func__);
> > +            return 0xffffffff;
> > +        }
> > +    } else {
> > +        trace_tpm_tis_i2c_send(data);
> > +    }
> > +
> > +    if (i2cst->offset < sizeof(i2cst->data)) {
> > +        i2cst->operation = OP_SEND;
> > +
> > +        /* Remember data locally for non-FIFO registers */
> > +        if ((i2cst->offset == 0) ||
> > +            (i2cst->data[0] != TPM_I2C_REG_DATA_FIFO)) {
> > +            i2cst->data[i2cst->offset++] = data;
> > +        } else {
> > +            tpm_tis_write_data(&i2cst->state, i2cst->tis_addr, data, 
> > 1);
> > +        }
> > +
> > +        return 0;
> > +
> > +    }
> > +
> > +    /* Return non-zero to indicate NAK */
> > +    return 1;
> > +}
> > +
> > +static Property tpm_tis_i2c_properties[] = {
> > +    DEFINE_PROP_UINT32("irq", TPMStateI2C, state.irq_num, TPM_TIS_IRQ),
>
> You should remove this here. Maybe one day we will add it back when we 
> can test it.
Done
> > +    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;
> > +    }
>
> This block can go.
Done
> > +}
> > +
> > +static void tpm_tis_i2c_reset(DeviceState *dev)
> > +{
> > +    TPMStateI2C *i2cst = TPM_TIS_I2C(dev);
> > +    TPMState *s = &i2cst->state;
> > +
> > +    tpm_tis_i2c_clear_data(i2cst);
> > +
> > +    i2cst->csum_enable = 0;
> > +    i2cst->loc_sel = 0x00;
> > +
> > +    return tpm_tis_reset(s);
> > +}
> > +
> > +static void tpm_tis_i2c_initfn(Object *obj)
> > +{
> > +    TPMStateI2C *i2cst = TPM_TIS_I2C(obj);
> > +    TPMState *s = &i2cst->state;
> > +
> > +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
> > +}
>
> This function also has to go.

Done


Thanks for the review.

Ninad