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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 11/12/2012 08:16 AM, Stefan Berger wrote:
> 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;
> +                }
> [...]
>
>

Ok that looks good.

-- 
Regards,
Corey Bryant