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

[Stefan Berger]

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