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

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel