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

[Stefan Berger]

On 3/27/23 12:16, 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.


> + * 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;
> +    uint32_t     data_read;
> +    TPMStateI2C *i2cst = TPM_TIS_I2C(i2c);
> +    TPMState    *s = &i2cst->state;
> +    uint16_t     i2c_reg = i2cst->data[0];
> +
> +    if (i2cst->operation == OP_RECV) {
> +
> +        /* Do not cache FIFO data. */
> +        if (i2cst->data[0] == TPM_I2C_REG_DATA_FIFO) {
> +            data_read = tpm_tis_read_data(s, i2cst->tis_addr, 1);
> +            ret = (data_read & 0xff);
> +        } else if (i2cst->offset < sizeof(i2cst->data)) {
> +            ret = i2cst->data[i2cst->offset++];
> +        }
> +
> +    } else if ((i2cst->operation == OP_SEND) && (i2cst->offset < 2)) {
> +        /* First receive call after send */
> +
> +        i2cst->operation = OP_RECV;
> +
> +        switch (i2c_reg) {
> +        case TPM_I2C_REG_LOC_SEL:
> +            /* Location selection register is managed by i2c */
> +            i2cst->data[1] = i2cst->loc_sel;
> +            break;
> +        case TPM_I2C_REG_DATA_FIFO:
> +            /* FIFO data is directly read from TPM TIS */
> +            data_read = tpm_tis_read_data(s, i2cst->tis_addr, 1);
> +            i2cst->data[1] = (data_read & 0xff);
> +            break;
> +        case TPM_I2C_REG_DATA_CSUM_ENABLE:
> +            i2cst->data[1] = i2cst->csum_enable;
> +            break;
> +        case TPM_I2C_REG_INT_CAPABILITY:
> +            /* Interrupt itpm_tis_read_data(s, i2cst->tis_addr, 1);s not 
> supported as there is not way to test it. */
We can test that this register returns the right values. What we cannot test is 
running this model with interrupts.


> +            i2cst->data[1] = TPM_I2C_INT_ENABLE_MASK;
> +            i2cst->data[2] = TPM_I2C_INT_ENABLE_MASK;
> +            i2cst->data[3] = TPM_I2C_INT_ENABLE_MASK;
> +            i2cst->data[4] = TPM_I2C_INT_ENABLE_MASK;

If you map the register in the table above you could do:

data_read = tpm_tis_read_data(s, i2cst->tis_addr, 1);
tpm_tis_i2c_set_data(data_read & TPM_I2C_INT_ENABLE_MASK);

Now that it's used in 3 locations the followig funtion would make sense:

static void tpm_tis_i2c_set_data(uint32_t data) {
    i2cst->data[1] = data;
    i2cst->data[2] = data >> 8;
    i2cst->data[3] = data >> 16;
    i2cst->data[4] = data >> 24;
}


Otherwise if you don't want to map it just call

tpm_tis_i2c_set_data(0);


> +            break;
> +        case TPM_I2C_REG_DATA_CSUM_GET:
> +            /*
> +             * Checksum registers are not supported by common code hence
> +             * call a common code to get the checksum.
> +             */
> +            data_read = tpm_tis_get_checksum(s);
> +
> +            /* Save the byte stream in data field */
> +            i2cst->data[1] = (data_read & 0xff);
> +            i2cst->data[2] = ((data_read >> 8) & 0xff);

tpm_tis_i2c_set_data(data_read);


> +            break;
> +        default:
> +            data_read = tpm_tis_read_data(s, i2cst->tis_addr, 4);
> +
> +            switch (i2c_reg) {
> +            case TPM_I2C_REG_INTF_CAPABILITY:
> +                /* Prepare the capabilities as per I2C interface */
> +                data_read = tpm_i2c_interface_capability(i2cst,
> +                                                         data_read);
> +                break;
> +            case TPM_I2C_REG_STS:
> +                /*
> +                 * As per specs, STS bit 31:26 are reserved and must
> +                 * be set to 0
> +                 */
> +                data_read &= TPM_I2C_STS_READ_MASK;
> +                break;
> +            }
> +
> +            /* Save byte stream in data[] */
> +            i2cst->data[1] = data_read;
> +            i2cst->data[2] = data_read >> 8;
> +            i2cst->data[3] = data_read >> 16;
> +            i2cst->data[4] = data_read >> 24;

tpm_tis_i2c_set_data(data_read);

> +            break;
> +        }
> +

The rest looks good.