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
