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 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