Re: [Xen-devel] [PATCH v5 01/16] x86/hvm: make sure emulation is retried if domain is shutting down

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Don Slutz [mailto:don.slutz@xxxxxxxxx]
> Sent: 30 June 2015 17:14
> To: Andrew Cooper; Paul Durrant; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v5 01/16] x86/hvm: make sure emulation is
> retried if domain is shutting down
> 
> 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 <don.slutz@xxxxxxxxx>
> >> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> >> Keir Fraser <keir@xxxxxxx>
> >> Jan Beulich <jbeulich@xxxxxxxx>
> >> Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >
> >> ---
> >>   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)".

I don't think we need to lock. The is_shutting_down flag from this code's PoV 
will only ever go from 0 -> 1 so if shutdown deferral failed I think it is 
perfectly safe to check without taking the spinlock.

What we are essentially want to know in handle_pio() is whether the I/O has 
been issued or whether the whole thing needs to be retried. Another option to 
do this would be to change the io_state to something like IOREQ_INPROGRESS (or 
whatever it is called) when the I/O is actually issued and then test for that. 
Maybe hvm_send_assist_req() should just become a void function and the return 
code just set on the basis of the io_state after it has been called.

Anyway, all that is more code churn and the aim of this patch was a minimal 
diff to fix the problem... which I think it does.

  Paul

> 
> 
> >>                   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@xxxxxxxxxxxxx
> > http://lists.xen.org/xen-devel


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