From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v5 01/16] x86/hvm: make sure emulation is retried if domain is shutting down Date: Tue, 30 Jun 2015 12:14:22 -0400 Message-ID: <5592C05E.30907@Gmail.com> References: <1435669558-5421-1-git-send-email-paul.durrant@citrix.com> <1435669558-5421-2-git-send-email-paul.durrant@citrix.com> <55929D72.30808@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z9yB1-0004aO-HU for xen-devel@lists.xenproject.org; Tue, 30 Jun 2015 16:14:27 +0000 Received: by ykdv136 with SMTP id v136so13489781ykd.0 for ; Tue, 30 Jun 2015 09:14:25 -0700 (PDT) In-Reply-To: <55929D72.30808@citrix.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: Andrew Cooper , Paul Durrant , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 06/30/15 09:45, Andrew Cooper wrote: > On 30/06/15 14:05, Paul Durrant wrote: >> The addition of commit 2df1aa01 "x86/hvm: remove hvm_io_pending() check >> in hvmemul_do_io()" causes a problem in migration because I/O that was >> caught by the test of vcpu_start_shutdown_deferral() in >> hvm_send_assist_req() is now considered completed rather than requiring >> a retry. >> >> This patch fixes the problem by having hvm_send_assist_req() return >> X86EMUL_RETRY rather than X86EMUL_OKAY if the >> vcpu_start_shutdown_deferral() test fails and then making sure that >> the emulation state is reset if the domain is found to be shutting >> down. >> >> Reported-by: Don Slutz >> Signed-off-by: Paul Durrant >> Keir Fraser >> Jan Beulich >> Andrew Cooper > Reviewed-by: Andrew Cooper > >> --- >> xen/arch/x86/hvm/emulate.c | 2 +- >> xen/arch/x86/hvm/hvm.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c >> index fe5661d..8b60843 100644 >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -183,7 +183,7 @@ static int hvmemul_do_io( >> else >> { >> rc = hvm_send_assist_req(s, &p); >> - if ( rc != X86EMUL_RETRY ) >> + if ( rc != X86EMUL_RETRY || curr->domain->is_shutting_down ) Having spent more time thinking about this, a safer test is "!curr-> defer_shutdown" . The reason are as follows: 1) This uses what vcpu_start_shutdown_deferral() returns. But it is only valid if vcpu_start_shutdown_deferral() is called which is the case now. 2) The checking of "curr->domain->is_shutting_down" without also checking for "curr-> defer_shutdown" may open a timing window. 3) No use of "spin_lock(&d->shutdown_lock)". >> vio->io_state = HVMIO_none; >> else if ( data_is_addr || dir == IOREQ_WRITE ) >> rc = X86EMUL_OKAY; >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> index 535d622..f0cf064 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -2613,7 +2613,7 @@ int hvm_send_assist_req(struct hvm_ioreq_server *s, ioreq_t *proto_p) >> >> ASSERT(s); >> if ( unlikely(!vcpu_start_shutdown_deferral(curr)) ) >> - return X86EMUL_OKAY; >> + return X86EMUL_RETRY; I will also note that this change makes hvm_send_assist_req() have only 2 return codes: X86EMUL_RETRY X86EMUL_UNHANDLEABLE and so is not clear why its return should not be a bool. And there are now 2 reasons why X86EMUL_RETRY is returned, which require the caller to redo a test. Granted X86EMUL_OKAY is bad. But I wounder if it would be better to add an enum for hvm_send_assist_req() to return in stead of overloading the usage of X86EMUL_UNHANDLEABLE and X86EMUL_RETRY with yet more meanings. -Don Slutz >> >> list_for_each_entry ( sv, >> &s->ioreq_vcpu_list, > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel