Re: [Qemu-devel] [PATCH V19 2/7] Add TPM (frontend) hardware interface (TPM TIS) to Qemu

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

Thanks for your responses.  I have a few comments below.

On 10/24/2012 02:46 PM, Stefan Berger wrote:
> On 09/27/2012 10:22 AM, Corey Bryant wrote:
>>
>>
>> On 06/04/2012 03:37 PM, Stefan Berger wrote:
>>> +
>>> +/* whether the STS interrupt is supported */
>>> +/*#define RAISE_STS_IRQ */
>>
>> Why is this commented out?
>>
> Will activate it.
>
>>> +    if ((tis->loc[locty].inte & TPM_TIS_INT_ENABLED) &&
>>> +        (tis->loc[locty].inte & irqmask)) {
>>> +        dprintf("tpm_tis: Raising IRQ for flag %08x\n", irqmask);
>>> +        qemu_irq_raise(s->s.tis.irq);
>>> +        tis->loc[locty].ints |= irqmask;
>>
>> Should the ints irqmask be set before the interrupt is raised?
>>
>
> I don't think the order matters. If multiple threads were in here we'd
> need to lock the access to the variable altogether.
>
>
>>> +    TPMTISState *tis = &s->s.tis;
>>> +    int change = (s->s.tis.active_locty != new_active_locty);
>>> +
>>> +    if (change && TPM_TIS_IS_VALID_LOCTY(s->s.tis.active_locty)) {
>>> +        /* reset flags on the old active locality */
>>> +        tis->loc[s->s.tis.active_locty].access &=
>>> + ~(TPM_TIS_ACCESS_ACTIVE_LOCALITY|TPM_TIS_ACCESS_REQUEST_USE);
>>
>> Should TPM_TIS_ACCESS_REQUEST_USE be modified for the old locality
>> when we are in here for TPM_TIS_ACCESS_SEIZE?
>>
>> The TPM TIS document says the following (in the TPM_ACCESS_x table,
>> under the Seize bit description):
>>
>> "2. Setting this bit does not affect the state of the
>> TPM_ACCESS_x.requestUse bit for any locality except the one issuing
>> the Seize bit."
>>
>
> Good catch. In the case of the function being called as part of a seize,
> the REQUEST_USE flag will not be touched anymore.
>
>>> +    tis->loc[locty].sts = TPM_TIS_STS_VALID |
>>> TPM_TIS_STS_DATA_AVAILABLE;
>>> +    tis->loc[locty].status = TPM_TIS_STATUS_COMPLETION;
>>
>> Can tis->loc[locty].status be changed to tis->loc[locty].state?  This
>> is very confusing when named "status" because it is easy to confuse
>> with the TPM_INT_STATUS register, which in actuality it is unrelated.
>>
>
> Yes, it's now called TPMTISState and the previously existing TPMTISState
> has now been renamed to TPMTISEmuState.
>
>
>>> +        ret =
>>> tis->loc[locty].r_buffer.buffer[tis->loc[locty].r_offset++];
>>> +        if (tis->loc[locty].r_offset >= len) {
>>> +            /* got last byte */
>>> +            tis->loc[locty].sts = TPM_TIS_STS_VALID;
>>
>> Should dataAvail be turned off here?
>>
>
> The data available flag TPM_TIS_STS_DATA_AVAILABLE is part of the .sts
> field and is turned off due to the above value assignment.
>
>>> +        if (tis->active_locty == locty) {
>>> +            if ((tis->loc[locty].sts & TPM_TIS_STS_DATA_AVAILABLE)) {
>>> +                val =
>>> (tpm_tis_get_size_from_buffer(&tis->loc[locty].r_buffer)
>>> +                        - tis->loc[locty].r_offset) << 8 |
>>> tis->loc[locty].sts;
>>
>> Can you create/use a #define for burstCount instead of using a
>> hard-coded 8?
>>
> Ok.
>
>>> +    switch (off) {
>>> +    case TPM_TIS_REG_ACCESS:
>>> +
>>> +        if ((val & TPM_TIS_ACCESS_SEIZE)) {
>>> +            val &= ~(TPM_TIS_ACCESS_REQUEST_USE |
>>> +                     TPM_TIS_ACCESS_ACTIVE_LOCALITY);
>>> +        }
>>
>> Is there a reason why this code can't be merged with the "(val &
>> TPM_TIS_ACCESS_SEIZE)" check that is down below?
>>
>
> The above code means that in case the SEIZE flag is set, all other flags
> are ignored. It makes the subsequent tests for single bits a lot easier
> to handle. I prefer to do the masking of those bits at the beginning.
>
>
>>> +                /* check for ongoing seize by a higher locality */
>>> +                for (l = locty + 1; l < TPM_TIS_NUM_LOCALITIES; l++) {
>>> +                    if ((tis->loc[l].access & TPM_TIS_ACCESS_SEIZE)) {
>>> +                        break;
>>
>> Were you intending to break from the for loop or the while?
>>
>
> Right. I am setting a flag here now to then leave the while loop.
>

Are you setting the flag or testing it?  I'm not sure this code is 
serving any purpose the way it is, since it is testing the flag and then 
breaking from the for loop if it's on.  That's why I was wondering if 
you meant to break from the while loop instead.

>>> +
>>> +        tis->loc[locty].inte = (val & (TPM_TIS_INT_ENABLED | (0x3 <<
>>> 3) |
>>> + TPM_TIS_INTERRUPTS_SUPPORTED));
>>
>> Is 0x3 << 3 == typePolarity? Could a #define be introduced for this
>> instead of hard coding it?
>>
>
> Ok.
>
>>> +            case TPM_TIS_STATUS_EXECUTION:
>>> +            case TPM_TIS_STATUS_RECEPTION:
>>> +                /* abort currently running command */
>>> +                dprintf("tpm_tis: %s: Initiating abort.\n",
>>> +                        __func__);
>>> +                tpm_tis_prep_abort(s, locty, locty);
>>> +            break;
>>> +
>>> +            case TPM_TIS_STATUS_COMPLETION:
>>
>> Does this path need to abort if TPM_STS_x.dataAvail is on? This
>> comment is based on "Table 19: State Transition Table." from the TPM
>> TIS document.
>>
>
> If TPM_TIS_STATUS_COMPLETION is the current state, then independent of
> the TPM_TIS_STS_DATA_AVAILABLE flag the state transition is to idle
> (states 30 and 37 in the spec). Following state 0.B in the spec, we
> implement a TPM without idle state and so we transition to READY state
> immediately. The data available flag should be reset, though.
>

Ok.  But row 30 in the table also says it aborts the command in the 
"Action Taken" column.

>
>>> + tis->loc[locty].sts = TPM_TIS_STS_EXPECT |
>>> +                                          TPM_TIS_STS_VALID;
>>> +                } else {
>>> +                    /* packet complete */
>>> +                    tis->loc[locty].sts = TPM_TIS_STS_VALID;
>>
>> Should EXPECT be turned off here instead of where it is currently
>> turned off in tpm_tis_tpm_send?
>>
>
> The TPM_TIS_STS_EXPECT is turned off as part of the above value
> assignment but I removed the unnecessary masking of this bit in
> tpm_tis_tpm_send now and let the GO flag only submit the command if the
> EXPECT flag is not set anymore.
>
> I hope this addresses your concerns in this part.

Thanks.  Yes, you've addressed my concerns minus the few comments I have 
above.

-- 
Regards,
Corey Bryant