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

[Corey Bryant]

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