Re: [PATCH v5.1 01/12] mini-os/tpm{back, front}: Change shared page ABI

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 04/11/2013 10:14 AM, Ian Campbell wrote:
> On Fri, 2013-03-22 at 22:30 +0000, Daniel De Graaf wrote:
>> @@ -529,15 +526,27 @@ void free_tpmif(tpmif_t* tpmif)
>>   void tpmback_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
>>   {
>>      tpmif_t* tpmif = (tpmif_t*) data;
>> -   tpmif_tx_request_t* tx = &tpmif->tx->ring[0].req;
>> -   /* Throw away 0 size events, these can trigger from event channel unmasking */
>> -   if(tx->size == 0)
>> -      return;
>> -
>> -   TPMBACK_DEBUG("EVENT CHANNEL FIRE %u/%u\n", (unsigned int) tpmif->domid, tpmif->handle);
>> -   tpmif_req_ready(tpmif);
>> -   wake_up(&waitq);
>> +   vtpm_shared_page_t* pg = tpmif->page;
>>
>
> Do we not need a barrier somewhere around here to ensure that the far
> end's write to pg->state is visible to this cpu?

The frontend's write to pg->state is always done prior to the frontend
sending its event channel notification, so an explicit barrier is not
needed in this function. Since there is only one read and a clear
dependency on the one write, so I'm not sure where the barrier here
would need to go even if it was needed.

We might need a barrier in send_response between the memcpy and setting
the state to VTPM_STATE_FINISH.  It so happens that the existing code
includes a call to local_irq_save which includes barrier(), making the
code technically safe - but an explicit barrier would clarify this and
avoid potential bugs introduced by moving tpmif_req_finished() around.

> The writer does:
> 	write all fields apart from state
> 	barrier()
> 	write state.
>
> no need for a barrier at the end of that lot either?
>
>> +   switch (pg->state)
>> +   {
>> +   case VTPM_STATE_SUBMIT:
>> +      TPMBACK_DEBUG("EVENT CHANNEL FIRE %u/%u\n", (unsigned int) tpmif->domid, tpmif->handle);
>> +      tpmif_req_ready(tpmif);
>> +      wake_up(&waitq);
>> +      break;
>> +   case VTPM_STATE_CANCEL:
>> +      /* If we are busy with a request, do nothing */
>> +      if (tpmif->flags & TPMIF_REQ_READY)
>> +         return;
>> +      /* Acknowledge the cancellation if we are idle */
>> +      pg->state = VTPM_STATE_IDLE;
>> +      notify_remote_via_evtchn(tpmif->evtchn);
>> +      return;
>> +   default:
>> +      /* Spurious wakeup; do nothing */
>> +      return;
>> +   }
>>   }
>>
>


-- 
Daniel De Graaf
National Security Agency