From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH v5.1 01/12] mini-os/tpm{back, front}: Change shared page ABI Date: Thu, 11 Apr 2013 10:38:12 -0400 Message-ID: <5166CAD4.7090209@tycho.nsa.gov> References: <1363991313-25481-1-git-send-email-dgdegra@tycho.nsa.gov> <1363991420-25569-1-git-send-email-dgdegra@tycho.nsa.gov> <1365689654.8126.4.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1365689654.8126.4.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "Matthew.Fioravante@jhuapl.edu" , "xen-devel@lists.xen.org" , "JBeulich@suse.com" , "konrad.wilk@oracle.com" List-Id: xen-devel@lists.xenproject.org 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