[PATCH 1/2] x86/hvm: Fix HVMOP_get_param when skipping creating the default ioreq server

[PATCH 1/2] x86/hvm: Fix HVMOP_get_param when skipping creating the default ioreq server

[Andrew Cooper]

c/s e7dabe5 "x86/hvm: don't unconditionally create a default ioreq server"
added a break statement, but the logic previously depended on falling through
into the default case to fill in the value the caller asked for.

This causes the sending migration code to put a junk PARAM into the stream,
and the receiving side to fail to zero the IOREQ pages, causing QEMU to object
when it finds stale requests while starting up.

Reorder the code so it more clearly falls through into the default case.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/hvm/hvm.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2b3977a..61f5029 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5275,9 +5275,6 @@ static int hvmop_get_param(
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
-    {
-        domid_t domid;
-
         /*
          * It may be necessary to create a default ioreq server here,
          * because legacy versions of QEMU are not aware of the new API for
@@ -5285,15 +5282,16 @@ static int hvmop_get_param(
          * under construction then it will not be QEMU querying the
          * parameters and thus the query should not have that side-effect.
          */
-        if ( d->creation_finished )
-            break;
+        if ( !d->creation_finished )
+        {
+            domid_t domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
+
+            rc = hvm_create_ioreq_server(d, domid, 1,
+                                         HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
+            if ( rc != 0 && rc != -EEXIST )
+                goto out;
+        }
 
-        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
-        rc = hvm_create_ioreq_server(d, domid, 1,
-                                     HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
-        if ( rc != 0 && rc != -EEXIST )
-            goto out;
-    }
     /*FALLTHRU*/
     default:
         a.value = d->arch.hvm_domain.params[a.index];
-- 
2.1.4


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

[PATCH 2/2] xen: Fix determining when domain creation is complete

[Andrew Cooper]

d->creation_finished is used in several places alter behaviour depending on
whether the domain is being created, or is already running.

However, there is a latent bug if a toolstack component makes a pair of
pause/unpause calls, where creation will be considered finished prematurely.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>

This is probably a backport candidate, as creation_finished was introduced in
4.8
---
 xen/common/domain.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3abaca9..05130e2 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1004,13 +1004,6 @@ int domain_unpause_by_systemcontroller(struct domain *d)
 {
     int old, new, prev = d->controller_pause_count;
 
-    /*
-     * We record this information here for populate_physmap to figure out
-     * that the domain has finished being created. In fact, we're only
-     * allowed to set the MEMF_no_tlbflush flag during VM creation.
-     */
-    d->creation_finished = true;
-
     do
     {
         old = prev;
@@ -1022,6 +1015,20 @@ int domain_unpause_by_systemcontroller(struct domain *d)
         prev = cmpxchg(&d->controller_pause_count, old, new);
     } while ( prev != old );
 
+    /*
+     * d->controller_pause_count is initialised to 1, and the toolstack is
+     * responsible for making one unpause hypercall when it wishes the guest
+     * to start running.
+     *
+     * All other toolstack operations should make a pair of pause/unpause
+     * calls and rely on the reference counting here.
+     *
+     * Creation is considered finished when the controller reference count
+     * first drops to 0.
+     */
+    if ( new == 0 )
+        d->creation_finished = true;
+
     domain_unpause(d);
 
     return 0;
-- 
2.1.4


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

Re: [PATCH 1/2] x86/hvm: Fix HVMOP_get_param when skipping creating the default ioreq server

[Jan Beulich]

>>> On 12.12.16 at 19:29, <andrew.cooper3@citrix.com> wrote:
> c/s e7dabe5 "x86/hvm: don't unconditionally create a default ioreq server"
> added a break statement, but the logic previously depended on falling through
> into the default case to fill in the value the caller asked for.
> 
> This causes the sending migration code to put a junk PARAM into the stream,
> and the receiving side to fail to zero the IOREQ pages, causing QEMU to object
> when it finds stale requests while starting up.
> 
> Reorder the code so it more clearly falls through into the default case.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 2/2] xen: Fix determining when domain creation is complete

[Jan Beulich]

>>> On 12.12.16 at 19:29, <andrew.cooper3@citrix.com> wrote:
> d->creation_finished is used in several places alter behaviour depending on
> whether the domain is being created, or is already running.
> 
> However, there is a latent bug if a toolstack component makes a pair of
> pause/unpause calls, where creation will be considered finished prematurely.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH 2/2] xen: Fix determining when domain creation is complete

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 12 December 2016 18:30
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Boris
> Ostrovsky <boris.ostrovsky@oracle.com>
> Subject: [PATCH 2/2] xen: Fix determining when domain creation is complete
> 
> d->creation_finished is used in several places alter behaviour depending on
> whether the domain is being created, or is already running.
> 
> However, there is a latent bug if a toolstack component makes a pair of
> pause/unpause calls, where creation will be considered finished
> prematurely.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>

I tested this on initial boot and restore.

Tested-by: Paul Durrant <paul.durrant@citrix.com>

> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> This is probably a backport candidate, as creation_finished was introduced in
> 4.8
> ---
>  xen/common/domain.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 3abaca9..05130e2 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1004,13 +1004,6 @@ int domain_unpause_by_systemcontroller(struct
> domain *d)
>  {
>      int old, new, prev = d->controller_pause_count;
> 
> -    /*
> -     * We record this information here for populate_physmap to figure out
> -     * that the domain has finished being created. In fact, we're only
> -     * allowed to set the MEMF_no_tlbflush flag during VM creation.
> -     */
> -    d->creation_finished = true;
> -
>      do
>      {
>          old = prev;
> @@ -1022,6 +1015,20 @@ int domain_unpause_by_systemcontroller(struct
> domain *d)
>          prev = cmpxchg(&d->controller_pause_count, old, new);
>      } while ( prev != old );
> 
> +    /*
> +     * d->controller_pause_count is initialised to 1, and the toolstack is
> +     * responsible for making one unpause hypercall when it wishes the guest
> +     * to start running.
> +     *
> +     * All other toolstack operations should make a pair of pause/unpause
> +     * calls and rely on the reference counting here.
> +     *
> +     * Creation is considered finished when the controller reference count
> +     * first drops to 0.
> +     */
> +    if ( new == 0 )
> +        d->creation_finished = true;
> +
>      domain_unpause(d);
> 
>      return 0;
> --
> 2.1.4


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

Re: [PATCH 1/2] x86/hvm: Fix HVMOP_get_param when skipping creating the default ioreq server

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 12 December 2016 18:30
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>; Boris
> Ostrovsky <boris.ostrovsky@oracle.com>
> Subject: [PATCH 1/2] x86/hvm: Fix HVMOP_get_param when skipping
> creating the default ioreq server
> 
> c/s e7dabe5 "x86/hvm: don't unconditionally create a default ioreq server"
> added a break statement, but the logic previously depended on falling
> through
> into the default case to fill in the value the caller asked for.
>

Yes, I mistakenly thought I was breaking out of an inner switch. Thanks for fixing this up.

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
 
> This causes the sending migration code to put a junk PARAM into the stream,
> and the receiving side to fail to zero the IOREQ pages, causing QEMU to
> object
> when it finds stale requests while starting up.
> 
> Reorder the code so it more clearly falls through into the default case.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 2b3977a..61f5029 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5275,9 +5275,6 @@ static int hvmop_get_param(
>      case HVM_PARAM_IOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_EVTCHN:
> -    {
> -        domid_t domid;
> -
>          /*
>           * It may be necessary to create a default ioreq server here,
>           * because legacy versions of QEMU are not aware of the new API for
> @@ -5285,15 +5282,16 @@ static int hvmop_get_param(
>           * under construction then it will not be QEMU querying the
>           * parameters and thus the query should not have that side-effect.
>           */
> -        if ( d->creation_finished )
> -            break;
> +        if ( !d->creation_finished )
> +        {
> +            domid_t domid = d-
> >arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> +
> +            rc = hvm_create_ioreq_server(d, domid, 1,
> +                                         HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
> +            if ( rc != 0 && rc != -EEXIST )
> +                goto out;
> +        }
> 
> -        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> -        rc = hvm_create_ioreq_server(d, domid, 1,
> -                                     HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
> -        if ( rc != 0 && rc != -EEXIST )
> -            goto out;
> -    }
>      /*FALLTHRU*/
>      default:
>          a.value = d->arch.hvm_domain.params[a.index];
> --
> 2.1.4


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

Re: [PATCH 2/2] xen: Fix determining when domain creation is complete

[Jan Beulich]

>>> On 12.12.16 at 19:29, <andrew.cooper3@citrix.com> wrote:
> d->creation_finished is used in several places alter behaviour depending on
> whether the domain is being created, or is already running.
> 
> However, there is a latent bug if a toolstack component makes a pair of
> pause/unpause calls, where creation will be considered finished prematurely.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> This is probably a backport candidate, as creation_finished was introduced in
> 4.8

Yes. I actually mean to take the combination of Paul's original change
and your 1/2, too.

Jan


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