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

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

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.

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


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

  Regards,
       Stefan