Re: [Qemu-devel] [PATCH v3 06/13] tpm_tis: move r/w_offsets to TPMState

[Stefan Berger <stefanb@linux.vnet.ibm.com>]

On 12/21/2017 09:41 AM, Marc-André Lureau wrote:
> Hi
>
> On Fri, Nov 10, 2017 at 3:11 PM, Stefan Berger
> <stefanb@linux.vnet.ibm.com> wrote:
>> Now that we have a single buffer, we also only need a single set of
>> read/write offsets into that buffer. This works since only one
>> locality can be active.
>>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>>   hw/tpm/tpm_tis.c | 57 +++++++++++++++++++++++++++-----------------------------
>>   1 file changed, 27 insertions(+), 30 deletions(-)
>>
>> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
>> index 0b6dd7f..dfdddd3 100644
>> waddr addr,
>>               }
>>
>>               while ((s->loc[locty].sts & TPM_TIS_STS_EXPECT) && size > 0) {
>> -                if (s->loc[locty].w_offset < s->be_buffer_size) {
>> -                    s->buffer[s->loc[locty].w_offset++] =
>> +                if (s->w_offset < s->be_buffer_size) {
>> +                    s->buffer[s->w_offset++] =
>>                           (uint8_t)val;
>>                       val >>= 8;
>>                       size--;
>> @@ -931,13 +928,13 @@ static void tpm_tis_mmio_write(void *opaque, hwaddr addr,
>>               }
>>
>>               /* check for complete packet */
>> -            if (s->loc[locty].w_offset > 5 &&
>> +            if (s->w_offset > 5 &&
>>                   (s->loc[locty].sts & TPM_TIS_STS_EXPECT)) {
>>                   /* we have a packet length - see if we have all of it */
>>                   bool need_irq = !(s->loc[locty].sts & TPM_TIS_STS_VALID);
>>
>>                   len = tpm_cmd_get_size(&s->buffer);
>> -                if (len > s->loc[locty].w_offset) {
>> +                if (len > s->w_offset) {
>>                       tpm_tis_sts_set(&s->loc[locty],
>>                                       TPM_TIS_STS_EXPECT | TPM_TIS_STS_VALID);
>>                   } else {
>> @@ -1022,8 +1019,8 @@ static void tpm_tis_reset(DeviceState *dev)
>>           s->loc[c].ints = 0;
>>           s->loc[c].state = TPM_TIS_STATE_IDLE;
>>
>> -        s->loc[c].w_offset = 0;
>> -        s->loc[c].r_offset = 0;
>> +        s->w_offset = 0;
>> +        s->r_offset = 0;
>>       }
>>
>>       tpm_tis_do_startup_tpm(s, s->be_buffer_size);
>> --
>> 2.5.5
>>
>>
> Looks good, but I wonder why it's not part of "tpm_tis: move buffers
> from localities into common location"
>
> Not a big deal though, so
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

I thought it may be easier to review/follow if I do all these steps 
separately. I could squash it together.


    Stefan

>
>
>