qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Corey Bryant
Subject: Re: [Qemu-devel] [PATCH V19 2/7] Add TPM (frontend) hardware interface (TPM TIS) to Qemu
Date: Thu, 08 Nov 2012 10:39:44 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121009 Thunderbird/16.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]