[PATCH v8 00/11] toolstack-assisted approach to PVHVM guest kexec

[PATCH v8 00/11] toolstack-assisted approach to PVHVM guest kexec

[Vitaly Kuznetsov]

This patch series provides x86 PVHVM domains with an ability to perform
kexec/kdump-style operations.

The list of currently known issues blocking kexec (and why the series is
required) is:
- Bound event channels.
- Registered vcpu_info.
- PIRQ/emuirq mappings.
- shared_info frame after XENMAPSPACE_shared_info operation.
- Active grant mappings.

Previously there were several attempts to solve these issues in different ways:
- Individual 'shutdown' hypercalls (e.g. VCPUOP_reset_vcpu_info) (we agreed
  that one 'reset everything' hypercall is better).
- Try building new domain reassigning old domain's memory (memory reassignment
  turned out being too cumbersome).
- Toolstack-less 'reset everything' (turned out being impossible because there
  is not enough knowledge in the hypervisor, e.g. interdomain channels are
  being set up by the toolstack).

This series is a mix of the previously sent 'toolstack-based' and
'reset everything' series. Here are some key points:
- No new domain is created.
- Domain is asking for soft reset with SCHEDOP_shutdown with
  SHUTDOWN_soft_reset shutdown reason.
- XEN_DOMCTL_soft_reset is being called by the toolstack.
- Device model is being restarted.

Patches from 'toolstack-based' series:
- "[PATCH v8 01/11] xen: introduce SHUTDOWN_soft_reset shutdown reason"
- "[PATCH v8 02/11] libxl: support SHUTDOWN_soft_reset shutdown reason" (with Ian's ack)
- "[PATCH v8 03/11] xl: introduce enum domain_restart_type" (with Ian's ack)
- "[PATCH v8 10/11] libxc: add XC_DEVICE_MODEL_SAVE_FILE" (with Ian's ack)
- "[PATCH v8 11/11] (lib)xl: soft reset support" (heavily rewritten)

Patches from 'reset everything' RFC:
- "[PATCH v8 04/11] xen: evtchn: make evtchn_reset() ready for soft reset"
- "[PATCH v8 06/11] xen: Introduce XEN_DOMCTL_soft_reset"
- "[PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()"

New patches:
- "[PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()"
- "[PATCH v8 07/11] flask: DOMCTL_soft_reset support"
- "[PATCH v8 09/11] libxc: support XEN_DOMCTL_soft_reset operation"

With regards to active grants. In this series we restart domain's device model,
remove it from xenstore and introduce it back. The only 'misbehaving' backend
keeping active mapping I currently observe is xenconsoled: currently it has no
interface to disconnect from a domain (it just periodically scans all domains
to determine if any of them are dead). This is not an issue for us because:
- This matches standard domain startup as this grant mapping is being set up by
  the toolstack.
- Guest domain is aware of this special page.
grant_table_warn_active_grants() is required to find possible misbehaving
backends in future. It can be easily dropped from this series.

Some changes are mentioned in individual patches.

v7 of the 'toolstack-based approach to pvhvm guest kexec' is available here:
http://lists.xenproject.org/archives/html/xen-devel/2015-05/msg03558.html

RFC of the 'reset everything approach to PVHVM guest kexec' is available here:
http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg00446.html

 docs/man/xl.cfg.pod.5                        |  10 ++
 tools/flask/policy/policy/modules/xen/xen.if |   2 +-
 tools/libxc/include/xenctrl.h                |   3 +
 tools/libxc/include/xenguest.h               |   2 +
 tools/libxc/xc_domain.c                      |   9 ++
 tools/libxl/libxl.c                          |  16 ++-
 tools/libxl/libxl.h                          |  12 ++
 tools/libxl/libxl_create.c                   | 181 +++++++++++++++++++++++----
 tools/libxl/libxl_dm.c                       |   2 +-
 tools/libxl/libxl_internal.h                 |   4 +
 tools/libxl/libxl_types.idl                  |   4 +
 tools/libxl/xl.h                             |   7 ++
 tools/libxl/xl_cmdimpl.c                     |  58 ++++++---
 tools/python/xen/lowlevel/xl/xl.c            |   1 +
 xen/arch/arm/domain.c                        |   5 +
 xen/arch/x86/domain.c                        |  79 ++++++++++++
 xen/arch/x86/hvm/hvm.c                       |   2 +-
 xen/common/domain.c                          |  34 ++++-
 xen/common/domctl.c                          |   9 ++
 xen/common/event_channel.c                   |  38 +++---
 xen/common/grant_table.c                     |  31 +++++
 xen/common/shutdown.c                        |   6 +
 xen/include/asm-x86/hvm/hvm.h                |   2 +
 xen/include/public/domctl.h                  |   1 +
 xen/include/public/sched.h                   |   3 +-
 xen/include/xen/domain.h                     |   2 +
 xen/include/xen/event.h                      |   3 +
 xen/include/xen/grant_table.h                |   5 +
 xen/include/xen/sched.h                      |   2 +
 xen/xsm/flask/hooks.c                        |   3 +
 xen/xsm/flask/policy/access_vectors          |   2 +
 31 files changed, 470 insertions(+), 68 deletions(-)

-- 
2.4.2

[PATCH v8 01/11] xen: introduce SHUTDOWN_soft_reset shutdown reason

[Vitaly Kuznetsov]

This special type of shutdown is supposed to be used by PVHVM guests when
they want to perform some sort of kexec/kdump.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 xen/common/shutdown.c      | 6 ++++++
 xen/include/public/sched.h | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index 9cfbf7a..03a8641 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -66,6 +66,12 @@ void hwdom_shutdown(u8 reason)
         machine_restart(0);
         break; /* not reached */
 
+    case SHUTDOWN_soft_reset:
+        printk("Hardware domain %d did unsupported soft reset, rebooting.\n",
+               hardware_domain->domain_id);
+        machine_restart(0);
+        break; /* not reached */
+
     default:
         printk("Hardware Dom%u shutdown (unknown reason %u): ",
                hardware_domain->domain_id, reason);
diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
index 4000ac9..705bc52 100644
--- a/xen/include/public/sched.h
+++ b/xen/include/public/sched.h
@@ -159,7 +159,8 @@ DEFINE_XEN_GUEST_HANDLE(sched_watchdog_t);
 #define SHUTDOWN_suspend    2  /* Clean up, save suspend info, kill.         */
 #define SHUTDOWN_crash      3  /* Tell controller we've crashed.             */
 #define SHUTDOWN_watchdog   4  /* Restart because watchdog time expired.     */
-#define SHUTDOWN_MAX        4  /* Maximum valid shutdown reason.             */
+#define SHUTDOWN_soft_reset 5  /* Domain did soft reset. Clean up and resume.*/
+#define SHUTDOWN_MAX        5  /* Maximum valid shutdown reason.             */
 /* ` } */
 
 #endif /* __XEN_PUBLIC_SCHED_H__ */
-- 
2.4.2

Re: [PATCH v8 01/11] xen: introduce SHUTDOWN_soft_reset shutdown reason

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 06:11:43PM +0200, Vitaly Kuznetsov wrote:
> This special type of shutdown is supposed to be used by PVHVM guests when
> they want to perform some sort of kexec/kdump.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> ---
>  xen/common/shutdown.c      | 6 ++++++
>  xen/include/public/sched.h | 3 ++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
> index 9cfbf7a..03a8641 100644
> --- a/xen/common/shutdown.c
> +++ b/xen/common/shutdown.c
> @@ -66,6 +66,12 @@ void hwdom_shutdown(u8 reason)
>          machine_restart(0);
>          break; /* not reached */
>  
> +    case SHUTDOWN_soft_reset:
> +        printk("Hardware domain %d did unsupported soft reset, rebooting.\n",
> +               hardware_domain->domain_id);
> +        machine_restart(0);
> +        break; /* not reached */
> +
>      default:
>          printk("Hardware Dom%u shutdown (unknown reason %u): ",
>                 hardware_domain->domain_id, reason);
> diff --git a/xen/include/public/sched.h b/xen/include/public/sched.h
> index 4000ac9..705bc52 100644
> --- a/xen/include/public/sched.h
> +++ b/xen/include/public/sched.h
> @@ -159,7 +159,8 @@ DEFINE_XEN_GUEST_HANDLE(sched_watchdog_t);
>  #define SHUTDOWN_suspend    2  /* Clean up, save suspend info, kill.         */
>  #define SHUTDOWN_crash      3  /* Tell controller we've crashed.             */
>  #define SHUTDOWN_watchdog   4  /* Restart because watchdog time expired.     */
> -#define SHUTDOWN_MAX        4  /* Maximum valid shutdown reason.             */
> +#define SHUTDOWN_soft_reset 5  /* Domain did soft reset. Clean up and resume.*/
> +#define SHUTDOWN_MAX        5  /* Maximum valid shutdown reason.             */
>  /* ` } */
>  
>  #endif /* __XEN_PUBLIC_SCHED_H__ */
> -- 
> 2.4.2
> 

Re: [PATCH v8 01/11] xen: introduce SHUTDOWN_soft_reset shutdown reason

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [PATCH v8 01/11] xen: introduce SHUTDOWN_soft_reset shutdown reason"):
> On Tue, Jun 23, 2015 at 06:11:43PM +0200, Vitaly Kuznetsov wrote:
> > This special type of shutdown is supposed to be used by PVHVM guests when
> > they want to perform some sort of kexec/kdump.
...
> > +#define SHUTDOWN_soft_reset 5  /* Domain did soft reset. Clean up and resume.*/

I would like more documentation about the semantics of this new
request.  (The semantics of the existing shutdown requests are fairly
well understood because they generally map to real hardware.)

Ian.

Re: [PATCH v8 01/11] xen: introduce SHUTDOWN_soft_reset shutdown reason

[Vitaly Kuznetsov]

Ian Jackson <Ian.Jackson@eu.citrix.com> writes:

> Konrad Rzeszutek Wilk writes ("Re: [PATCH v8 01/11] xen: introduce SHUTDOWN_soft_reset shutdown reason"):
>> On Tue, Jun 23, 2015 at 06:11:43PM +0200, Vitaly Kuznetsov wrote:
>> > This special type of shutdown is supposed to be used by PVHVM guests when
>> > they want to perform some sort of kexec/kdump.
> ...
>> > +#define SHUTDOWN_soft_reset 5  /* Domain did soft reset. Clean up and resume.*/
>
> I would like more documentation about the semantics of this new
> request.  (The semantics of the existing shutdown requests are fairly
> well understood because they generally map to real hardware.)

Sure,

would you like me to expand the comment here or should I write something
somewhere else?

Thanks,

-- 
  Vitaly

[PATCH v8 02/11] libxl: support SHUTDOWN_soft_reset shutdown reason

[Vitaly Kuznetsov]

Use letter 'S' to indicate a domain in such state.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_types.idl       | 1 +
 tools/libxl/xl_cmdimpl.c          | 2 +-
 tools/python/xen/lowlevel/xl/xl.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 23f27d4..9001f65 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -177,6 +177,7 @@ libxl_shutdown_reason = Enumeration("shutdown_reason", [
     (2, "suspend"),
     (3, "crash"),
     (4, "watchdog"),
+    (5, "soft_reset"),
     ], init_val = "LIBXL_SHUTDOWN_REASON_UNKNOWN")
 
 libxl_vga_interface_type = Enumeration("vga_interface_type", [
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c858068..7247cf1 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3725,7 +3725,7 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
                          bool cpupool, const libxl_dominfo *info, int nb_domain)
 {
     int i;
-    static const char shutdown_reason_letters[]= "-rscw";
+    static const char shutdown_reason_letters[]= "-rscwS";
     libxl_bitmap nodemap;
     libxl_physinfo physinfo;
 
diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
index 32f982a..7c61160 100644
--- a/tools/python/xen/lowlevel/xl/xl.c
+++ b/tools/python/xen/lowlevel/xl/xl.c
@@ -784,6 +784,7 @@ PyMODINIT_FUNC initxl(void)
     _INT_CONST_LIBXL(m, SHUTDOWN_REASON_SUSPEND);
     _INT_CONST_LIBXL(m, SHUTDOWN_REASON_CRASH);
     _INT_CONST_LIBXL(m, SHUTDOWN_REASON_WATCHDOG);
+    _INT_CONST_LIBXL(m, SHUTDOWN_REASON_SOFT_RESET);
 
     genwrap__init(m);
 }
-- 
2.4.2

Re: [PATCH v8 02/11] libxl: support SHUTDOWN_soft_reset shutdown reason

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 06:11:44PM +0200, Vitaly Kuznetsov wrote:
> Use letter 'S' to indicate a domain in such state.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/libxl_types.idl       | 1 +
>  tools/libxl/xl_cmdimpl.c          | 2 +-
>  tools/python/xen/lowlevel/xl/xl.c | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 23f27d4..9001f65 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -177,6 +177,7 @@ libxl_shutdown_reason = Enumeration("shutdown_reason", [
>      (2, "suspend"),
>      (3, "crash"),
>      (4, "watchdog"),
> +    (5, "soft_reset"),
>      ], init_val = "LIBXL_SHUTDOWN_REASON_UNKNOWN")
>  
>  libxl_vga_interface_type = Enumeration("vga_interface_type", [
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c858068..7247cf1 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3725,7 +3725,7 @@ static void list_domains(bool verbose, bool context, bool claim, bool numa,
>                           bool cpupool, const libxl_dominfo *info, int nb_domain)
>  {
>      int i;
> -    static const char shutdown_reason_letters[]= "-rscw";
> +    static const char shutdown_reason_letters[]= "-rscwS";
>      libxl_bitmap nodemap;
>      libxl_physinfo physinfo;
>  
> diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
> index 32f982a..7c61160 100644
> --- a/tools/python/xen/lowlevel/xl/xl.c
> +++ b/tools/python/xen/lowlevel/xl/xl.c
> @@ -784,6 +784,7 @@ PyMODINIT_FUNC initxl(void)
>      _INT_CONST_LIBXL(m, SHUTDOWN_REASON_SUSPEND);
>      _INT_CONST_LIBXL(m, SHUTDOWN_REASON_CRASH);
>      _INT_CONST_LIBXL(m, SHUTDOWN_REASON_WATCHDOG);
> +    _INT_CONST_LIBXL(m, SHUTDOWN_REASON_SOFT_RESET);
>  
>      genwrap__init(m);
>  }
> -- 
> 2.4.2
> 

Re: [PATCH v8 02/11] libxl: support SHUTDOWN_soft_reset shutdown reason

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [PATCH v8 02/11] libxl: support SHUTDOWN_soft_reset shutdown reason"):
> On Tue, Jun 23, 2015 at 06:11:44PM +0200, Vitaly Kuznetsov wrote:
> > Use letter 'S' to indicate a domain in such state.
> > 
> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I would like to see the basic xl parts in this patch too; that is,
introduce `on_soft_reset' etc., presumably with a default of `reboot'.

That would make patch 11/11 easier to read, too, because it would
consist only of the new action, and not any of the new on_ option.

Ian.

Re: [PATCH v8 02/11] libxl: support SHUTDOWN_soft_reset shutdown reason

[Vitaly Kuznetsov]

Ian Jackson <Ian.Jackson@eu.citrix.com> writes:

> Konrad Rzeszutek Wilk writes ("Re: [PATCH v8 02/11] libxl: support SHUTDOWN_soft_reset shutdown reason"):
>> On Tue, Jun 23, 2015 at 06:11:44PM +0200, Vitaly Kuznetsov wrote:
>> > Use letter 'S' to indicate a domain in such state.
>> > 
>> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
>> 
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> I would like to see the basic xl parts in this patch too; that is,
> introduce `on_soft_reset' etc., presumably with a default of `reboot'.
>
> That would make patch 11/11 easier to read, too, because it would
> consist only of the new action, and not any of the new on_ option.

Ok, will move some stuff from PATCH 11 here.

Thanks,

-- 
  Vitaly

[PATCH v8 03/11] xl: introduce enum domain_restart_type

[Vitaly Kuznetsov]

As a preparation before adding new restart type (soft reset) put all
restart types into an enum.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
Changes since v7:
- s,restarter,restarted, [Ian Campbell]
---
 tools/libxl/xl.h         |  6 ++++++
 tools/libxl/xl_cmdimpl.c | 23 ++++++++++-------------
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 5bc138c..374680c 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -186,6 +186,12 @@ enum output_format {
 };
 extern enum output_format default_output_format;
 
+typedef enum {
+    DOMAIN_RESTART_NONE = 0,     /* No domain restart */
+    DOMAIN_RESTART_NORMAL,       /* Domain should be restarted */
+    DOMAIN_RESTART_RENAME,       /* Domain should be renamed and restarted */
+} domain_restart_type;
+
 extern void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh);
 
 #define XL_GLOBAL_CONFIG XEN_CONFIG_DIR "/xl.conf"
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 7247cf1..32217fa 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -2283,15 +2283,12 @@ static void reload_domain_config(uint32_t domid,
     }
 }
 
-/* Returns 1 if domain should be restarted,
- * 2 if domain should be renamed then restarted, or 0
- * Can update r_domid if domain is destroyed etc */
-static int handle_domain_death(uint32_t *r_domid,
-                               libxl_event *event,
-                               libxl_domain_config *d_config)
-
+/* Can update r_domid if domain is destroyed */
+static domain_restart_type handle_domain_death(uint32_t *r_domid,
+                                               libxl_event *event,
+                                               libxl_domain_config *d_config)
 {
-    int restart = 0;
+    domain_restart_type restart = DOMAIN_RESTART_NONE;
     libxl_action_on_shutdown action;
 
     switch (event->u.domain_shutdown.shutdown_reason) {
@@ -2346,12 +2343,12 @@ static int handle_domain_death(uint32_t *r_domid,
 
     case LIBXL_ACTION_ON_SHUTDOWN_RESTART_RENAME:
         reload_domain_config(*r_domid, d_config);
-        restart = 2;
+        restart = DOMAIN_RESTART_RENAME;
         break;
 
     case LIBXL_ACTION_ON_SHUTDOWN_RESTART:
         reload_domain_config(*r_domid, d_config);
-        restart = 1;
+        restart = DOMAIN_RESTART_NORMAL;
         /* fall-through */
     case LIBXL_ACTION_ON_SHUTDOWN_DESTROY:
         LOG("Domain %d needs to be cleaned up: destroying the domain",
@@ -2799,7 +2796,7 @@ start:
                 event->u.domain_shutdown.shutdown_reason,
                 event->u.domain_shutdown.shutdown_reason);
             switch (handle_domain_death(&domid, event, &d_config)) {
-            case 2:
+            case DOMAIN_RESTART_RENAME:
                 if (!preserve_domain(&domid, event, &d_config)) {
                     /* If we fail then exit leaving the old domain in place. */
                     ret = -1;
@@ -2807,7 +2804,7 @@ start:
                 }
 
                 /* Otherwise fall through and restart. */
-            case 1:
+            case DOMAIN_RESTART_NORMAL:
                 libxl_event_free(ctx, event);
                 libxl_evdisable_domain_death(ctx, deathw);
                 deathw = NULL;
@@ -2843,7 +2840,7 @@ start:
                 sleep(2);
                 goto start;
 
-            case 0:
+            case DOMAIN_RESTART_NONE:
                 LOG("Done. Exiting now");
                 libxl_event_free(ctx, event);
                 ret = 0;
-- 
2.4.2

Re: [PATCH v8 03/11] xl: introduce enum domain_restart_type

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 06:11:45PM +0200, Vitaly Kuznetsov wrote:
> As a preparation before adding new restart type (soft reset) put all
> restart types into an enum.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Changes since v7:
> - s,restarter,restarted, [Ian Campbell]
> ---
>  tools/libxl/xl.h         |  6 ++++++
>  tools/libxl/xl_cmdimpl.c | 23 ++++++++++-------------
>  2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
> index 5bc138c..374680c 100644
> --- a/tools/libxl/xl.h
> +++ b/tools/libxl/xl.h
> @@ -186,6 +186,12 @@ enum output_format {
>  };
>  extern enum output_format default_output_format;
>  
> +typedef enum {
> +    DOMAIN_RESTART_NONE = 0,     /* No domain restart */
> +    DOMAIN_RESTART_NORMAL,       /* Domain should be restarted */
> +    DOMAIN_RESTART_RENAME,       /* Domain should be renamed and restarted */
> +} domain_restart_type;
> +
>  extern void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh);
>  
>  #define XL_GLOBAL_CONFIG XEN_CONFIG_DIR "/xl.conf"
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 7247cf1..32217fa 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2283,15 +2283,12 @@ static void reload_domain_config(uint32_t domid,
>      }
>  }
>  
> -/* Returns 1 if domain should be restarted,
> - * 2 if domain should be renamed then restarted, or 0
> - * Can update r_domid if domain is destroyed etc */
> -static int handle_domain_death(uint32_t *r_domid,
> -                               libxl_event *event,
> -                               libxl_domain_config *d_config)
> -
> +/* Can update r_domid if domain is destroyed */
> +static domain_restart_type handle_domain_death(uint32_t *r_domid,
> +                                               libxl_event *event,
> +                                               libxl_domain_config *d_config)
>  {
> -    int restart = 0;
> +    domain_restart_type restart = DOMAIN_RESTART_NONE;
>      libxl_action_on_shutdown action;
>  
>      switch (event->u.domain_shutdown.shutdown_reason) {
> @@ -2346,12 +2343,12 @@ static int handle_domain_death(uint32_t *r_domid,
>  
>      case LIBXL_ACTION_ON_SHUTDOWN_RESTART_RENAME:
>          reload_domain_config(*r_domid, d_config);
> -        restart = 2;
> +        restart = DOMAIN_RESTART_RENAME;
>          break;
>  
>      case LIBXL_ACTION_ON_SHUTDOWN_RESTART:
>          reload_domain_config(*r_domid, d_config);
> -        restart = 1;
> +        restart = DOMAIN_RESTART_NORMAL;
>          /* fall-through */
>      case LIBXL_ACTION_ON_SHUTDOWN_DESTROY:
>          LOG("Domain %d needs to be cleaned up: destroying the domain",
> @@ -2799,7 +2796,7 @@ start:
>                  event->u.domain_shutdown.shutdown_reason,
>                  event->u.domain_shutdown.shutdown_reason);
>              switch (handle_domain_death(&domid, event, &d_config)) {
> -            case 2:
> +            case DOMAIN_RESTART_RENAME:
>                  if (!preserve_domain(&domid, event, &d_config)) {
>                      /* If we fail then exit leaving the old domain in place. */
>                      ret = -1;
> @@ -2807,7 +2804,7 @@ start:
>                  }
>  
>                  /* Otherwise fall through and restart. */
> -            case 1:
> +            case DOMAIN_RESTART_NORMAL:
>                  libxl_event_free(ctx, event);
>                  libxl_evdisable_domain_death(ctx, deathw);
>                  deathw = NULL;
> @@ -2843,7 +2840,7 @@ start:
>                  sleep(2);
>                  goto start;
>  
> -            case 0:
> +            case DOMAIN_RESTART_NONE:
>                  LOG("Done. Exiting now");
>                  libxl_event_free(ctx, event);
>                  ret = 0;
> -- 
> 2.4.2
> 

[PATCH v8 04/11] xen: evtchn: make evtchn_reset() ready for soft reset

[Vitaly Kuznetsov]

We need to close all event channel so the domain performing soft reset
will be able to open them back.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 xen/common/event_channel.c | 38 +++++++++++++++++++-------------------
 xen/include/xen/event.h    |  3 +++
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 2208de0..894bdf2 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -937,26 +937,19 @@ int evtchn_unmask(unsigned int port)
 }
 
 
-static long evtchn_reset(evtchn_reset_t *r)
+void evtchn_reset(struct domain *d)
 {
-    domid_t dom = r->dom;
-    struct domain *d;
-    int i, rc;
-
-    d = rcu_lock_domain_by_any_id(dom);
-    if ( d == NULL )
-        return -ESRCH;
+    unsigned int i;
 
-    rc = xsm_evtchn_reset(XSM_TARGET, current->domain, d);
-    if ( rc )
-        goto out;
+    if ( d != current->domain )
+        domain_pause(d);
 
     for ( i = 0; port_is_valid(d, i); i++ )
         (void)__evtchn_close(d, i);
 
     spin_lock(&d->event_lock);
 
-    if ( (dom == DOMID_SELF) && d->evtchn_fifo )
+    if ( d->evtchn_fifo )
     {
         /*
          * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying
@@ -969,12 +962,8 @@ static long evtchn_reset(evtchn_reset_t *r)
 
     spin_unlock(&d->event_lock);
 
-    rc = 0;
-
-out:
-    rcu_unlock_domain(d);
-
-    return rc;
+    if ( d != current->domain )
+        domain_unpause(d);
 }
 
 static long evtchn_set_priority(const struct evtchn_set_priority *set_priority)
@@ -1099,9 +1088,20 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     case EVTCHNOP_reset: {
         struct evtchn_reset reset;
+        struct domain *d;
+
         if ( copy_from_guest(&reset, arg, 1) != 0 )
             return -EFAULT;
-        rc = evtchn_reset(&reset);
+
+        d = rcu_lock_domain_by_any_id(reset.dom);
+        if ( d == NULL )
+            return -ESRCH;
+
+        rc = xsm_evtchn_reset(XSM_TARGET, current->domain, d);
+        if ( !rc )
+            evtchn_reset(d);
+
+        rcu_unlock_domain(d);
         break;
     }
 
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index af923d1..b99e19b 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -126,6 +126,9 @@ void evtchn_check_pollers(struct domain *d, unsigned int port);
 
 void evtchn_2l_init(struct domain *d);
 
+/* Close all event channels and reset to 2-level ABI */
+void evtchn_reset(struct domain *d);
+
 /*
  * Low-level event channel port ops.
  */
-- 
2.4.2

Re: [PATCH v8 04/11] xen: evtchn: make evtchn_reset() ready for soft reset

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 06:11:46PM +0200, Vitaly Kuznetsov wrote:
> We need to close all event channel so the domain performing soft reset
> will be able to open them back.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  xen/common/event_channel.c | 38 +++++++++++++++++++-------------------
>  xen/include/xen/event.h    |  3 +++
>  2 files changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 2208de0..894bdf2 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -937,26 +937,19 @@ int evtchn_unmask(unsigned int port)
>  }
>  
>  
> -static long evtchn_reset(evtchn_reset_t *r)
> +void evtchn_reset(struct domain *d)
>  {
> -    domid_t dom = r->dom;
> -    struct domain *d;
> -    int i, rc;
> -
> -    d = rcu_lock_domain_by_any_id(dom);
> -    if ( d == NULL )
> -        return -ESRCH;
> +    unsigned int i;
>  
> -    rc = xsm_evtchn_reset(XSM_TARGET, current->domain, d);
> -    if ( rc )
> -        goto out;
> +    if ( d != current->domain )
> +        domain_pause(d);

That is a bit of blocking operation. Could it be an requirement
that the domain has to be paused before this operation is done?

As in:

	if ( d != current->domain && !d->controller_pause_count)
	{
		return -EINVAL;
	}
and then there is no need for the domain_pause/unpause business
here.
>  
>      for ( i = 0; port_is_valid(d, i); i++ )
>          (void)__evtchn_close(d, i);
>  
>      spin_lock(&d->event_lock);
>  
> -    if ( (dom == DOMID_SELF) && d->evtchn_fifo )
> +    if ( d->evtchn_fifo )
>      {
>          /*
>           * Guest domain called EVTCHNOP_reset with DOMID_SELF, destroying

You should also update the comment saying 'DOMID_SELF' as now
it can be another domain doing it as well.

> @@ -969,12 +962,8 @@ static long evtchn_reset(evtchn_reset_t *r)
>  
>      spin_unlock(&d->event_lock);
>  
> -    rc = 0;
> -
> -out:
> -    rcu_unlock_domain(d);
> -
> -    return rc;
> +    if ( d != current->domain )
> +        domain_unpause(d);
>  }
>  
>  static long evtchn_set_priority(const struct evtchn_set_priority *set_priority)
> @@ -1099,9 +1088,20 @@ long do_event_channel_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>      case EVTCHNOP_reset: {
>          struct evtchn_reset reset;
> +        struct domain *d;
> +
>          if ( copy_from_guest(&reset, arg, 1) != 0 )
>              return -EFAULT;
> -        rc = evtchn_reset(&reset);
> +
> +        d = rcu_lock_domain_by_any_id(reset.dom);
> +        if ( d == NULL )
> +            return -ESRCH;
> +
> +        rc = xsm_evtchn_reset(XSM_TARGET, current->domain, d);
> +        if ( !rc )
> +            evtchn_reset(d);
> +
> +        rcu_unlock_domain(d);
>          break;
>      }
>  
> diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
> index af923d1..b99e19b 100644
> --- a/xen/include/xen/event.h
> +++ b/xen/include/xen/event.h
> @@ -126,6 +126,9 @@ void evtchn_check_pollers(struct domain *d, unsigned int port);
>  
>  void evtchn_2l_init(struct domain *d);
>  
> +/* Close all event channels and reset to 2-level ABI */
> +void evtchn_reset(struct domain *d);
> +
>  /*
>   * Low-level event channel port ops.
>   */
> -- 
> 2.4.2
> 

[PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()

[Vitaly Kuznetsov]

Log first 10 active grants of a domain. This function is going to be used
for soft reset, active grants on this path usually mean misbehaving backends
refusing to release their mappings on shutdown.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 xen/common/grant_table.c      | 31 +++++++++++++++++++++++++++++++
 xen/include/xen/grant_table.h |  5 +++++
 2 files changed, 36 insertions(+)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index db5e5db..c67db28 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3309,6 +3309,37 @@ gnttab_release_mappings(
     }
 }
 
+void grant_table_warn_active_grants(struct domain *d)
+{
+    struct grant_table *gt = d->grant_table;
+    struct active_grant_entry *act;
+    grant_ref_t ref;
+    unsigned int nr_active = 0;
+
+    read_lock(&gt->lock);
+
+    for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
+    {
+        act = active_entry_acquire(gt, ref);
+        if ( !act->pin )
+        {
+            active_entry_release(act);
+            continue;
+        }
+
+        nr_active++;
+        if ( nr_active <= 10 )
+            printk(XENLOG_G_DEBUG "Dom%d has an active grant: GFN: %lx"
+                   " (MFN: %lx)\n", d->domain_id, act->gfn, act->frame);
+        active_entry_release(act);
+    }
+
+    if ( nr_active > 10 )
+        printk(XENLOG_G_DEBUG "Dom%d has too many (%d) active grants"
+               " to report\n", d->domain_id, nr_active);
+
+    read_unlock(&gt->lock);
+}
 
 void
 grant_table_destroy(
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 9c7b5a3..54005cc 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -90,6 +90,11 @@ void grant_table_destroy(
     struct domain *d);
 void grant_table_init_vcpu(struct vcpu *v);
 
+/*
+ * Check if domain has active grants and log first 10 of them.
+ */
+void grant_table_warn_active_grants(struct domain *d);
+
 /* Domain death release of granted mappings of other domains' memory. */
 void
 gnttab_release_mappings(
-- 
2.4.2

Re: [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote:
> Log first 10 active grants of a domain. This function is going to be used
> for soft reset, active grants on this path usually mean misbehaving backends
> refusing to release their mappings on shutdown.

Is there an particular reason 10 was choosen instead of 42 for example :-)

Also the 10 should probably have an #define for it.

Not sure I understand the usage case - except for development uses
to report on the Xen console? But if that is the case why not
use the 'g' on the ring console?

> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  xen/common/grant_table.c      | 31 +++++++++++++++++++++++++++++++
>  xen/include/xen/grant_table.h |  5 +++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index db5e5db..c67db28 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -3309,6 +3309,37 @@ gnttab_release_mappings(
>      }
>  }
>  
> +void grant_table_warn_active_grants(struct domain *d)
> +{
> +    struct grant_table *gt = d->grant_table;
> +    struct active_grant_entry *act;
> +    grant_ref_t ref;
> +    unsigned int nr_active = 0;
> +
> +    read_lock(&gt->lock);
> +
> +    for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
> +    {
> +        act = active_entry_acquire(gt, ref);
> +        if ( !act->pin )
> +        {
> +            active_entry_release(act);
> +            continue;
> +        }
> +
> +        nr_active++;
> +        if ( nr_active <= 10 )
> +            printk(XENLOG_G_DEBUG "Dom%d has an active grant: GFN: %lx"
> +                   " (MFN: %lx)\n", d->domain_id, act->gfn, act->frame);
> +        active_entry_release(act);
> +    }
> +
> +    if ( nr_active > 10 )
> +        printk(XENLOG_G_DEBUG "Dom%d has too many (%d) active grants"
> +               " to report\n", d->domain_id, nr_active);
> +
> +    read_unlock(&gt->lock);
> +}
>  
>  void
>  grant_table_destroy(
> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
> index 9c7b5a3..54005cc 100644
> --- a/xen/include/xen/grant_table.h
> +++ b/xen/include/xen/grant_table.h
> @@ -90,6 +90,11 @@ void grant_table_destroy(
>      struct domain *d);
>  void grant_table_init_vcpu(struct vcpu *v);
>  
> +/*
> + * Check if domain has active grants and log first 10 of them.
> + */
> +void grant_table_warn_active_grants(struct domain *d);
> +
>  /* Domain death release of granted mappings of other domains' memory. */
>  void
>  gnttab_release_mappings(
> -- 
> 2.4.2
> 

Re: [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()

[Jan Beulich]

>>> On 10.07.15 at 18:24, <konrad.wilk@oracle.com> wrote:
> On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote:
>> Log first 10 active grants of a domain. This function is going to be used
>> for soft reset, active grants on this path usually mean misbehaving backends
>> refusing to release their mappings on shutdown.
> 
> Is there an particular reason 10 was choosen instead of 42 for example :-)
> 
> Also the 10 should probably have an #define for it.

Or even be command line controllable.

Jan

Re: [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()

[Ian Campbell]

On Mon, 2015-07-13 at 09:45 +0100, Jan Beulich wrote:
> >>> On 10.07.15 at 18:24, <konrad.wilk@oracle.com> wrote:
> > On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote:
> >> Log first 10 active grants of a domain. This function is going to be used
> >> for soft reset, active grants on this path usually mean misbehaving backends
> >> refusing to release their mappings on shutdown.
> > 
> > Is there an particular reason 10 was choosen instead of 42 for example :-)
> > 
> > Also the 10 should probably have an #define for it.
> 
> Or even be command line controllable.

That sounds like overkill to me, what's wrong with some random hardcoded
number for a simple debug aid like this?


Ian.

Re: [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()

[Jan Beulich]

>>> On 13.07.15 at 11:08, <ian.campbell@citrix.com> wrote:
> On Mon, 2015-07-13 at 09:45 +0100, Jan Beulich wrote:
>> >>> On 10.07.15 at 18:24, <konrad.wilk@oracle.com> wrote:
>> > On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote:
>> >> Log first 10 active grants of a domain. This function is going to be used
>> >> for soft reset, active grants on this path usually mean misbehaving 
> backends
>> >> refusing to release their mappings on shutdown.
>> > 
>> > Is there an particular reason 10 was choosen instead of 42 for example :-)
>> > 
>> > Also the 10 should probably have an #define for it.
>> 
>> Or even be command line controllable.
> 
> That sounds like overkill to me, what's wrong with some random hardcoded
> number for a simple debug aid like this?

>From briefly looking at the code it seemed to be more than just a
debug aid (i.e. failing the operation if the count was exceeded). If
the number indeed only controls how many entries get printed,
then a #define certainly is fine.

Jan

Re: [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()

[Vitaly Kuznetsov]

"Jan Beulich" <JBeulich@suse.com> writes:

>>>> On 13.07.15 at 11:08, <ian.campbell@citrix.com> wrote:
>> On Mon, 2015-07-13 at 09:45 +0100, Jan Beulich wrote:
>>> >>> On 10.07.15 at 18:24, <konrad.wilk@oracle.com> wrote:
>>> > On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote:
>>> >> Log first 10 active grants of a domain. This function is going to be used
>>> >> for soft reset, active grants on this path usually mean misbehaving 
>> backends
>>> >> refusing to release their mappings on shutdown.
>>> > 
>>> > Is there an particular reason 10 was choosen instead of 42 for example :-)
>>> > 
>>> > Also the 10 should probably have an #define for it.
>>> 
>>> Or even be command line controllable.
>> 
>> That sounds like overkill to me, what's wrong with some random hardcoded
>> number for a simple debug aid like this?
>
> From briefly looking at the code it seemed to be more than just a
> debug aid (i.e. failing the operation if the count was exceeded). If
> the number indeed only controls how many entries get printed,
> then a #define certainly is fine.

Yes, it is just a debug aid in cases something goes wrong in
future. This info is supposed to be useful for hardware domain admin to
help finding misbehaving backends.

-- 
  Vitaly

Re: [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()

[Vitaly Kuznetsov]

Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:

> On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote:
>> Log first 10 active grants of a domain. This function is going to be used
>> for soft reset, active grants on this path usually mean misbehaving backends
>> refusing to release their mappings on shutdown.
>
> Is there an particular reason 10 was choosen instead of 42 for example :-)
>

I was inpired by dump_pageframe_info() :-)

> Also the 10 should probably have an #define for it.
>

Ok, any preferred place/name for such define?

> Not sure I understand the usage case - except for development uses
> to report on the Xen console? But if that is the case why not
> use the 'g' on the ring console?

If there is a misbehaving backend and this cases the domain to crash
right after the soft reset 'g' option will not be available and it won't
be clear what caused the domain to crash.

>
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> ---
>>  xen/common/grant_table.c      | 31 +++++++++++++++++++++++++++++++
>>  xen/include/xen/grant_table.h |  5 +++++
>>  2 files changed, 36 insertions(+)
>> 
>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
>> index db5e5db..c67db28 100644
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -3309,6 +3309,37 @@ gnttab_release_mappings(
>>      }
>>  }
>>  
>> +void grant_table_warn_active_grants(struct domain *d)
>> +{
>> +    struct grant_table *gt = d->grant_table;
>> +    struct active_grant_entry *act;
>> +    grant_ref_t ref;
>> +    unsigned int nr_active = 0;
>> +
>> +    read_lock(&gt->lock);
>> +
>> +    for ( ref = 0; ref != nr_grant_entries(gt); ref++ )
>> +    {
>> +        act = active_entry_acquire(gt, ref);
>> +        if ( !act->pin )
>> +        {
>> +            active_entry_release(act);
>> +            continue;
>> +        }
>> +
>> +        nr_active++;
>> +        if ( nr_active <= 10 )
>> +            printk(XENLOG_G_DEBUG "Dom%d has an active grant: GFN: %lx"
>> +                   " (MFN: %lx)\n", d->domain_id, act->gfn, act->frame);
>> +        active_entry_release(act);
>> +    }
>> +
>> +    if ( nr_active > 10 )
>> +        printk(XENLOG_G_DEBUG "Dom%d has too many (%d) active grants"
>> +               " to report\n", d->domain_id, nr_active);
>> +
>> +    read_unlock(&gt->lock);
>> +}
>>  
>>  void
>>  grant_table_destroy(
>> diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
>> index 9c7b5a3..54005cc 100644
>> --- a/xen/include/xen/grant_table.h
>> +++ b/xen/include/xen/grant_table.h
>> @@ -90,6 +90,11 @@ void grant_table_destroy(
>>      struct domain *d);
>>  void grant_table_init_vcpu(struct vcpu *v);
>>  
>> +/*
>> + * Check if domain has active grants and log first 10 of them.
>> + */
>> +void grant_table_warn_active_grants(struct domain *d);
>> +
>>  /* Domain death release of granted mappings of other domains' memory. */
>>  void
>>  gnttab_release_mappings(
>> -- 
>> 2.4.2
>> 

-- 
  Vitaly

Re: [PATCH v8 05/11] xen: grant_table: implement grant_table_warn_active_grants()

[Konrad Rzeszutek Wilk]

On Mon, Jul 13, 2015 at 03:44:04PM +0200, Vitaly Kuznetsov wrote:
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:
> 
> > On Tue, Jun 23, 2015 at 06:11:47PM +0200, Vitaly Kuznetsov wrote:
> >> Log first 10 active grants of a domain. This function is going to be used
> >> for soft reset, active grants on this path usually mean misbehaving backends
> >> refusing to release their mappings on shutdown.
> >
> > Is there an particular reason 10 was choosen instead of 42 for example :-)
> >
> 
> I was inpired by dump_pageframe_info() :-)

Ah!
> 
> > Also the 10 should probably have an #define for it.
> >
> 
> Ok, any preferred place/name for such define?

I would say just include it at the start of the function (and #undef
at the end), and maybe later on (if you want to) do a cleanup patch for
this and dump_pageframe_info to be controlled by the same
'too long to display' logic.

Which I would say could be a function that :
 - if 'loglvl=info' is used would do the 10.
 - if 'loglvl=all', then there is no limit.

But that is such a minor thing.
> 
> > Not sure I understand the usage case - except for development uses
> > to report on the Xen console? But if that is the case why not
> > use the 'g' on the ring console?
> 
> If there is a misbehaving backend and this cases the domain to crash
> right after the soft reset 'g' option will not be available and it won't
> be clear what caused the domain to crash.

Aah, that is good information. Please include that in the commit.

[PATCH v8 06/11] xen: Introduce XEN_DOMCTL_soft_reset

[Vitaly Kuznetsov]

New domctl resets state for a domain allowing it to 'start over': register
vcpu_info, switch to FIFO ABI for event channels. Still active grants are
being logged to help debugging misbehaving backends.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 xen/common/domain.c         | 29 ++++++++++++++++++++++++++---
 xen/common/domctl.c         |  9 +++++++++
 xen/include/public/domctl.h |  1 +
 xen/include/xen/sched.h     |  2 ++
 4 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 3bc52e6..ade80ff 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1010,6 +1010,26 @@ int domain_unpause_by_systemcontroller(struct domain *d)
     return 0;
 }
 
+int domain_soft_reset(struct domain *d)
+{
+    struct vcpu *v;
+
+    for_each_vcpu ( d, v )
+        if ( !v->paused_for_shutdown )
+            return -EINVAL;
+
+    evtchn_reset(d);
+
+    grant_table_warn_active_grants(d);
+
+    for_each_vcpu ( d, v )
+        unmap_vcpu_info(v);
+
+    domain_resume(d);
+
+    return 0;
+}
+
 int vcpu_reset(struct vcpu *v)
 {
     struct domain *d = v->domain;
@@ -1121,12 +1141,12 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
 
 /*
  * Unmap the vcpu info page if the guest decided to place it somewhere
- * else.  This is only used from arch_domain_destroy, so there's no
- * need to do anything clever.
+ * else. This is used from arch_domain_destroy and domain_soft_reset.
  */
 void unmap_vcpu_info(struct vcpu *v)
 {
     unsigned long mfn;
+    struct domain *d = v->domain;
 
     if ( v->vcpu_info_mfn == INVALID_MFN )
         return;
@@ -1135,7 +1155,10 @@ void unmap_vcpu_info(struct vcpu *v)
     unmap_domain_page_global((void *)
                              ((unsigned long)v->vcpu_info & PAGE_MASK));
 
-    v->vcpu_info = &dummy_vcpu_info;
+    v->vcpu_info = ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS)
+                    ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
+                    : &dummy_vcpu_info);
+
     v->vcpu_info_mfn = INVALID_MFN;
 
     put_page_and_type(mfn_to_page(mfn));
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index ce517a7..8cb6a2a 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -703,6 +703,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         break;
     }
 
+    case XEN_DOMCTL_soft_reset:
+        if ( d == current->domain )
+        {
+            ret = -EINVAL;
+            break;
+        }
+        ret = domain_soft_reset(d);
+        break;
+
     case XEN_DOMCTL_destroydomain:
         ret = domain_kill(d);
         if ( ret == -ERESTART )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index bc45ea5..5b4ba6b 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1114,6 +1114,7 @@ struct xen_domctl {
 #define XEN_DOMCTL_setvnumainfo                  74
 #define XEN_DOMCTL_psr_cmt_op                    75
 #define XEN_DOMCTL_monitor_op                    77
+#define XEN_DOMCTL_soft_reset                    78
 #define XEN_DOMCTL_gdbsx_guestmemio            1000
 #define XEN_DOMCTL_gdbsx_pausevcpu             1001
 #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index d810e1c..1120741 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -609,6 +609,8 @@ void domain_shutdown(struct domain *d, u8 reason);
 void domain_resume(struct domain *d);
 void domain_pause_for_debugger(void);
 
+int domain_soft_reset(struct domain *d);
+
 int vcpu_start_shutdown_deferral(struct vcpu *v);
 void vcpu_end_shutdown_deferral(struct vcpu *v);
 
-- 
2.4.2

Re: [PATCH v8 06/11] xen: Introduce XEN_DOMCTL_soft_reset

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 06:11:48PM +0200, Vitaly Kuznetsov wrote:
> New domctl resets state for a domain allowing it to 'start over': register
> vcpu_info, switch to FIFO ABI for event channels. Still active grants are
> being logged to help debugging misbehaving backends.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  xen/common/domain.c         | 29 ++++++++++++++++++++++++++---
>  xen/common/domctl.c         |  9 +++++++++
>  xen/include/public/domctl.h |  1 +
>  xen/include/xen/sched.h     |  2 ++
>  4 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 3bc52e6..ade80ff 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1010,6 +1010,26 @@ int domain_unpause_by_systemcontroller(struct domain *d)
>      return 0;
>  }
>  
> +int domain_soft_reset(struct domain *d)
> +{
> +    struct vcpu *v;
> +
> +    for_each_vcpu ( d, v )
> +        if ( !v->paused_for_shutdown )
> +            return -EINVAL;

I think you need to hold a lock to make sure that nobody else
during this loop messes with 'v->paused_for_shutdown' value.

That is the shutdown_lock.

> +
> +    evtchn_reset(d);
> +
> +    grant_table_warn_active_grants(d);
> +
> +    for_each_vcpu ( d, v )
> +        unmap_vcpu_info(v);
> +
> +    domain_resume(d);
> +
> +    return 0;
> +}
> +
>  int vcpu_reset(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
> @@ -1121,12 +1141,12 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
>  
>  /*
>   * Unmap the vcpu info page if the guest decided to place it somewhere
> - * else.  This is only used from arch_domain_destroy, so there's no
> - * need to do anything clever.
> + * else. This is used from arch_domain_destroy and domain_soft_reset.
>   */
>  void unmap_vcpu_info(struct vcpu *v)
>  {
>      unsigned long mfn;
> +    struct domain *d = v->domain;
>  
>      if ( v->vcpu_info_mfn == INVALID_MFN )
>          return;
> @@ -1135,7 +1155,10 @@ void unmap_vcpu_info(struct vcpu *v)
>      unmap_domain_page_global((void *)
>                               ((unsigned long)v->vcpu_info & PAGE_MASK));
>  
> -    v->vcpu_info = &dummy_vcpu_info;
> +    v->vcpu_info = ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS)
> +                    ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
> +                    : &dummy_vcpu_info);
> +

Could you explain that bit please? Why not use dummy_vcpu_info?

Ah, you are doing what alloc_vcpu does.
>      v->vcpu_info_mfn = INVALID_MFN;

Perhaps we should jus thave a function called 'vcpu_info_reset' that
would do this - and called in both places?

>  
>      put_page_and_type(mfn_to_page(mfn));
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index ce517a7..8cb6a2a 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -703,6 +703,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>          break;
>      }
>  
> +    case XEN_DOMCTL_soft_reset:
> +        if ( d == current->domain )
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +        ret = domain_soft_reset(d);
> +        break;
> +
>      case XEN_DOMCTL_destroydomain:
>          ret = domain_kill(d);
>          if ( ret == -ERESTART )
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index bc45ea5..5b4ba6b 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1114,6 +1114,7 @@ struct xen_domctl {
>  #define XEN_DOMCTL_setvnumainfo                  74
>  #define XEN_DOMCTL_psr_cmt_op                    75
>  #define XEN_DOMCTL_monitor_op                    77
> +#define XEN_DOMCTL_soft_reset                    78
>  #define XEN_DOMCTL_gdbsx_guestmemio            1000
>  #define XEN_DOMCTL_gdbsx_pausevcpu             1001
>  #define XEN_DOMCTL_gdbsx_unpausevcpu           1002
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index d810e1c..1120741 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -609,6 +609,8 @@ void domain_shutdown(struct domain *d, u8 reason);
>  void domain_resume(struct domain *d);
>  void domain_pause_for_debugger(void);
>  
> +int domain_soft_reset(struct domain *d);
> +
>  int vcpu_start_shutdown_deferral(struct vcpu *v);
>  void vcpu_end_shutdown_deferral(struct vcpu *v);
>  
> -- 
> 2.4.2
> 

[PATCH v8 07/11] flask: DOMCTL_soft_reset support

[Vitaly Kuznetsov]

Add new soft_reset vector to domain2 class, add it to create_domain
in the default policy.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 tools/flask/policy/policy/modules/xen/xen.if | 2 +-
 xen/xsm/flask/hooks.c                        | 3 +++
 xen/xsm/flask/policy/access_vectors          | 2 ++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.if b/tools/flask/policy/policy/modules/xen/xen.if
index f4cde11..0dcd620 100644
--- a/tools/flask/policy/policy/modules/xen/xen.if
+++ b/tools/flask/policy/policy/modules/xen/xen.if
@@ -52,7 +52,7 @@ define(`create_domain_common', `
 			getaffinity setaffinity setvcpuextstate };
 	allow $1 $2:domain2 { set_cpuid settsc setscheduler setclaim
 			set_max_evtchn set_vnumainfo get_vnumainfo cacheflush
-			psr_cmt_op };
+			psr_cmt_op soft_reset };
 	allow $1 $2:security check_context;
 	allow $1 $2:shadow enable;
 	allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 6e37d29..0c9a2ed 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -735,6 +735,9 @@ static int flask_domctl(struct domain *d, int cmd)
     case XEN_DOMCTL_psr_cmt_op:
         return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__PSR_CMT_OP);
 
+    case XEN_DOMCTL_soft_reset:
+        return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SOFT_RESET);
+
     default:
         printk("flask_domctl: Unknown op %d\n", cmd);
         return -EPERM;
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 68284d5..a1bb241 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -224,6 +224,8 @@ class domain2
 # XEN_DOMCTL_monitor_op
 # XEN_DOMCTL_vm_event_op
     vm_event
+# XEN_DOMCTL_soft_reset
+    soft_reset
 # XENMEM_access_op
     mem_access
 # XENMEM_paging_op
-- 
2.4.2

Re: [PATCH v8 07/11] flask: DOMCTL_soft_reset support

[Daniel De Graaf]

On 06/23/2015 12:11 PM, Vitaly Kuznetsov wrote:
> Add new soft_reset vector to domain2 class, add it to create_domain
> in the default policy.
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

[PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()

[Vitaly Kuznetsov]

x86-specific hook cleans up the pirq-emuirq mappings, destroys all ioreq
servers and and replaces the shared_info frame with an empty page to support
subsequent XENMAPSPACE_shared_info call.

ARM-specific hook is a noop for now.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
Changes since 'PATCH RFC' of the 'reset everything' approach to PVHVM guest kexec:
- Coding style, check get_gfn_query() return value, various minor fixes [Jan Beulich]
- Do not unpause VCPUs on arch hook failure [Jan Beulich]
---
 xen/arch/arm/domain.c         |  5 +++
 xen/arch/x86/domain.c         | 79 +++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/hvm.c        |  2 +-
 xen/common/domain.c           |  5 +++
 xen/include/asm-x86/hvm/hvm.h |  2 ++
 xen/include/xen/domain.h      |  2 ++
 6 files changed, 94 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 24b8938..c112afa 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
 {
 }
 
+int arch_domain_soft_reset(struct domain *d)
+{
+    return 0;
+}
+
 static int is_guest_pv32_psr(uint32_t psr)
 {
     switch (psr & PSR_MODE_MASK)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0363650..2dd0e0d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -682,6 +682,85 @@ void arch_domain_unpause(struct domain *d)
         viridian_time_ref_count_thaw(d);
 }
 
+int arch_domain_soft_reset(struct domain *d)
+{
+    struct page_info *page = virt_to_page(d->shared_info), *new_page;
+    int ret = 0;
+    struct domain *owner;
+    unsigned long mfn, mfn_new, gfn;
+    p2m_type_t p2mt;
+    unsigned int i;
+
+    /* Soft reset is supported for HVM domains only */
+    if ( !is_hvm_domain(d) )
+        return -EINVAL;
+
+    hvm_destroy_all_ioreq_servers(d);
+
+    spin_lock(&d->event_lock);
+    for ( i = 1; i < d->nr_pirqs ; i ++ )
+        if ( domain_pirq_to_emuirq(d, i) != IRQ_UNBOUND )
+        {
+            ret = unmap_domain_pirq_emuirq(d, i);
+            if ( ret )
+                break;
+        }
+    spin_unlock(&d->event_lock);
+
+    if ( ret )
+        return ret;
+
+    /*
+     * shared_info page needs to be replaced with a new page, otherwise we
+     * will get a hole if the domain does XENMAPSPACE_shared_info
+     */
+
+    owner = page_get_owner_and_reference(page);
+    /* If shared_info page wasn't mounted, we do not need to replace it */
+    if ( owner != d )
+        goto exit_put_page;
+
+    mfn = page_to_mfn(page);
+    if ( !mfn_valid(mfn) )
+    {
+        printk(XENLOG_G_ERR "Dom%d's shared_info page points to invalid MFN\n",
+               d->domain_id);
+        ret = -EINVAL;
+        goto exit_put_page;
+    }
+
+    gfn = mfn_to_gmfn(d, mfn);
+    if ( mfn_x(get_gfn_query(d, gfn, &p2mt)) == INVALID_MFN )
+    {
+        printk(XENLOG_G_ERR "Failed to get Dom%d's shared_info GFN (%lx)\n",
+               d->domain_id, gfn);
+        ret = -EINVAL;
+        goto exit_put_page;
+    }
+
+    new_page = alloc_domheap_page(d, 0);
+    if ( !new_page )
+    {
+        printk(XENLOG_G_ERR "Failed to alloc a page to replace"
+               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
+        ret = -ENOMEM;
+        goto exit_put_gfn;
+    }
+    mfn_new = page_to_mfn(new_page);
+    guest_physmap_remove_page(d, gfn, mfn, 0);
+
+    ret = guest_physmap_add_page(d, gfn, mfn_new, 0);
+    if ( ret )
+        printk(XENLOG_G_ERR "Failed to add a page to replace"
+               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
+ exit_put_gfn:
+    put_gfn(d, gfn);
+ exit_put_page:
+    put_page(page);
+
+    return ret;
+}
+
 unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
 {
     unsigned long hv_cr4_mask, hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d5e5242..506a7be 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1319,7 +1319,7 @@ static void hvm_all_ioreq_servers_remove_vcpu(struct domain *d, struct vcpu *v)
     spin_unlock(&d->arch.hvm_domain.ioreq_server.lock);
 }
 
-static void hvm_destroy_all_ioreq_servers(struct domain *d)
+void hvm_destroy_all_ioreq_servers(struct domain *d)
 {
     struct hvm_ioreq_server *s, *next;
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index ade80ff..cd5ed42 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1013,6 +1013,7 @@ int domain_unpause_by_systemcontroller(struct domain *d)
 int domain_soft_reset(struct domain *d)
 {
     struct vcpu *v;
+    int ret;
 
     for_each_vcpu ( d, v )
         if ( !v->paused_for_shutdown )
@@ -1025,6 +1026,10 @@ int domain_soft_reset(struct domain *d)
     for_each_vcpu ( d, v )
         unmap_vcpu_info(v);
 
+    ret = arch_domain_soft_reset(d);
+    if (ret)
+        return ret;
+
     domain_resume(d);
 
     return 0;
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 77eeac5..ffdc379 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -234,6 +234,8 @@ bool_t hvm_send_assist_req(struct hvm_ioreq_server *s, ioreq_t *p);
 void hvm_broadcast_assist_req(ioreq_t *p);
 void hvm_complete_assist_req(ioreq_t *p);
 
+void hvm_destroy_all_ioreq_servers(struct domain *d);
+
 void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat);
 int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat);
 
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 848db8a..a469fe0 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -65,6 +65,8 @@ void arch_domain_shutdown(struct domain *d);
 void arch_domain_pause(struct domain *d);
 void arch_domain_unpause(struct domain *d);
 
+int arch_domain_soft_reset(struct domain *d);
+
 int arch_set_info_guest(struct vcpu *, vcpu_guest_context_u);
 void arch_get_info_guest(struct vcpu *, vcpu_guest_context_u);
 
-- 
2.4.2

Re: [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 06:11:50PM +0200, Vitaly Kuznetsov wrote:
> x86-specific hook cleans up the pirq-emuirq mappings, destroys all ioreq
> servers and and replaces the shared_info frame with an empty page to support
> subsequent XENMAPSPACE_shared_info call.
> 
> ARM-specific hook is a noop for now.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> Changes since 'PATCH RFC' of the 'reset everything' approach to PVHVM guest kexec:
> - Coding style, check get_gfn_query() return value, various minor fixes [Jan Beulich]
> - Do not unpause VCPUs on arch hook failure [Jan Beulich]
> ---
>  xen/arch/arm/domain.c         |  5 +++
>  xen/arch/x86/domain.c         | 79 +++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c        |  2 +-
>  xen/common/domain.c           |  5 +++
>  xen/include/asm-x86/hvm/hvm.h |  2 ++
>  xen/include/xen/domain.h      |  2 ++
>  6 files changed, 94 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 24b8938..c112afa 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
>  {
>  }
>  
> +int arch_domain_soft_reset(struct domain *d)
> +{
> +    return 0;
> +}
> +
>  static int is_guest_pv32_psr(uint32_t psr)
>  {
>      switch (psr & PSR_MODE_MASK)
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 0363650..2dd0e0d 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -682,6 +682,85 @@ void arch_domain_unpause(struct domain *d)
>          viridian_time_ref_count_thaw(d);
>  }
>  
> +int arch_domain_soft_reset(struct domain *d)
> +{
> +    struct page_info *page = virt_to_page(d->shared_info), *new_page;
> +    int ret = 0;
> +    struct domain *owner;
> +    unsigned long mfn, mfn_new, gfn;
> +    p2m_type_t p2mt;
> +    unsigned int i;
> +
> +    /* Soft reset is supported for HVM domains only */

Missing full stop. But perhaps we could explain what would be needed
for an PV guest to make it work (not as something for you to do
of course but an victim^H^H^Hvolunteer!).

> +    if ( !is_hvm_domain(d) )
> +        return -EINVAL;
> +
> +    hvm_destroy_all_ioreq_servers(d);
> +
> +    spin_lock(&d->event_lock);
> +    for ( i = 1; i < d->nr_pirqs ; i ++ )

Is pirq 0 special?

s/i ++/i++/

> +        if ( domain_pirq_to_emuirq(d, i) != IRQ_UNBOUND )
> +        {
> +            ret = unmap_domain_pirq_emuirq(d, i);
> +            if ( ret )
> +                break;
> +        }

Could you add outer {} to the loop please?

> +    spin_unlock(&d->event_lock);
> +
> +    if ( ret )
> +        return ret;
> +
> +    /*
> +     * shared_info page needs to be replaced with a new page, otherwise we
> +     * will get a hole if the domain does XENMAPSPACE_shared_info

Full stop missing.
> +     */
> +
> +    owner = page_get_owner_and_reference(page);
> +    /* If shared_info page wasn't mounted, we do not need to replace it */

s/mounted/used/
Missing full stop.
> +    if ( owner != d )
> +        goto exit_put_page;
> +
> +    mfn = page_to_mfn(page);
> +    if ( !mfn_valid(mfn) )
> +    {
> +        printk(XENLOG_G_ERR "Dom%d's shared_info page points to invalid MFN\n",
> +               d->domain_id);

Would it be good to print out the PFN of the shared page to help narrow the cause?

> +        ret = -EINVAL;
> +        goto exit_put_page;
> +    }
> +
> +    gfn = mfn_to_gmfn(d, mfn);
> +    if ( mfn_x(get_gfn_query(d, gfn, &p2mt)) == INVALID_MFN )
> +    {
> +        printk(XENLOG_G_ERR "Failed to get Dom%d's shared_info GFN (%lx)\n",
> +               d->domain_id, gfn);
> +        ret = -EINVAL;
> +        goto exit_put_page;
> +    }
> +
> +    new_page = alloc_domheap_page(d, 0);
> +    if ( !new_page )
> +    {
> +        printk(XENLOG_G_ERR "Failed to alloc a page to replace"
> +               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
> +        ret = -ENOMEM;
> +        goto exit_put_gfn;
> +    }
> +    mfn_new = page_to_mfn(new_page);
> +    guest_physmap_remove_page(d, gfn, mfn, 0);

s/0/PAGE_ORDER_4K/
> +
> +    ret = guest_physmap_add_page(d, gfn, mfn_new, 0);

s/0/PAGE_ORDER_4K/

> +    if ( ret )
> +        printk(XENLOG_G_ERR "Failed to add a page to replace"
> +               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);

Should we free the new_page in this case?

> + exit_put_gfn:
> +    put_gfn(d, gfn);
> + exit_put_page:
> +    put_page(page);
> +
> +    return ret;
> +}
> +
>  unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
>  {
>      unsigned long hv_cr4_mask, hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d5e5242..506a7be 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1319,7 +1319,7 @@ static void hvm_all_ioreq_servers_remove_vcpu(struct domain *d, struct vcpu *v)
>      spin_unlock(&d->arch.hvm_domain.ioreq_server.lock);
>  }
>  
> -static void hvm_destroy_all_ioreq_servers(struct domain *d)
> +void hvm_destroy_all_ioreq_servers(struct domain *d)
>  {
>      struct hvm_ioreq_server *s, *next;
>  
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index ade80ff..cd5ed42 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1013,6 +1013,7 @@ int domain_unpause_by_systemcontroller(struct domain *d)
>  int domain_soft_reset(struct domain *d)
>  {
>      struct vcpu *v;
> +    int ret;
>  
>      for_each_vcpu ( d, v )
>          if ( !v->paused_for_shutdown )
> @@ -1025,6 +1026,10 @@ int domain_soft_reset(struct domain *d)
>      for_each_vcpu ( d, v )
>          unmap_vcpu_info(v);
>  
> +    ret = arch_domain_soft_reset(d);
> +    if (ret)
> +        return ret;
> +
>      domain_resume(d);
>  
>      return 0;
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 77eeac5..ffdc379 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -234,6 +234,8 @@ bool_t hvm_send_assist_req(struct hvm_ioreq_server *s, ioreq_t *p);
>  void hvm_broadcast_assist_req(ioreq_t *p);
>  void hvm_complete_assist_req(ioreq_t *p);
>  
> +void hvm_destroy_all_ioreq_servers(struct domain *d);
> +
>  void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat);
>  int hvm_set_guest_pat(struct vcpu *v, u64 guest_pat);
>  
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 848db8a..a469fe0 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -65,6 +65,8 @@ void arch_domain_shutdown(struct domain *d);
>  void arch_domain_pause(struct domain *d);
>  void arch_domain_unpause(struct domain *d);
>  
> +int arch_domain_soft_reset(struct domain *d);
> +
>  int arch_set_info_guest(struct vcpu *, vcpu_guest_context_u);
>  void arch_get_info_guest(struct vcpu *, vcpu_guest_context_u);
>  
> -- 
> 2.4.2
> 

Re: [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()

[Vitaly Kuznetsov]

Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:

> On Tue, Jun 23, 2015 at 06:11:50PM +0200, Vitaly Kuznetsov wrote:
>> ...
>>  
>> +int arch_domain_soft_reset(struct domain *d)
>> +{
>> +    struct page_info *page = virt_to_page(d->shared_info), *new_page;
>> +    int ret = 0;
>> +    struct domain *owner;
>> +    unsigned long mfn, mfn_new, gfn;
>> +    p2m_type_t p2mt;
>> +    unsigned int i;
>> +
>> +    /* Soft reset is supported for HVM domains only */
>
> Missing full stop. But perhaps we could explain what would be needed
> for an PV guest to make it work (not as something for you to do
> of course but an victim^H^H^Hvolunteer!).
>

Oh, I seriously doubt we'll ever be doing soft reset for classic PV. I
don't see an easy way to preserve existent memory and without this soft
reset is pointless. PVH (or PVHVM-without-dm) looks much more
promising.

>> +    if ( !is_hvm_domain(d) )
>> +        return -EINVAL;

I suggest we leave it here with the comment above and decide something
later based on the chosen path for PVH.

>> +
>> +    hvm_destroy_all_ioreq_servers(d);
>> +
>> +    spin_lock(&d->event_lock);
>> +    for ( i = 1; i < d->nr_pirqs ; i ++ )
>
> Is pirq 0 special?
>

No, for some reason I though it is not a valid number for pirq. Will fix
in v9.

>> +    if ( owner != d )
>> +        goto exit_put_page;
>> +
>> +    mfn = page_to_mfn(page);
>> +    if ( !mfn_valid(mfn) )
>> +    {
>> +        printk(XENLOG_G_ERR "Dom%d's shared_info page points to invalid MFN\n",
>> +               d->domain_id);
>
> Would it be good to print out the PFN of the shared page to help narrow the cause?
>

I think this case is impossibe under normal circumstances and it's not
clear to me how to get the PFN (did you mean GFN?) in such case.

shared_info is always allocated in arch_domain_create() from Xen heap
and page_to_mfn() will never return INVALID_MFN here. In case we'll ever
see this printed we'll have examine why this is not true anymore. This
piece of code will have to be updated.

>> +    if ( ret )
>> +        printk(XENLOG_G_ERR "Failed to add a page to replace"
>> +               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
>
> Should we free the new_page in this case?
>

The new page is already in domain's page_list so we won't lose it on
domain destroy but there is also no point in keeping it there if we
failed to add it to physmap. Will free it in v9.


-- 
  Vitaly

Re: [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()

[Konrad Rzeszutek Wilk]

On Tue, Jul 14, 2015 at 05:52:44PM +0200, Vitaly Kuznetsov wrote:
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:
> 
> > On Tue, Jun 23, 2015 at 06:11:50PM +0200, Vitaly Kuznetsov wrote:
> >> ...
> >>  
> >> +int arch_domain_soft_reset(struct domain *d)
> >> +{
> >> +    struct page_info *page = virt_to_page(d->shared_info), *new_page;
> >> +    int ret = 0;
> >> +    struct domain *owner;
> >> +    unsigned long mfn, mfn_new, gfn;
> >> +    p2m_type_t p2mt;
> >> +    unsigned int i;
> >> +
> >> +    /* Soft reset is supported for HVM domains only */
> >
> > Missing full stop. But perhaps we could explain what would be needed
> > for an PV guest to make it work (not as something for you to do
> > of course but an victim^H^H^Hvolunteer!).
> >
> 
> Oh, I seriously doubt we'll ever be doing soft reset for classic PV. I
> don't see an easy way to preserve existent memory and without this soft
> reset is pointless. PVH (or PVHVM-without-dm) looks much more
> promising.
> 
> >> +    if ( !is_hvm_domain(d) )
> >> +        return -EINVAL;
> 
> I suggest we leave it here with the comment above and decide something
> later based on the chosen path for PVH.
> 
> >> +
> >> +    hvm_destroy_all_ioreq_servers(d);
> >> +
> >> +    spin_lock(&d->event_lock);
> >> +    for ( i = 1; i < d->nr_pirqs ; i ++ )
> >
> > Is pirq 0 special?
> >
> 
> No, for some reason I though it is not a valid number for pirq. Will fix
> in v9.
> 
> >> +    if ( owner != d )
> >> +        goto exit_put_page;
> >> +
> >> +    mfn = page_to_mfn(page);
> >> +    if ( !mfn_valid(mfn) )
> >> +    {
> >> +        printk(XENLOG_G_ERR "Dom%d's shared_info page points to invalid MFN\n",
> >> +               d->domain_id);
> >
> > Would it be good to print out the PFN of the shared page to help narrow the cause?
> >
> 
> I think this case is impossibe under normal circumstances and it's not
> clear to me how to get the PFN (did you mean GFN?) in such case.

Yes.
> 
> shared_info is always allocated in arch_domain_create() from Xen heap
> and page_to_mfn() will never return INVALID_MFN here. In case we'll ever
> see this printed we'll have examine why this is not true anymore. This
> piece of code will have to be updated.

Ok, One way it could be if the guest decided to expunge this GFN fro
the guest (I think). Thought I am not sure why it would do such a thing :-)

> 
> >> +    if ( ret )
> >> +        printk(XENLOG_G_ERR "Failed to add a page to replace"
> >> +               " Dom%d's shared_info frame %lx\n", d->domain_id, gfn);
> >
> > Should we free the new_page in this case?
> >
> 
> The new page is already in domain's page_list so we won't lose it on
> domain destroy but there is also no point in keeping it there if we
> failed to add it to physmap. Will free it in v9.

Thanks!
> 
> 
> -- 
>   Vitaly

Re: [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()

[Vitaly Kuznetsov]

Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:

> On Tue, Jul 14, 2015 at 05:52:44PM +0200, Vitaly Kuznetsov wrote:
>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> writes:
>> 
>> > On Tue, Jun 23, 2015 at 06:11:50PM +0200, Vitaly Kuznetsov wrote:
>> >> ...
>> >>  
>> >> +int arch_domain_soft_reset(struct domain *d)
>> >> +{
>> >> +    struct page_info *page = virt_to_page(d->shared_info), *new_page;
>> >> +    int ret = 0;
>> >> +    struct domain *owner;
>> >> +    unsigned long mfn, mfn_new, gfn;
>> >> +    p2m_type_t p2mt;
>> >> +    unsigned int i;
>> >> +
>> >> ...
>> >> +    /* Soft reset is supported for HVM domains only */
>> >> +    if ( owner != d )
>> >> +        goto exit_put_page;
>> >> +
>> >> +    mfn = page_to_mfn(page);
>> >> +    if ( !mfn_valid(mfn) )
>> >> +    {
>> >> +        printk(XENLOG_G_ERR "Dom%d's shared_info page points to invalid MFN\n",
>> >> +               d->domain_id);
>> >
>> > Would it be good to print out the PFN of the shared page to help narrow the cause?
>> >
>> 
>> I think this case is impossibe under normal circumstances and it's not
>> clear to me how to get the PFN (did you mean GFN?) in such case.
>
> Yes.
>> 
>> shared_info is always allocated in arch_domain_create() from Xen heap
>> and page_to_mfn() will never return INVALID_MFN here. In case we'll ever
>> see this printed we'll have examine why this is not true anymore. This
>> piece of code will have to be updated.
>
> Ok, One way it could be if the guest decided to expunge this GFN fro
> the guest (I think). Thought I am not sure why it would do such a thing :-)
>

I'm not sure as we do page_to_mfn(virt_to_page(d->shared_info)) here and
d->shared_info is a hypervisor-s va, guest domain can't do anything to
it.

-- 
  Vitaly

Re: [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()

[Julien Grall]

Hi Vitaly,

On 23/06/2015 18:11, Vitaly Kuznetsov wrote:
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 24b8938..c112afa 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
>   {
>   }
>
> +int arch_domain_soft_reset(struct domain *d)
> +{
> +    return 0;

I suspect that it would needs some code in this function in order to 
support soft reset on ARM. If so, should not you return -ENOSYS until 
someone take care of the ARM support?

> +}
> +

Regards,

-- 
Julien Grall

Re: [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()

[Vitaly Kuznetsov]

Julien Grall <julien.grall@citrix.com> writes:

> Hi Vitaly,
>
> On 23/06/2015 18:11, Vitaly Kuznetsov wrote:
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 24b8938..c112afa 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
>>   {
>>   }
>>
>> +int arch_domain_soft_reset(struct domain *d)
>> +{
>> +    return 0;
>
> I suspect that it would needs some code in this function in order to
> support soft reset on ARM. If so, should not you return -ENOSYS until
> someone take care of the ARM support?
>

I was hoping kexec for ARM will magically work without any arch-specific
bits here :-)

Sure, let's make it -ENOSYS.

>> +}
>> +
>
> Regards,

-- 
  Vitaly

Re: [PATCH v8 08/11] xen: arch-specific hooks for domain_soft_reset()

[Julien Grall]

Hi Vitaly,

On 16/07/2015 13:36, Vitaly Kuznetsov wrote:
> Julien Grall <julien.grall@citrix.com> writes:
>> On 23/06/2015 18:11, Vitaly Kuznetsov wrote:
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index 24b8938..c112afa 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -653,6 +653,11 @@ void arch_domain_unpause(struct domain *d)
>>>    {
>>>    }
>>>
>>> +int arch_domain_soft_reset(struct domain *d)
>>> +{
>>> +    return 0;
>>
>> I suspect that it would needs some code in this function in order to
>> support soft reset on ARM. If so, should not you return -ENOSYS until
>> someone take care of the ARM support?
>>
>
> I was hoping kexec for ARM will magically work without any arch-specific
> bits here :-)

I don't know how work kexec for Xen, but I think we will have at least 
to do the same as x86 for the shared info page.

> Sure, let's make it -ENOSYS.

Thank you!


-- 
Julien Grall

[PATCH v8 09/11] libxc: support XEN_DOMCTL_soft_reset operation

[Vitaly Kuznetsov]

Introduce xc_domain_soft_reset() function supporting XEN_DOMCTL_soft_reset.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 tools/libxc/include/xenctrl.h | 3 +++
 tools/libxc/xc_domain.c       | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index d1d2ab3..7aa0e81 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1301,6 +1301,9 @@ int xc_domain_setvnuma(xc_interface *xch,
                         unsigned int *vcpu_to_vnode,
                         unsigned int *vnode_to_pnode);
 
+int xc_domain_soft_reset(xc_interface *xch,
+                         uint32_t domid);
+
 #if defined(__i386__) || defined(__x86_64__)
 /*
  * PC BIOS standard E820 types and structure.
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index ce51e69..a59d0b0 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2452,6 +2452,15 @@ int xc_domain_setvnuma(xc_interface *xch,
     return rc;
 }
 
+
+int xc_domain_soft_reset(xc_interface *xch,
+                         uint32_t domid)
+{
+    DECLARE_DOMCTL;
+    domctl.cmd = XEN_DOMCTL_soft_reset;
+    domctl.domain = (domid_t)domid;
+    return do_domctl(xch, &domctl);
+}
 /*
  * Local variables:
  * mode: C
-- 
2.4.2

Re: [PATCH v8 09/11] libxc: support XEN_DOMCTL_soft_reset operation

[Wei Liu]

On Tue, Jun 23, 2015 at 06:11:51PM +0200, Vitaly Kuznetsov wrote:
> Introduce xc_domain_soft_reset() function supporting XEN_DOMCTL_soft_reset.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxc/include/xenctrl.h | 3 +++
>  tools/libxc/xc_domain.c       | 9 +++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index d1d2ab3..7aa0e81 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1301,6 +1301,9 @@ int xc_domain_setvnuma(xc_interface *xch,
>                          unsigned int *vcpu_to_vnode,
>                          unsigned int *vnode_to_pnode);
>  
> +int xc_domain_soft_reset(xc_interface *xch,
> +                         uint32_t domid);
> +
>  #if defined(__i386__) || defined(__x86_64__)
>  /*
>   * PC BIOS standard E820 types and structure.
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index ce51e69..a59d0b0 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -2452,6 +2452,15 @@ int xc_domain_setvnuma(xc_interface *xch,
>      return rc;
>  }
>  
> +
> +int xc_domain_soft_reset(xc_interface *xch,
> +                         uint32_t domid)
> +{
> +    DECLARE_DOMCTL;
> +    domctl.cmd = XEN_DOMCTL_soft_reset;
> +    domctl.domain = (domid_t)domid;
> +    return do_domctl(xch, &domctl);
> +}
>  /*
>   * Local variables:
>   * mode: C
> -- 
> 2.4.2

Re: [PATCH v8 09/11] libxc: support XEN_DOMCTL_soft_reset operation

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 06:11:51PM +0200, Vitaly Kuznetsov wrote:
> Introduce xc_domain_soft_reset() function supporting XEN_DOMCTL_soft_reset.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Well, that is easy to review :-)

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxc/include/xenctrl.h | 3 +++
>  tools/libxc/xc_domain.c       | 9 +++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index d1d2ab3..7aa0e81 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1301,6 +1301,9 @@ int xc_domain_setvnuma(xc_interface *xch,
>                          unsigned int *vcpu_to_vnode,
>                          unsigned int *vnode_to_pnode);
>  
> +int xc_domain_soft_reset(xc_interface *xch,
> +                         uint32_t domid);
> +
>  #if defined(__i386__) || defined(__x86_64__)
>  /*
>   * PC BIOS standard E820 types and structure.
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index ce51e69..a59d0b0 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -2452,6 +2452,15 @@ int xc_domain_setvnuma(xc_interface *xch,
>      return rc;
>  }
>  
> +
> +int xc_domain_soft_reset(xc_interface *xch,
> +                         uint32_t domid)
> +{
> +    DECLARE_DOMCTL;
> +    domctl.cmd = XEN_DOMCTL_soft_reset;
> +    domctl.domain = (domid_t)domid;
> +    return do_domctl(xch, &domctl);
> +}
>  /*
>   * Local variables:
>   * mode: C
> -- 
> 2.4.2
> 

Re: [PATCH v8 09/11] libxc: support XEN_DOMCTL_soft_reset operation

[Ian Jackson]

Vitaly Kuznetsov writes ("[PATCH v8 09/11] libxc: support XEN_DOMCTL_soft_reset operation"):
> Introduce xc_domain_soft_reset() function supporting XEN_DOMCTL_soft_reset.

This is a sane-looking hypercall wrapper, so

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

[PATCH v8 10/11] libxc: add XC_DEVICE_MODEL_SAVE_FILE

[Vitaly Kuznetsov]

Use this in libxl_dm instead of hard-coding.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxc/include/xenguest.h | 2 ++
 tools/libxl/libxl_dm.c         | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h
index 7581263..c95e93c 100644
--- a/tools/libxc/include/xenguest.h
+++ b/tools/libxc/include/xenguest.h
@@ -147,6 +147,8 @@ int xc_domain_restore2(xc_interface *xch, int io_fd, uint32_t dom,
  * of the new domain is automatically appended to the filename,
  * separated by a ".".
  */
+
+#define XC_DEVICE_MODEL_SAVE_FILE "/var/lib/xen/qemu-save"
 #define XC_DEVICE_MODEL_RESTORE_FILE "/var/lib/xen/qemu-resume"
 
 /**
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 33f9ce6..db789ac 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -31,7 +31,7 @@ static const char *libxl_tapif_script(libxl__gc *gc)
 
 const char *libxl__device_model_savefile(libxl__gc *gc, uint32_t domid)
 {
-    return libxl__sprintf(gc, "/var/lib/xen/qemu-save.%d", domid);
+    return libxl__sprintf(gc, XC_DEVICE_MODEL_SAVE_FILE".%d", domid);
 }
 
 static const char *qemu_xen_path(libxl__gc *gc)
-- 
2.4.2

[PATCH v8 11/11] (lib)xl: soft reset support

[Vitaly Kuznetsov]

Use existing create/restore path to perform 'soft reset' for HVM domains.
Tear everything down, e.g. destroy domain's device model, remove the domain
from xenstore, save toolstack record and start over.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
Changes since 'v7'
- 'Reset everything approach': XEN_DOMCTL_soft_reset doesn't destroy the
  original domain any more.
- libxl__domain_soft_reset_state introduced [Ian Campbell]
- Separate do_domain_soft_reset to not intertwine with do_domain_create
  [Ian Campbell]

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 docs/man/xl.cfg.pod.5        |  10 +++
 tools/libxl/libxl.c          |  16 +++-
 tools/libxl/libxl.h          |  12 +++
 tools/libxl/libxl_create.c   | 181 +++++++++++++++++++++++++++++++++++++------
 tools/libxl/libxl_internal.h |   4 +
 tools/libxl/libxl_types.idl  |   3 +
 tools/libxl/xl.h             |   1 +
 tools/libxl/xl_cmdimpl.c     |  33 +++++++-
 8 files changed, 232 insertions(+), 28 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index a3e0e2e..27f4c2a 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -368,6 +368,11 @@ destroy the domain.
 write a "coredump" of the domain to F</var/lib/xen/dump/NAME> and then
 restart the domain.
 
+=item B<soft-reset>
+
+cleanup the domain without destroying it, restart the device model. This action
+is supported for HVM guests only.
+
 =back
 
 The default for C<on_poweroff> is C<destroy>.
@@ -386,6 +391,11 @@ Default is C<destroy>.
 
 Action to take if the domain crashes.  Default is C<destroy>.
 
+=item B<on_soft_reset="ACTION">
+
+Action to take if the domain performs 'soft reset' (e.g. does kexec).
+Default is C<soft-reset>.
+
 =back
 
 =head3 Direct Kernel Boot
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index d86ea62..a6743a7 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1492,6 +1492,7 @@ void libxl__domain_destroy(libxl__egc *egc, libxl__domain_destroy_state *dds)
         dds->stubdom.ao = ao;
         dds->stubdom.domid = stubdomid;
         dds->stubdom.callback = stubdom_destroy_callback;
+        dds->stubdom.soft_reset = false;
         libxl__destroy_domid(egc, &dds->stubdom);
     } else {
         dds->stubdom_finished = 1;
@@ -1500,6 +1501,7 @@ void libxl__domain_destroy(libxl__egc *egc, libxl__domain_destroy_state *dds)
     dds->domain.ao = ao;
     dds->domain.domid = dds->domid;
     dds->domain.callback = domain_destroy_callback;
+    dds->domain.soft_reset = dds->soft_reset;
     libxl__destroy_domid(egc, &dds->domain);
 }
 
@@ -1680,10 +1682,13 @@ static void devices_destroy_cb(libxl__egc *egc,
 
     /* Clean up qemu-save and qemu-resume files. They are
      * intermediate files created by libxc. Unfortunately they
-     * don't fit in existing userdata scheme very well.
+     * don't fit in existing userdata scheme very well. In soft reset
+     * case we need to keep the file.
      */
-    rc = libxl__remove_file(gc, libxl__device_model_savefile(gc, domid));
-    if (rc < 0) goto out;
+    if (!dis->soft_reset) {
+        rc = libxl__remove_file(gc, libxl__device_model_savefile(gc, domid));
+        if (rc < 0) goto out;
+    }
     rc = libxl__remove_file(gc,
              GCSPRINTF(XC_DEVICE_MODEL_RESTORE_FILE".%u", domid));
     if (rc < 0) goto out;
@@ -1694,7 +1699,10 @@ static void devices_destroy_cb(libxl__egc *egc,
         ctx->xch = xc_interface_open(ctx->lg,0,0);
         if (!ctx->xch) goto badchild;
 
-        rc = xc_domain_destroy(ctx->xch, domid);
+        if (!dis->soft_reset)
+            rc = xc_domain_destroy(ctx->xch, domid);
+        else
+            rc = xc_domain_soft_reset(ctx->xch, domid);
         if (rc < 0) goto badchild;
         _exit(0);
 
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 0a7913b..2006fd6f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -200,6 +200,13 @@
 #define LIBXL_HAVE_DEVICETREE_PASSTHROUGH 1
 
 /*
+ * LIBXL_HAVE_SOFT_RESET indicates that libxl supports performing 'soft reset'
+ * for domains and there is 'soft_reset' shutdown reason in enum
+ * libxl_shutdown_reason.
+ */
+#define LIBXL_HAVE_SOFT_RESET 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
@@ -1012,6 +1019,11 @@ int static inline libxl_domain_create_restore_0x040200(
 
 #endif
 
+int libxl_domain_soft_reset(libxl_ctx *ctx, libxl_domain_config *d_config,
+                            uint32_t domid, const libxl_asyncop_how *ao_how,
+                            const libxl_asyncprogress_how *aop_console_how)
+                            LIBXL_EXTERNAL_CALLERS_ONLY;
+
   /* A progress report will be made via ao_console_how, of type
    * domain_create_console_available, when the domain's primary
    * console is available and can be connected to.
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 86384d2..7b8dcf5 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -536,8 +536,6 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
     /* convenience aliases */
     libxl_domain_create_info *info = &d_config->c_info;
 
-    assert(!libxl_domid_valid_guest(*domid));
-
     uuid_string = libxl__uuid2string(gc, info->uuid);
     if (!uuid_string) {
         rc = ERROR_NOMEM;
@@ -558,7 +556,6 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
         }
         flags |= XEN_DOMCTL_CDF_hap;
     }
-    *domid = -1;
 
     /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
     libxl_uuid_copy(ctx, (libxl_uuid *)handle, &info->uuid);
@@ -570,13 +567,16 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
         goto out;
     }
 
-    ret = xc_domain_create_config(ctx->xch, info->ssidref,
-                                  handle, flags, domid,
-                                  xc_config);
-    if (ret < 0) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain creation fail");
-        rc = ERROR_FAIL;
-        goto out;
+    /* Valid domid here means we're soft resetting */
+    if (!libxl_domid_valid_guest(*domid)) {
+        ret = xc_domain_create_config(ctx->xch, info->ssidref,
+                                      handle, flags, domid,
+                                      xc_config);
+        if (ret < 0) {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "domain creation fail");
+            rc = ERROR_FAIL;
+            goto out;
+        }
     }
 
     ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
@@ -809,9 +809,8 @@ static void initiate_domain_create(libxl__egc *egc,
     libxl_domain_config *const d_config = dcs->guest_config;
     libxl__domain_build_state *const state = &dcs->build_state;
     const int restore_fd = dcs->restore_fd;
-    memset(&dcs->build_state, 0, sizeof(dcs->build_state));
 
-    domid = 0;
+    domid = dcs->domid_soft_reset;
 
     if (d_config->c_info.ssid_label) {
         char *s = d_config->c_info.ssid_label;
@@ -953,7 +952,7 @@ static void initiate_domain_create(libxl__egc *egc,
             d_config->nics[i].devid = ++last_devid;
     }
 
-    if (restore_fd >= 0) {
+    if (restore_fd >= 0 || dcs->domid_soft_reset != INVALID_DOMID) {
         LOG(DEBUG, "restoring, not running bootloader");
         domcreate_bootloader_done(egc, &dcs->bl, 0);
     } else  {
@@ -1027,7 +1026,7 @@ static void domcreate_bootloader_done(libxl__egc *egc,
     dcs->dmss.dm.callback = domcreate_devmodel_started;
     dcs->dmss.callback = domcreate_devmodel_started;
 
-    if ( restore_fd < 0 ) {
+    if ( restore_fd < 0 && dcs->domid_soft_reset == INVALID_DOMID) {
         rc = libxl__domain_build(gc, d_config, domid, state);
         domcreate_rebuild_done(egc, dcs, rc);
         return;
@@ -1057,9 +1056,11 @@ static void domcreate_bootloader_done(libxl__egc *egc,
         rc = ERROR_INVAL;
         goto out;
     }
-    libxl__xc_domain_restore(egc, dcs,
-                             hvm, pae, superpages);
-    return;
+    if ( restore_fd >= 0 ) {
+        libxl__xc_domain_restore(egc, dcs,
+                                 hvm, pae, superpages);
+        return;
+    }
 
  out:
     libxl__xc_domain_restore_done(egc, dcs, rc, 0, 0);
@@ -1143,8 +1144,12 @@ void libxl__xc_domain_restore_done(libxl__egc *egc, void *dcs_void,
         goto out;
 
     if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
-        state->saved_state = GCSPRINTF(
-                       XC_DEVICE_MODEL_RESTORE_FILE".%d", domid);
+        if (fd != -1)
+            state->saved_state = GCSPRINTF(
+                XC_DEVICE_MODEL_RESTORE_FILE".%d", domid);
+        else
+            state->saved_state = GCSPRINTF(
+                XC_DEVICE_MODEL_SAVE_FILE".%d", domid);
     }
 
 out:
@@ -1153,9 +1158,12 @@ out:
         libxl__file_reference_unmap(&state->pv_ramdisk);
     }
 
-    esave = errno;
-    libxl_fd_set_nonblock(ctx, fd, 0);
-    errno = esave;
+    /* fd == -1 here means we're doing soft reset */
+    if (fd != -1) {
+        esave = errno;
+        libxl_fd_set_nonblock(ctx, fd, 0);
+        errno = esave;
+    }
     domcreate_rebuild_done(egc, dcs, ret);
 }
 
@@ -1572,6 +1580,14 @@ typedef struct {
     uint32_t *domid_out;
 } libxl__app_domain_create_state;
 
+typedef struct {
+    libxl__app_domain_create_state cdcs;
+    libxl__domain_destroy_state dds;
+    libxl__domain_suspend_state dss;
+    uint8_t *toolstack_buf;
+    uint32_t toolstack_len;
+} libxl__domain_soft_reset_state;
+
 static void domain_create_cb(libxl__egc *egc,
                              libxl__domain_create_state *dcs,
                              int rc, uint32_t domid);
@@ -1593,6 +1609,7 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
     cdcs->dcs.restore_fd = restore_fd;
     cdcs->dcs.callback = domain_create_cb;
     cdcs->dcs.checkpointed_stream = checkpointed_stream;
+    cdcs->dcs.domid_soft_reset = INVALID_DOMID;
     libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how);
     cdcs->domid_out = domid;
 
@@ -1601,6 +1618,115 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
     return AO_INPROGRESS;
 }
 
+static void domain_soft_reset_cb(libxl__egc *egc,
+                                 libxl__domain_destroy_state *dds,
+                                 int rc)
+{
+    STATE_AO_GC(dds->ao);
+    libxl__domain_soft_reset_state *srs = CONTAINER_OF(dds, *srs, dds);
+    libxl__app_domain_create_state *cdcs = &srs->cdcs;
+
+    if (rc) {
+        LOG(ERROR, "destruction of domain %u failed", dds->domid);
+        goto error;
+    }
+
+    rc = libxl__toolstack_restore(cdcs->dcs.domid_soft_reset,
+                                  srs->toolstack_buf, srs->toolstack_len,
+                                  &cdcs->dcs.shs);
+    if (rc) {
+        LOG(ERROR, "failed to restore toolstack record");
+        goto error;
+    }
+
+    initiate_domain_create(egc, &cdcs->dcs);
+    return;
+
+error:
+    domcreate_complete(egc, &cdcs->dcs, rc);
+}
+
+static int do_domain_soft_reset(libxl_ctx *ctx, libxl_domain_config *d_config,
+                                uint32_t domid_soft_reset,
+                                const libxl_asyncop_how *ao_how,
+                                const libxl_asyncprogress_how *aop_console_how)
+{
+    AO_CREATE(ctx, 0, ao_how);
+    libxl__domain_soft_reset_state *srs;
+    libxl__app_domain_create_state *cdcs;
+    libxl__domain_create_state *dcs;
+    libxl__domain_build_state *state;
+    libxl__domain_suspend_state *dss;
+    char *dom_path, *xs_store_mfn, *xs_console_mfn;
+    uint32_t domid_out;
+    int rc;
+
+    GCNEW(srs);
+    cdcs = &srs->cdcs;
+    dcs = &cdcs->dcs;
+    state = &dcs->build_state;
+    dss = &srs->dss;
+
+    srs->cdcs.dcs.ao = ao;
+    srs->cdcs.dcs.guest_config = d_config;
+    libxl_domain_config_init(&srs->cdcs.dcs.guest_config_saved);
+    libxl_domain_config_copy(ctx, &srs->cdcs.dcs.guest_config_saved, d_config);
+    cdcs->dcs.restore_fd = -1;
+    cdcs->dcs.domid_soft_reset = domid_soft_reset;
+    cdcs->dcs.callback = domain_create_cb;
+    libxl__ao_progress_gethow(&srs->cdcs.dcs.aop_console_how, aop_console_how);
+    cdcs->domid_out = &domid_out;
+
+    dom_path = libxl__xs_get_dompath(gc, domid_soft_reset);
+    if (!dom_path) {
+        LOG(ERROR, "failed to read domain path");
+        return AO_ABORT(ERROR_FAIL);
+    }
+
+    xs_store_mfn = xs_read(ctx->xsh, XBT_NULL,
+                           GCSPRINTF("%s/store/ring-ref", dom_path), NULL);
+    state->store_mfn = xs_store_mfn ? atol(xs_store_mfn): 0;
+    free(xs_store_mfn);
+
+    xs_console_mfn = xs_read(ctx->xsh, XBT_NULL,
+                             GCSPRINTF("%s/console/ring-ref", dom_path), NULL);
+    state->console_mfn = xs_console_mfn ? atol(xs_console_mfn): 0;
+    free(xs_console_mfn);
+
+    dss->ao = ao;
+    dss->domid = domid_soft_reset;
+    dss->dm_savefile = GCSPRINTF(XC_DEVICE_MODEL_SAVE_FILE".%d",
+                                 domid_soft_reset);
+
+    rc = libxl__toolstack_save(domid_soft_reset, &srs->toolstack_buf,
+                               &srs->toolstack_len, dss);
+    if (rc) {
+        LOG(ERROR, "failed to save toolstack record");
+        return AO_ABORT(ERROR_FAIL);
+    }
+
+    rc = libxl__domain_suspend_device_model(gc, dss);
+    if (rc) {
+        LOG(ERROR, "failed to suspend device model");
+        return AO_ABORT(ERROR_FAIL);
+    }
+
+    /*
+     * On the domain creation path it will be introduced to xenstore with
+     * (probably) different store/console channels so we need to release
+     * it here.
+     */
+    xs_release_domain(ctx->xsh, cdcs->dcs.domid_soft_reset);
+
+    srs->dds.ao = ao;
+    srs->dds.domid = domid_soft_reset;
+    srs->dds.callback = domain_soft_reset_cb;
+    srs->dds.soft_reset = true;
+    libxl__domain_destroy(egc, &srs->dds);
+
+    return AO_INPROGRESS;
+}
+
 static void domain_create_cb(libxl__egc *egc,
                              libxl__domain_create_state *dcs,
                              int rc, uint32_t domid)
@@ -1633,6 +1759,17 @@ int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config,
                             params->checkpointed_stream, ao_how, aop_console_how);
 }
 
+int libxl_domain_soft_reset(libxl_ctx *ctx, libxl_domain_config *d_config,
+                            uint32_t domid, const libxl_asyncop_how *ao_how,
+                            const libxl_asyncprogress_how *aop_console_how)
+{
+    libxl_domain_build_info *const info = &d_config->b_info;
+
+    if (info->type != LIBXL_DOMAIN_TYPE_HVM) return ERROR_INVAL;
+
+    return do_domain_soft_reset(ctx, d_config, domid, ao_how, aop_console_how);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e96d6b5..212e007 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -106,6 +106,7 @@
 #define TAP_DEVICE_SUFFIX "-emu"
 #define DISABLE_UDEV_PATH "libxl/disable_udev"
 #define DOMID_XS_PATH "domid"
+#define INVALID_DOMID ~0
 
 #define ARRAY_SIZE(a) (sizeof(a) / sizeof(a[0]))
 
@@ -3008,6 +3009,7 @@ struct libxl__destroy_domid_state {
     /* private to implementation */
     libxl__devices_remove_state drs;
     libxl__ev_child destroyer;
+    bool soft_reset;
 };
 
 struct libxl__domain_destroy_state {
@@ -3022,6 +3024,7 @@ struct libxl__domain_destroy_state {
     int stubdom_finished;
     libxl__destroy_domid_state domain;
     int domain_finished;
+    bool soft_reset;
 };
 
 /*
@@ -3122,6 +3125,7 @@ struct libxl__domain_create_state {
     libxl_domain_config *guest_config;
     libxl_domain_config guest_config_saved; /* vanilla config */
     int restore_fd;
+    uint32_t domid_soft_reset;
     libxl__domain_create_cb *callback;
     libxl_asyncprogress_how aop_console_how;
     /* private to domain_create */
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9001f65..953d098 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -123,6 +123,8 @@ libxl_action_on_shutdown = Enumeration("action_on_shutdown", [
 
     (5, "COREDUMP_DESTROY"),
     (6, "COREDUMP_RESTART"),
+
+    (7, "SOFT_RESET"),
     ], init_val = "LIBXL_ACTION_ON_SHUTDOWN_DESTROY")
 
 libxl_trigger = Enumeration("trigger", [
@@ -584,6 +586,7 @@ libxl_domain_config = Struct("domain_config", [
     ("on_reboot", libxl_action_on_shutdown),
     ("on_watchdog", libxl_action_on_shutdown),
     ("on_crash", libxl_action_on_shutdown),
+    ("on_soft_reset", libxl_action_on_shutdown),
     ], dir=DIR_IN)
 
 libxl_diskinfo = Struct("diskinfo", [
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 374680c..5771b4c 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -190,6 +190,7 @@ typedef enum {
     DOMAIN_RESTART_NONE = 0,     /* No domain restart */
     DOMAIN_RESTART_NORMAL,       /* Domain should be restarted */
     DOMAIN_RESTART_RENAME,       /* Domain should be renamed and restarted */
+    DOMAIN_RESTART_SOFT_RESET,   /* Soft reset should be performed */
 } domain_restart_type;
 
 extern void printf_info_sexp(int domid, libxl_domain_config *d_config, FILE *fh);
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 32217fa..0c25a4e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -130,6 +130,8 @@ static const char *action_on_shutdown_names[] = {
 
     [LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY] = "coredump-destroy",
     [LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_RESTART] = "coredump-restart",
+
+    [LIBXL_ACTION_ON_SHUTDOWN_SOFT_RESET] = "soft-reset",
 };
 
 /* Optional data, in order:
@@ -1329,6 +1331,13 @@ static void parse_config_data(const char *config_source,
         exit(1);
     }
 
+    if (xlu_cfg_get_string (config, "on_soft_reset", &buf, 0))
+        buf = "soft-reset";
+    if (!parse_action_on_shutdown(buf, &d_config->on_soft_reset)) {
+        fprintf(stderr, "Unknown on_soft_reset action \"%s\" specified\n", buf);
+        exit(1);
+    }
+
     /* libxl_get_required_shadow_memory() must be called after final values
      * (default or specified) for vcpus and memory are set, because the
      * calculation depends on those values. */
@@ -2307,6 +2316,9 @@ static domain_restart_type handle_domain_death(uint32_t *r_domid,
     case LIBXL_SHUTDOWN_REASON_WATCHDOG:
         action = d_config->on_watchdog;
         break;
+    case LIBXL_SHUTDOWN_REASON_SOFT_RESET:
+        action = d_config->on_soft_reset;
+        break;
     default:
         LOG("Unknown shutdown reason code %d. Destroying domain.",
             event->u.domain_shutdown.shutdown_reason);
@@ -2357,6 +2369,11 @@ static domain_restart_type handle_domain_death(uint32_t *r_domid,
         *r_domid = INVALID_DOMID;
         break;
 
+    case LIBXL_ACTION_ON_SHUTDOWN_SOFT_RESET:
+        reload_domain_config(*r_domid, d_config);
+        restart = DOMAIN_RESTART_SOFT_RESET;
+        break;
+
     case LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_DESTROY:
     case LIBXL_ACTION_ON_SHUTDOWN_COREDUMP_RESTART:
         /* Already handled these above. */
@@ -2532,6 +2549,7 @@ static uint32_t create_domain(struct domain_create *dom_info)
     int restore_fd = -1;
     const libxl_asyncprogress_how *autoconnect_console_how;
     struct save_file_header hdr;
+    uint32_t domid_soft_reset = INVALID_DOMID;
 
     int restoring = (restore_file || (migrate_fd >= 0));
 
@@ -2732,7 +2750,13 @@ start:
          * restore/migrate-receive it again.
          */
         restoring = 0;
-    }else{
+    } else if ( domid_soft_reset != INVALID_DOMID ) {
+        /* Do soft reset */
+        ret = libxl_domain_soft_reset(ctx, &d_config, domid_soft_reset,
+                                      0, autoconnect_console_how);
+        domid = domid_soft_reset;
+        domid_soft_reset = INVALID_DOMID;
+    } else {
         ret = libxl_domain_create_new(ctx, &d_config, &domid,
                                       0, autoconnect_console_how);
     }
@@ -2796,8 +2820,13 @@ start:
                 event->u.domain_shutdown.shutdown_reason,
                 event->u.domain_shutdown.shutdown_reason);
             switch (handle_domain_death(&domid, event, &d_config)) {
+            case DOMAIN_RESTART_SOFT_RESET:
+                domid_soft_reset = domid;
+                domid = INVALID_DOMID;
+                /* fall through */
             case DOMAIN_RESTART_RENAME:
-                if (!preserve_domain(&domid, event, &d_config)) {
+                if (domid_soft_reset == INVALID_DOMID &&
+                    !preserve_domain(&domid, event, &d_config)) {
                     /* If we fail then exit leaving the old domain in place. */
                     ret = -1;
                     goto out;
-- 
2.4.2

Re: [PATCH v8 11/11] (lib)xl: soft reset support

[Konrad Rzeszutek Wilk]

On Tue, Jun 23, 2015 at 06:11:53PM +0200, Vitaly Kuznetsov wrote:
> Use existing create/restore path to perform 'soft reset' for HVM domains.
> Tear everything down, e.g. destroy domain's device model, remove the domain
> from xenstore, save toolstack record and start over.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> Changes since 'v7'
> - 'Reset everything approach': XEN_DOMCTL_soft_reset doesn't destroy the
>   original domain any more.
> - libxl__domain_soft_reset_state introduced [Ian Campbell]
> - Separate do_domain_soft_reset to not intertwine with do_domain_create
>   [Ian Campbell]
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  docs/man/xl.cfg.pod.5        |  10 +++
>  tools/libxl/libxl.c          |  16 +++-
>  tools/libxl/libxl.h          |  12 +++
>  tools/libxl/libxl_create.c   | 181 +++++++++++++++++++++++++++++++++++++------
>  tools/libxl/libxl_internal.h |   4 +
>  tools/libxl/libxl_types.idl  |   3 +
>  tools/libxl/xl.h             |   1 +
>  tools/libxl/xl_cmdimpl.c     |  33 +++++++-
>  8 files changed, 232 insertions(+), 28 deletions(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a3e0e2e..27f4c2a 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -368,6 +368,11 @@ destroy the domain.
>  write a "coredump" of the domain to F</var/lib/xen/dump/NAME> and then
>  restart the domain.
>  
> +=item B<soft-reset>
> +
> +cleanup the domain without destroying it, restart the device model. This action
> +is supported for HVM guests only.
> +
>  =back
>  
>  The default for C<on_poweroff> is C<destroy>.
> @@ -386,6 +391,11 @@ Default is C<destroy>.
>  
>  Action to take if the domain crashes.  Default is C<destroy>.
>  
> +=item B<on_soft_reset="ACTION">
> +
> +Action to take if the domain performs 'soft reset' (e.g. does kexec).
> +Default is C<soft-reset>.
> +
>  =back
>  
>  =head3 Direct Kernel Boot
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index d86ea62..a6743a7 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1492,6 +1492,7 @@ void libxl__domain_destroy(libxl__egc *egc, libxl__domain_destroy_state *dds)
>          dds->stubdom.ao = ao;
>          dds->stubdom.domid = stubdomid;
>          dds->stubdom.callback = stubdom_destroy_callback;
> +        dds->stubdom.soft_reset = false;
>          libxl__destroy_domid(egc, &dds->stubdom);
>      } else {
>          dds->stubdom_finished = 1;
> @@ -1500,6 +1501,7 @@ void libxl__domain_destroy(libxl__egc *egc, libxl__domain_destroy_state *dds)
>      dds->domain.ao = ao;
>      dds->domain.domid = dds->domid;
>      dds->domain.callback = domain_destroy_callback;
> +    dds->domain.soft_reset = dds->soft_reset;
>      libxl__destroy_domid(egc, &dds->domain);
>  }
>  
> @@ -1680,10 +1682,13 @@ static void devices_destroy_cb(libxl__egc *egc,
>  
>      /* Clean up qemu-save and qemu-resume files. They are
>       * intermediate files created by libxc. Unfortunately they
> -     * don't fit in existing userdata scheme very well.
> +     * don't fit in existing userdata scheme very well. In soft reset
> +     * case we need to keep the file.
>       */
> -    rc = libxl__remove_file(gc, libxl__device_model_savefile(gc, domid));
> -    if (rc < 0) goto out;
> +    if (!dis->soft_reset) {
> +        rc = libxl__remove_file(gc, libxl__device_model_savefile(gc, domid));
> +        if (rc < 0) goto out;
> +    }
>      rc = libxl__remove_file(gc,
>               GCSPRINTF(XC_DEVICE_MODEL_RESTORE_FILE".%u", domid));
>      if (rc < 0) goto out;
> @@ -1694,7 +1699,10 @@ static void devices_destroy_cb(libxl__egc *egc,
>          ctx->xch = xc_interface_open(ctx->lg,0,0);
>          if (!ctx->xch) goto badchild;
>  
> -        rc = xc_domain_destroy(ctx->xch, domid);
> +        if (!dis->soft_reset)
> +            rc = xc_domain_destroy(ctx->xch, domid);
> +        else

I would also pause the guest before doing this.

I didn't look at the rest very much, sorry to say.

Re: [PATCH v8 11/11] (lib)xl: soft reset support

[Ian Jackson]

Vitaly Kuznetsov writes ("[PATCH v8 11/11] (lib)xl: soft reset support"):
> Use existing create/restore path to perform 'soft reset' for HVM domains.
> Tear everything down, e.g. destroy domain's device model, remove the domain
> from xenstore, save toolstack record and start over.

This patch has a number of long lines (eg in the documentation and
comments) which make it hard to review.  Can you please keep it to 70
columns, or 75 if you absolutely must ?

I'm not sure that this descriptiion:

> +=item B<soft-reset>
> +
> +cleanup the domain without destroying it, restart the device
> +model. This action is supported for HVM guests only.

is really accurate from a user point of view.

Ian.

Re: [PATCH v8 11/11] (lib)xl: soft reset support

[Vitaly Kuznetsov]

Ian Jackson <Ian.Jackson@eu.citrix.com> writes:

> Vitaly Kuznetsov writes ("[PATCH v8 11/11] (lib)xl: soft reset support"):
>> Use existing create/restore path to perform 'soft reset' for HVM domains.
>> Tear everything down, e.g. destroy domain's device model, remove the domain
>> from xenstore, save toolstack record and start over.
>
> This patch has a number of long lines (eg in the documentation and
> comments) which make it hard to review.  Can you please keep it to 70
> columns, or 75 if you absolutely must ?

No problem, will do in v9. BTW, libxl/CODING_STYLE states that 'Lines
are limited to 75-80 characters'. I'd suggest we update that in case
70-75 is preferred.

>
> I'm not sure that this descriptiion:
>
>> +=item B<soft-reset>
>> +
>> +cleanup the domain without destroying it, restart the device
>> +model. This action is supported for HVM guests only.
>
> is really accurate from a user point of view.

Yea, I'm trying hard to avoid mentioning Linux and kexec while
describing soft reset. Will try to come up with something..


>
> Ian.

-- 
  Vitaly