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

[Ninad Palsule <ninad@linux.vnet.ibm.com>]

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
>>