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

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

On 3/22/23 7:05 AM, 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.
>
>> @@ -447,6 +452,15 @@ 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);
>> +}
>> +
>>   /*
>>    * Write a value to a register of the TIS interface
>>    * See specs pages 33-63 for description of the registers
>> @@ -600,6 +614,15 @@ static void tpm_tis_mmio_write(void *opaque, 
>> hwaddr addr,
>>       case TPM_TIS_REG_INT_VECTOR:
>>           /* hard wired -- ignore */
>>           break;
>> +    case TPM_TIS_REG_DATA_CSUM_ENABLE:
>> +        /*
>> +         * Checksum implemented by common code so no need to set
>> +         * any flags.
>> +         */
>
> Can you intercept handling this register on the I2C layer and add a 
> byte for its value so that it can be set correctly? We do want to be 
> able to write bit 0 to it to enable it and allow reading of bit 0 to 
> see what the state is. I don't want this byte of state on the TIS 
> layer since this creates state incompatibilities.
>
> And for getting the checksum value it should be also handled on the 
> I2C layer and ask tpm_tis_common.c to run crc_ccitt(0, s->buffer, 
> s->rw_offset) via a function call.
>
>    Stefan


Sure, Fix it by handling it in I2C and removed newly defined registers 
from TIS layer.

Thanks for the review.

Ninad Palsule