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 11:25 AM, Ian Campbell wrote:
> On Thu, 2013-04-11 at 15:38 +0100, Daniel De Graaf wrote:
> > 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.
> An event channel notification might happen to include barriers as part
> of its implementation but does the interface make any guarantees?
Since it looks like other mini-os drivers use wmb() before their event
channel notifications, I'll add that to be consistent.

> >   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.
> DOMU                            DOM_TPM
>         PRE: pg->status == IDLE
>         write data
>         barrier()
>         write pg->status = SUBMIT (A)
>         notify evtchn
>
>                                 receive evtchn
>                                 read pg->status (B)
>                                 do some stuff with the data
>                                 write pg->status (C)
>
> Don't we need some sort of memory barrier (as in rmb/wmb/mb not a
> compiler barrier()) between (A) and (B), to ensure that B sees the write
> at A and gets SUBMIT and not the previous value of IDLE?
>
> I wasn't thinking about barriers between the read at (B) and the write
> at (C), although now that you mention it a barrier might be needed
> *after* (C) so that the domU sees the vTPM as IDLE next time it come to
> use it...
>
> Ian.
--
Daniel De Graaf
National Security Agency

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