Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Don Slutz]

commit 2df1aa01bef7366798248ac6d03cfb42048b003d
Author: Paul Durrant <paul.durrant@citrix.com>
Date:   Tue Jun 23 18:07:49 2015 +0200

     x86/hvm: remove hvm_io_pending() check in hvmemul_do_io()

...
-            rc = X86EMUL_RETRY;
-            if ( !hvm_send_assist_req(s, &p) )
+            rc = hvm_send_assist_req(s, &p);
+            if ( rc != X86EMUL_RETRY )
...
      if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
-        return 0; /* implicitly bins the i/o operation */
+        return X86EMUL_OKAY;

So now X86EMUL_OKAY is returned from hvmemul_do_io() during shutdown.

 From Jan Beulich about this:


  Re: [Xen-devel] [PATCH 3/5] hvmemul_do_io: If the send to the ioreq
  server failed do not retry.
  <https://www.mail-archive.com/search?l=xen-devel@lists.xen.org&q=subject:%22Re%5C%3A+%5C%5BXen%5C-devel%5C%5D+%5C%5BPATCH+3%5C%2F5%5C%5D+hvmemul_do_io%5C%3A+If+the+send+to+the+ioreq+server+failed+do+not+retry.%22&o=newest>


Jan Beulich 
<https://www.mail-archive.com/search?l=xen-devel@lists.xen.org&q=from:%22Jan+Beulich%22> 
Fri, 30 Jan 2015 02:24:55 -0800 
<https://www.mail-archive.com/search?l=xen-devel@lists.xen.org&q=date:20150130> 


>>> On 30.01.15 at 01:52, <dsl...@verizon.com> wrote:
> I.E. do just what no backing DM does.


_If_ this is correct, the if() modified here should be folded with the
one a few lines up. But looking at the description of the commit that
introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
instruction emulation...", almost immediately modified by f20f3c8ece
"x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
this is really what we want, or at the very least your change
description should explain what was wrong with the original commit.

Jan



going up the call stack:

in handle_pio when hvmemul_do_pio_buffer() returns X86EMUL_OKAY, it 
returns 1.

    svm_vmexit_handler 2578 if ( handle_pio(port, bytes, dir) )
or
    vmx_vmexit_handler 3178 if ( handle_pio(port, bytes, dir) )

both update the IP in this case which is wrong during shutdown.

So I think:

             rc = hvm_send_assist_req(s, &p);
             if ( rc != X86EMUL_RETRY )
                  vio->io_state = HVMIO_none;

needs to change to:

                 rc = hvm_send_assist_req(s, &p);
                 if ( rc != X86EMUL_RETRY )
                 {
                     vio->io_state = HVMIO_none;
                     if ( rc == X86EMUL_OKAY )
                         rc = X86EMUL_RETRY;
                 }


    -Don Slutz

Re: Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Paul Durrant]

> -----Original Message-----
> From: Don Slutz [mailto:don.slutz@gmail.com]
> Sent: 27 June 2015 22:02
> To: xen-devel@lists.xen.org; Paul Durrant; Jan Beulich
> Subject: Migration bug added by commit
> 2df1aa01bef7366798248ac6d03cfb42048b003d
> 
> commit 2df1aa01bef7366798248ac6d03cfb42048b003d
> Author: Paul Durrant <paul.durrant@citrix.com>
> Date:   Tue Jun 23 18:07:49 2015 +0200
> 
>      x86/hvm: remove hvm_io_pending() check in hvmemul_do_io()
> 
> ...
> -            rc = X86EMUL_RETRY;
> -            if ( !hvm_send_assist_req(s, &p) )
> +            rc = hvm_send_assist_req(s, &p);
> +            if ( rc != X86EMUL_RETRY )
> ...
>       if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
> -        return 0; /* implicitly bins the i/o operation */
> +        return X86EMUL_OKAY;
> 
> So now X86EMUL_OKAY is returned from hvmemul_do_io() during
> shutdown.
> 
>  From Jan Beulich about this:
> 
> 
>   Re: [Xen-devel] [PATCH 3/5] hvmemul_do_io: If the send to the ioreq
>   server failed do not retry.
>   <https://www.mail-archive.com/search?l=xen-
> devel@lists.xen.org&q=subject:%22Re%5C%3A+%5C%5BXen%5C-
> devel%5C%5D+%5C%5BPATCH+3%5C%2F5%5C%5D+hvmemul_do_io%5C%3
> A+If+the+send+to+the+ioreq+server+failed+do+not+retry.%22&o=newest
> >
> 
> 
> Jan Beulich
> <https://www.mail-archive.com/search?l=xen-
> devel@lists.xen.org&q=from:%22Jan+Beulich%22>
> Fri, 30 Jan 2015 02:24:55 -0800
> <https://www.mail-archive.com/search?l=xen-
> devel@lists.xen.org&q=date:20150130>
> 
> 
> >>> On 30.01.15 at 01:52, <dsl...@verizon.com> wrote:
> > I.E. do just what no backing DM does.
> 
> 
> _If_ this is correct, the if() modified here should be folded with the
> one a few lines up. But looking at the description of the commit that
> introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
> instruction emulation...", almost immediately modified by f20f3c8ece
> "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
> this is really what we want, or at the very least your change
> description should explain what was wrong with the original commit.
> 
> Jan
> 
> 
> 
> going up the call stack:
> 
> in handle_pio when hvmemul_do_pio_buffer() returns X86EMUL_OKAY, it
> returns 1.
> 
>     svm_vmexit_handler 2578 if ( handle_pio(port, bytes, dir) )
> or
>     vmx_vmexit_handler 3178 if ( handle_pio(port, bytes, dir) )
> 
> both update the IP in this case which is wrong during shutdown.
> 

If this is indeed the problem then it seems to me that the correct fix is to add a check in handle_pio() and return 0 if the cpu is shutting down.

  Paul

> So I think:
> 
>              rc = hvm_send_assist_req(s, &p);
>              if ( rc != X86EMUL_RETRY )
>                   vio->io_state = HVMIO_none;
> 
> needs to change to:
> 
>                  rc = hvm_send_assist_req(s, &p);
>                  if ( rc != X86EMUL_RETRY )
>                  {
>                      vio->io_state = HVMIO_none;
>                      if ( rc == X86EMUL_OKAY )
>                          rc = X86EMUL_RETRY;
>                  }
> 
> 
>     -Don Slutz

Re: Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Paul Durrant]

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Paul Durrant
> Sent: 29 June 2015 10:42
> To: Don Slutz; xen-devel@lists.xen.org; Jan Beulich
> Subject: Re: [Xen-devel] Migration bug added by commit
> 2df1aa01bef7366798248ac6d03cfb42048b003d
> 
> > -----Original Message-----
> > From: Don Slutz [mailto:don.slutz@gmail.com]
> > Sent: 27 June 2015 22:02
> > To: xen-devel@lists.xen.org; Paul Durrant; Jan Beulich
> > Subject: Migration bug added by commit
> > 2df1aa01bef7366798248ac6d03cfb42048b003d
> >
> > commit 2df1aa01bef7366798248ac6d03cfb42048b003d
> > Author: Paul Durrant <paul.durrant@citrix.com>
> > Date:   Tue Jun 23 18:07:49 2015 +0200
> >
> >      x86/hvm: remove hvm_io_pending() check in hvmemul_do_io()
> >
> > ...
> > -            rc = X86EMUL_RETRY;
> > -            if ( !hvm_send_assist_req(s, &p) )
> > +            rc = hvm_send_assist_req(s, &p);
> > +            if ( rc != X86EMUL_RETRY )
> > ...
> >       if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
> > -        return 0; /* implicitly bins the i/o operation */
> > +        return X86EMUL_OKAY;
> >
> > So now X86EMUL_OKAY is returned from hvmemul_do_io() during
> > shutdown.
> >
> >  From Jan Beulich about this:
> >
> >
> >   Re: [Xen-devel] [PATCH 3/5] hvmemul_do_io: If the send to the ioreq
> >   server failed do not retry.
> >   <https://www.mail-archive.com/search?l=xen-
> > devel@lists.xen.org&q=subject:%22Re%5C%3A+%5C%5BXen%5C-
> >
> devel%5C%5D+%5C%5BPATCH+3%5C%2F5%5C%5D+hvmemul_do_io%5C%3
> >
> A+If+the+send+to+the+ioreq+server+failed+do+not+retry.%22&o=newest
> > >
> >
> >
> > Jan Beulich
> > <https://www.mail-archive.com/search?l=xen-
> > devel@lists.xen.org&q=from:%22Jan+Beulich%22>
> > Fri, 30 Jan 2015 02:24:55 -0800
> > <https://www.mail-archive.com/search?l=xen-
> > devel@lists.xen.org&q=date:20150130>
> >
> >
> > >>> On 30.01.15 at 01:52, <dsl...@verizon.com> wrote:
> > > I.E. do just what no backing DM does.
> >
> >
> > _If_ this is correct, the if() modified here should be folded with the
> > one a few lines up. But looking at the description of the commit that
> > introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
> > instruction emulation...", almost immediately modified by f20f3c8ece
> > "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
> > this is really what we want, or at the very least your change
> > description should explain what was wrong with the original commit.
> >
> > Jan
> >
> >
> >
> > going up the call stack:
> >
> > in handle_pio when hvmemul_do_pio_buffer() returns X86EMUL_OKAY, it
> > returns 1.
> >
> >     svm_vmexit_handler 2578 if ( handle_pio(port, bytes, dir) )
> > or
> >     vmx_vmexit_handler 3178 if ( handle_pio(port, bytes, dir) )
> >
> > both update the IP in this case which is wrong during shutdown.
> >
> 
> If this is indeed the problem then it seems to me that the correct fix is to add
> a check in handle_pio() and return 0 if the cpu is shutting down.
> 

Actually, with some more thought, it sounds like we essentially want to unwind the I/O emulation if we're shutting down and I think this could be achieved by having hvm_send_assist_req() return X86EMUL_RETRY if it hits the shutdown deferral case, having hvm_emul_do_io() reset the emulation state back to 'none' if the domain is shutting down, and then having handle_pio() return 0 if sees X86EMUL_RETRY without any form of completion being requested.

  Paul

>   Paul
> 
> > So I think:
> >
> >              rc = hvm_send_assist_req(s, &p);
> >              if ( rc != X86EMUL_RETRY )
> >                   vio->io_state = HVMIO_none;
> >
> > needs to change to:
> >
> >                  rc = hvm_send_assist_req(s, &p);
> >                  if ( rc != X86EMUL_RETRY )
> >                  {
> >                      vio->io_state = HVMIO_none;
> >                      if ( rc == X86EMUL_OKAY )
> >                          rc = X86EMUL_RETRY;
> >                  }
> >
> >
> >     -Don Slutz
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant
> Sent: 29 June 2015 13:54
> To: Paul Durrant; Don Slutz; xen-devel@lists.xen.org; Jan Beulich
> Subject: RE: [Xen-devel] Migration bug added by commit
> 2df1aa01bef7366798248ac6d03cfb42048b003d
> 
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > bounces@lists.xen.org] On Behalf Of Paul Durrant
> > Sent: 29 June 2015 10:42
> > To: Don Slutz; xen-devel@lists.xen.org; Jan Beulich
> > Subject: Re: [Xen-devel] Migration bug added by commit
> > 2df1aa01bef7366798248ac6d03cfb42048b003d
> >
> > > -----Original Message-----
> > > From: Don Slutz [mailto:don.slutz@gmail.com]
> > > Sent: 27 June 2015 22:02
> > > To: xen-devel@lists.xen.org; Paul Durrant; Jan Beulich
> > > Subject: Migration bug added by commit
> > > 2df1aa01bef7366798248ac6d03cfb42048b003d
> > >
> > > commit 2df1aa01bef7366798248ac6d03cfb42048b003d
> > > Author: Paul Durrant <paul.durrant@citrix.com>
> > > Date:   Tue Jun 23 18:07:49 2015 +0200
> > >
> > >      x86/hvm: remove hvm_io_pending() check in hvmemul_do_io()
> > >
> > > ...
> > > -            rc = X86EMUL_RETRY;
> > > -            if ( !hvm_send_assist_req(s, &p) )
> > > +            rc = hvm_send_assist_req(s, &p);
> > > +            if ( rc != X86EMUL_RETRY )
> > > ...
> > >       if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
> > > -        return 0; /* implicitly bins the i/o operation */
> > > +        return X86EMUL_OKAY;
> > >
> > > So now X86EMUL_OKAY is returned from hvmemul_do_io() during
> > > shutdown.
> > >
> > >  From Jan Beulich about this:
> > >
> > >
> > >   Re: [Xen-devel] [PATCH 3/5] hvmemul_do_io: If the send to the ioreq
> > >   server failed do not retry.
> > >   <https://www.mail-archive.com/search?l=xen-
> > > devel@lists.xen.org&q=subject:%22Re%5C%3A+%5C%5BXen%5C-
> > >
> >
> devel%5C%5D+%5C%5BPATCH+3%5C%2F5%5C%5D+hvmemul_do_io%5C%3
> > >
> >
> A+If+the+send+to+the+ioreq+server+failed+do+not+retry.%22&o=newest
> > > >
> > >
> > >
> > > Jan Beulich
> > > <https://www.mail-archive.com/search?l=xen-
> > > devel@lists.xen.org&q=from:%22Jan+Beulich%22>
> > > Fri, 30 Jan 2015 02:24:55 -0800
> > > <https://www.mail-archive.com/search?l=xen-
> > > devel@lists.xen.org&q=date:20150130>
> > >
> > >
> > > >>> On 30.01.15 at 01:52, <dsl...@verizon.com> wrote:
> > > > I.E. do just what no backing DM does.
> > >
> > >
> > > _If_ this is correct, the if() modified here should be folded with the
> > > one a few lines up. But looking at the description of the commit that
> > > introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
> > > instruction emulation...", almost immediately modified by f20f3c8ece
> > > "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
> > > this is really what we want, or at the very least your change
> > > description should explain what was wrong with the original commit.
> > >
> > > Jan
> > >
> > >
> > >
> > > going up the call stack:
> > >
> > > in handle_pio when hvmemul_do_pio_buffer() returns X86EMUL_OKAY,
> it
> > > returns 1.
> > >
> > >     svm_vmexit_handler 2578 if ( handle_pio(port, bytes, dir) )
> > > or
> > >     vmx_vmexit_handler 3178 if ( handle_pio(port, bytes, dir) )
> > >
> > > both update the IP in this case which is wrong during shutdown.
> > >
> >
> > If this is indeed the problem then it seems to me that the correct fix is to
> add
> > a check in handle_pio() and return 0 if the cpu is shutting down.
> >
> 
> Actually, with some more thought, it sounds like we essentially want to
> unwind the I/O emulation if we're shutting down and I think this could be
> achieved by having hvm_send_assist_req() return X86EMUL_RETRY if it hits
> the shutdown deferral case, having hvm_emul_do_io() reset the emulation
> state back to 'none' if the domain is shutting down, and then having
> handle_pio() return 0 if sees X86EMUL_RETRY without any form of
> completion being requested.
> 

I think this patch should do it for now:

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index a4d7225..cc6130c 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 )
                 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 8226d8c..aba4ef6 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2547,7 +2547,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;

     list_for_each_entry ( sv,
                           &s->ioreq_vcpu_list,

>   Paul
> 
> >   Paul
> >
> > > So I think:
> > >
> > >              rc = hvm_send_assist_req(s, &p);
> > >              if ( rc != X86EMUL_RETRY )
> > >                   vio->io_state = HVMIO_none;
> > >
> > > needs to change to:
> > >
> > >                  rc = hvm_send_assist_req(s, &p);
> > >                  if ( rc != X86EMUL_RETRY )
> > >                  {
> > >                      vio->io_state = HVMIO_none;
> > >                      if ( rc == X86EMUL_OKAY )
> > >                          rc = X86EMUL_RETRY;
> > >                  }
> > >
> > >
> > >     -Don Slutz
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel

Re: Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Don Slutz]

On 06/29/15 10:03, Paul Durrant wrote:
>> -----Original Message-----
>> From: Paul Durrant
>> Sent: 29 June 2015 13:54
>> To: Paul Durrant; Don Slutz; xen-devel@lists.xen.org; Jan Beulich
>> Subject: RE: [Xen-devel] Migration bug added by commit
>> 2df1aa01bef7366798248ac6d03cfb42048b003d
>>
>>> -----Original Message-----
>>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>>> bounces@lists.xen.org] On Behalf Of Paul Durrant
>>> Sent: 29 June 2015 10:42
>>> To: Don Slutz; xen-devel@lists.xen.org; Jan Beulich
>>> Subject: Re: [Xen-devel] Migration bug added by commit
>>> 2df1aa01bef7366798248ac6d03cfb42048b003d
>>>
>>>> -----Original Message-----
>>>> From: Don Slutz [mailto:don.slutz@gmail.com]
>>>> Sent: 27 June 2015 22:02
>>>> To: xen-devel@lists.xen.org; Paul Durrant; Jan Beulich
>>>> Subject: Migration bug added by commit
>>>> 2df1aa01bef7366798248ac6d03cfb42048b003d
>>>>
>>>> commit 2df1aa01bef7366798248ac6d03cfb42048b003d
>>>> Author: Paul Durrant <paul.durrant@citrix.com>
>>>> Date:   Tue Jun 23 18:07:49 2015 +0200
>>>>
>>>>       x86/hvm: remove hvm_io_pending() check in hvmemul_do_io()
>>>>
>>>> ...
>>>> -            rc = X86EMUL_RETRY;
>>>> -            if ( !hvm_send_assist_req(s, &p) )
>>>> +            rc = hvm_send_assist_req(s, &p);
>>>> +            if ( rc != X86EMUL_RETRY )
>>>> ...
>>>>        if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
>>>> -        return 0; /* implicitly bins the i/o operation */
>>>> +        return X86EMUL_OKAY;
>>>>
>>>> So now X86EMUL_OKAY is returned from hvmemul_do_io() during
>>>> shutdown.
>>>>
>>>>   From Jan Beulich about this:
>>>>
>>>>
>>>>    Re: [Xen-devel] [PATCH 3/5] hvmemul_do_io: If the send to the ioreq
>>>>    server failed do not retry.
>>>>    <https://www.mail-archive.com/search?l=xen-
>>>> devel@lists.xen.org&q=subject:%22Re%5C%3A+%5C%5BXen%5C-
>>>>
>> devel%5C%5D+%5C%5BPATCH+3%5C%2F5%5C%5D+hvmemul_do_io%5C%3
>> A+If+the+send+to+the+ioreq+server+failed+do+not+retry.%22&o=newest
>>>>
>>>> Jan Beulich
>>>> <https://www.mail-archive.com/search?l=xen-
>>>> devel@lists.xen.org&q=from:%22Jan+Beulich%22>
>>>> Fri, 30 Jan 2015 02:24:55 -0800
>>>> <https://www.mail-archive.com/search?l=xen-
>>>> devel@lists.xen.org&q=date:20150130>
>>>>
>>>>
>>>>>>> On 30.01.15 at 01:52, <dsl...@verizon.com> wrote:
>>>>> I.E. do just what no backing DM does.
>>>>
>>>> _If_ this is correct, the if() modified here should be folded with the
>>>> one a few lines up. But looking at the description of the commit that
>>>> introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
>>>> instruction emulation...", almost immediately modified by f20f3c8ece
>>>> "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
>>>> this is really what we want, or at the very least your change
>>>> description should explain what was wrong with the original commit.
>>>>
>>>> Jan
>>>>
>>>>
>>>>
>>>> going up the call stack:
>>>>
>>>> in handle_pio when hvmemul_do_pio_buffer() returns X86EMUL_OKAY,
>> it
>>>> returns 1.
>>>>
>>>>      svm_vmexit_handler 2578 if ( handle_pio(port, bytes, dir) )
>>>> or
>>>>      vmx_vmexit_handler 3178 if ( handle_pio(port, bytes, dir) )
>>>>
>>>> both update the IP in this case which is wrong during shutdown.
>>>>
>>> If this is indeed the problem then it seems to me that the correct fix is to
>> add
>>> a check in handle_pio() and return 0 if the cpu is shutting down.
>>>
>> Actually, with some more thought, it sounds like we essentially want to
>> unwind the I/O emulation if we're shutting down and I think this could be
>> achieved by having hvm_send_assist_req() return X86EMUL_RETRY if it hits
>> the shutdown deferral case, having hvm_emul_do_io() reset the emulation
>> state back to 'none' if the domain is shutting down, and then having
>> handle_pio() return 0 if sees X86EMUL_RETRY without any form of
>> completion being requested.
>>
> I think this patch should do it for now:
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index a4d7225..cc6130c 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 )

I do not know enough about "is_shutting_down" to agree.  What is clear 
is that
this test is not the same as "!vcpu_start_shutdown_deferral(curr)".

     -Don Slutz

>                   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 8226d8c..aba4ef6 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2547,7 +2547,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;
>
>       list_for_each_entry ( sv,
>                             &s->ioreq_vcpu_list,
>
>>    Paul
>>
>>>    Paul
>>>
>>>> So I think:
>>>>
>>>>               rc = hvm_send_assist_req(s, &p);
>>>>               if ( rc != X86EMUL_RETRY )
>>>>                    vio->io_state = HVMIO_none;
>>>>
>>>> needs to change to:
>>>>
>>>>                   rc = hvm_send_assist_req(s, &p);
>>>>                   if ( rc != X86EMUL_RETRY )
>>>>                   {
>>>>                       vio->io_state = HVMIO_none;
>>>>                       if ( rc == X86EMUL_OKAY )
>>>>                           rc = X86EMUL_RETRY;
>>>>                   }
>>>>
>>>>
>>>>      -Don Slutz
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel

Re: Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Paul Durrant]

> -----Original Message-----
> From: Don Slutz [mailto:don.slutz@gmail.com]
> Sent: 29 June 2015 16:14
> To: Paul Durrant; xen-devel@lists.xen.org; Jan Beulich
> Subject: Re: [Xen-devel] Migration bug added by commit
> 2df1aa01bef7366798248ac6d03cfb42048b003d
> 
> On 06/29/15 10:03, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Paul Durrant
> >> Sent: 29 June 2015 13:54
> >> To: Paul Durrant; Don Slutz; xen-devel@lists.xen.org; Jan Beulich
> >> Subject: RE: [Xen-devel] Migration bug added by commit
> >> 2df1aa01bef7366798248ac6d03cfb42048b003d
> >>
> >>> -----Original Message-----
> >>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> >>> bounces@lists.xen.org] On Behalf Of Paul Durrant
> >>> Sent: 29 June 2015 10:42
> >>> To: Don Slutz; xen-devel@lists.xen.org; Jan Beulich
> >>> Subject: Re: [Xen-devel] Migration bug added by commit
> >>> 2df1aa01bef7366798248ac6d03cfb42048b003d
> >>>
> >>>> -----Original Message-----
> >>>> From: Don Slutz [mailto:don.slutz@gmail.com]
> >>>> Sent: 27 June 2015 22:02
> >>>> To: xen-devel@lists.xen.org; Paul Durrant; Jan Beulich
> >>>> Subject: Migration bug added by commit
> >>>> 2df1aa01bef7366798248ac6d03cfb42048b003d
> >>>>
> >>>> commit 2df1aa01bef7366798248ac6d03cfb42048b003d
> >>>> Author: Paul Durrant <paul.durrant@citrix.com>
> >>>> Date:   Tue Jun 23 18:07:49 2015 +0200
> >>>>
> >>>>       x86/hvm: remove hvm_io_pending() check in hvmemul_do_io()
> >>>>
> >>>> ...
> >>>> -            rc = X86EMUL_RETRY;
> >>>> -            if ( !hvm_send_assist_req(s, &p) )
> >>>> +            rc = hvm_send_assist_req(s, &p);
> >>>> +            if ( rc != X86EMUL_RETRY )
> >>>> ...
> >>>>        if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
> >>>> -        return 0; /* implicitly bins the i/o operation */
> >>>> +        return X86EMUL_OKAY;
> >>>>
> >>>> So now X86EMUL_OKAY is returned from hvmemul_do_io() during
> >>>> shutdown.
> >>>>
> >>>>   From Jan Beulich about this:
> >>>>
> >>>>
> >>>>    Re: [Xen-devel] [PATCH 3/5] hvmemul_do_io: If the send to the ioreq
> >>>>    server failed do not retry.
> >>>>    <https://www.mail-archive.com/search?l=xen-
> >>>> devel@lists.xen.org&q=subject:%22Re%5C%3A+%5C%5BXen%5C-
> >>>>
> >>
> devel%5C%5D+%5C%5BPATCH+3%5C%2F5%5C%5D+hvmemul_do_io%5C%3
> >>
> A+If+the+send+to+the+ioreq+server+failed+do+not+retry.%22&o=newest
> >>>>
> >>>> Jan Beulich
> >>>> <https://www.mail-archive.com/search?l=xen-
> >>>> devel@lists.xen.org&q=from:%22Jan+Beulich%22>
> >>>> Fri, 30 Jan 2015 02:24:55 -0800
> >>>> <https://www.mail-archive.com/search?l=xen-
> >>>> devel@lists.xen.org&q=date:20150130>
> >>>>
> >>>>
> >>>>>>> On 30.01.15 at 01:52, <dsl...@verizon.com> wrote:
> >>>>> I.E. do just what no backing DM does.
> >>>>
> >>>> _If_ this is correct, the if() modified here should be folded with the
> >>>> one a few lines up. But looking at the description of the commit that
> >>>> introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an
> >>>> instruction emulation...", almost immediately modified by f20f3c8ece
> >>>> "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt
> >>>> this is really what we want, or at the very least your change
> >>>> description should explain what was wrong with the original commit.
> >>>>
> >>>> Jan
> >>>>
> >>>>
> >>>>
> >>>> going up the call stack:
> >>>>
> >>>> in handle_pio when hvmemul_do_pio_buffer() returns
> X86EMUL_OKAY,
> >> it
> >>>> returns 1.
> >>>>
> >>>>      svm_vmexit_handler 2578 if ( handle_pio(port, bytes, dir) )
> >>>> or
> >>>>      vmx_vmexit_handler 3178 if ( handle_pio(port, bytes, dir) )
> >>>>
> >>>> both update the IP in this case which is wrong during shutdown.
> >>>>
> >>> If this is indeed the problem then it seems to me that the correct fix is to
> >> add
> >>> a check in handle_pio() and return 0 if the cpu is shutting down.
> >>>
> >> Actually, with some more thought, it sounds like we essentially want to
> >> unwind the I/O emulation if we're shutting down and I think this could be
> >> achieved by having hvm_send_assist_req() return X86EMUL_RETRY if it
> hits
> >> the shutdown deferral case, having hvm_emul_do_io() reset the
> emulation
> >> state back to 'none' if the domain is shutting down, and then having
> >> handle_pio() return 0 if sees X86EMUL_RETRY without any form of
> >> completion being requested.
> >>
> > I think this patch should do it for now:
> >
> > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> > index a4d7225..cc6130c 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 )
> 
> I do not know enough about "is_shutting_down" to agree.  What is clear
> is that
> this test is not the same as "!vcpu_start_shutdown_deferral(curr)".
> 

It pretty much is. If you look at the code for vcpu_start_shutdown_deferral() then you can see that it will only return 0 in the case that the domain's is_shutting_down flag is true.

  Paul

>      -Don Slutz
> 
> >                   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 8226d8c..aba4ef6 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -2547,7 +2547,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;
> >
> >       list_for_each_entry ( sv,
> >                             &s->ioreq_vcpu_list,
> >
> >>    Paul
> >>
> >>>    Paul
> >>>
> >>>> So I think:
> >>>>
> >>>>               rc = hvm_send_assist_req(s, &p);
> >>>>               if ( rc != X86EMUL_RETRY )
> >>>>                    vio->io_state = HVMIO_none;
> >>>>
> >>>> needs to change to:
> >>>>
> >>>>                   rc = hvm_send_assist_req(s, &p);
> >>>>                   if ( rc != X86EMUL_RETRY )
> >>>>                   {
> >>>>                       vio->io_state = HVMIO_none;
> >>>>                       if ( rc == X86EMUL_OKAY )
> >>>>                           rc = X86EMUL_RETRY;
> >>>>                   }
> >>>>
> >>>>
> >>>>      -Don Slutz
> >>> _______________________________________________
> >>> Xen-devel mailing list
> >>> Xen-devel@lists.xen.org
> >>> http://lists.xen.org/xen-devel

Re: Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Jan Beulich]

>>> On 29.06.15 at 17:14, <don.slutz@gmail.com> wrote:
> On 06/29/15 10:03, Paul Durrant wrote:
>> I think this patch should do it for now:
>>
>> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>> index a4d7225..cc6130c 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 )
> 
> I do not know enough about "is_shutting_down" to agree.  What is clear 
> is that
> this test is not the same as "!vcpu_start_shutdown_deferral(curr)".

Together with Paul's reply the main question appears to have
remained un-answered: Does the patch suggested by Paul address
the problem you observed?

Jan

Re: Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 06 July 2015 11:03
> To: Don Slutz
> Cc: Paul Durrant; xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] Migration bug added by commit
> 2df1aa01bef7366798248ac6d03cfb42048b003d
> 
> >>> On 29.06.15 at 17:14, <don.slutz@gmail.com> wrote:
> > On 06/29/15 10:03, Paul Durrant wrote:
> >> I think this patch should do it for now:
> >>
> >> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> >> index a4d7225..cc6130c 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 )
> >
> > I do not know enough about "is_shutting_down" to agree.  What is clear
> > is that
> > this test is not the same as "!vcpu_start_shutdown_deferral(curr)".
> 
> Together with Paul's reply the main question appears to have
> remained un-answered: Does the patch suggested by Paul address
> the problem you observed?
>

I can at least say that the patch definitely resolved a regression seen in automated testing of migration of Windows Server 2003 VMs on XenServer.

  Paul

 
> Jan

Re: Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Jan Beulich]

>>> On 06.07.15 at 12:08, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 06 July 2015 11:03
>> To: Don Slutz
>> Cc: Paul Durrant; xen-devel@lists.xen.org 
>> Subject: Re: [Xen-devel] Migration bug added by commit
>> 2df1aa01bef7366798248ac6d03cfb42048b003d
>> 
>> >>> On 29.06.15 at 17:14, <don.slutz@gmail.com> wrote:
>> > On 06/29/15 10:03, Paul Durrant wrote:
>> >> I think this patch should do it for now:
>> >>
>> >> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
>> >> index a4d7225..cc6130c 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 )
>> >
>> > I do not know enough about "is_shutting_down" to agree.  What is clear
>> > is that
>> > this test is not the same as "!vcpu_start_shutdown_deferral(curr)".
>> 
>> Together with Paul's reply the main question appears to have
>> remained un-answered: Does the patch suggested by Paul address
>> the problem you observed?
>>
> 
> I can at least say that the patch definitely resolved a regression seen in 
> automated testing of migration of Windows Server 2003 VMs on XenServer.

Good. Not even having got close to the end of unread mails, I
suppose I'll find it somewhere as a formal submission...

Jan

Re: Migration bug added by commit 2df1aa01bef7366798248ac6d03cfb42048b003d

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 06 July 2015 11:14
> To: Paul Durrant
> Cc: Don Slutz; xen-devel@lists.xen.org
> Subject: RE: [Xen-devel] Migration bug added by commit
> 2df1aa01bef7366798248ac6d03cfb42048b003d
> 
> >>> On 06.07.15 at 12:08, <Paul.Durrant@citrix.com> wrote:
> >>  -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 06 July 2015 11:03
> >> To: Don Slutz
> >> Cc: Paul Durrant; xen-devel@lists.xen.org
> >> Subject: Re: [Xen-devel] Migration bug added by commit
> >> 2df1aa01bef7366798248ac6d03cfb42048b003d
> >>
> >> >>> On 29.06.15 at 17:14, <don.slutz@gmail.com> wrote:
> >> > On 06/29/15 10:03, Paul Durrant wrote:
> >> >> I think this patch should do it for now:
> >> >>
> >> >> diff --git a/xen/arch/x86/hvm/emulate.c
> b/xen/arch/x86/hvm/emulate.c
> >> >> index a4d7225..cc6130c 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 )
> >> >
> >> > I do not know enough about "is_shutting_down" to agree.  What is clear
> >> > is that
> >> > this test is not the same as "!vcpu_start_shutdown_deferral(curr)".
> >>
> >> Together with Paul's reply the main question appears to have
> >> remained un-answered: Does the patch suggested by Paul address
> >> the problem you observed?
> >>
> >
> > I can at least say that the patch definitely resolved a regression seen in
> > automated testing of migration of Windows Server 2003 VMs on
> XenServer.
> 
> Good. Not even having got close to the end of unread mails, I
> suppose I'll find it somewhere as a formal submission...
> 

Yes, it's 01/16 of v6 of the series. Andrew has already reviewed it.

  Paul

> Jan