From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55710) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TR5zN-000209-9A for qemu-devel@nongnu.org; Wed, 24 Oct 2012 14:47:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TR5zL-0004UB-Qq for qemu-devel@nongnu.org; Wed, 24 Oct 2012 14:47:37 -0400 Received: from e36.co.us.ibm.com ([32.97.110.154]:40247) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TR5zL-0004Tl-I4 for qemu-devel@nongnu.org; Wed, 24 Oct 2012 14:47:35 -0400 Received: from /spool/local by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 24 Oct 2012 12:47:32 -0600 Received: from d03relay05.boulder.ibm.com (d03relay05.boulder.ibm.com [9.17.195.107]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 9994519D8061 for ; Wed, 24 Oct 2012 12:46:20 -0600 (MDT) Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay05.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q9OIk5uc256502 for ; Wed, 24 Oct 2012 12:46:09 -0600 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q9OIlkbc000490 for ; Wed, 24 Oct 2012 12:47:47 -0600 Message-ID: <5088376B.8020504@linux.vnet.ibm.com> Date: Wed, 24 Oct 2012 14:46:03 -0400 From: Stefan Berger MIME-Version: 1.0 References: <1338838668-7544-1-git-send-email-stefanb@linux.vnet.ibm.com> <1338838668-7544-3-git-send-email-stefanb@linux.vnet.ibm.com> <5064612E.5080700@linux.vnet.ibm.com> In-Reply-To: <5064612E.5080700@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V19 2/7] Add TPM (frontend) hardware interface (TPM TIS) to Qemu List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Bryant Cc: mst@redhat.com, qemu-devel@nongnu.org, anthony@codemonkey.ws, andreas.niederl@iaik.tugraz.at 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