[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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_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
        write state.

no need for a barrier at the end of that lot either?

+   switch (pg->state)
+   {
+      TPMBACK_DEBUG("EVENT CHANNEL FIRE %u/%u\n", (unsigned int) tpmif->domid, 
+      tpmif_req_ready(tpmif);
+      wake_up(&waitq);
+      break;
+      /* 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

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.