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

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

On 11/08/2012 10:39 AM, Corey Bryant wrote:
> 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:
>>
>>>> +                /* 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.
>

Here's how the patch looks now:


+        if ((val & TPM_TIS_ACCESS_SEIZE)) {
+            /*
+             * allow seize if a locality is active and the requesting
+             * locality is higher than the one that's active
+             * OR
+             * allow seize for requesting locality if no locality is
+             * active
+             */
+            while ((TPM_TIS_IS_VALID_LOCTY(tis->active_locty) &&
+                    locty > tis->active_locty) ||
+                    !TPM_TIS_IS_VALID_LOCTY(tis->active_locty)) {
+                bool higher_seize = FALSE;
+
+                /* already a pending SEIZE ? */
+                if ((tis->loc[locty].access & TPM_TIS_ACCESS_SEIZE)) {
+                    break;
+                }
+
+                /* 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)) {
+                        higher_seize = TRUE;
+                        break;
+                    }
+                }
+
+                if (higher_seize) {
+                    break;
+                }
+
+                /* cancel any seize by a lower locality */
+                for (l = 0; l < locty - 1; l++) {
+                    tis->loc[l].access &= ~TPM_TIS_ACCESS_SEIZE;
+                }
[...]


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

Row 30 describes that abort for while the TPM is in 'Completion' state, 
meaning the TPM has delivered the response from the TPM to the TIS and 
now an application can pick up the response. The abort in this case is 
achieved through changing the state and resetting the (response) buffer 
pointers so that the application will not receive more response bytes.

Regards,
     Stefan