Xen devel - Nov 2011 - [PATCH] libxl: use named options for tsc_mode

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1320922479 0
# Node ID bc79b560aafa1e4dc42af00e6a326dc651b5636a
# Parent  460b507e15f864dd6712f5040e36538d6e076ae4
libxl: use named options for tsc_mode.

It seems that this knob is expoerted from the hypervisor as a raw
integer (no symbolic names) documented in xen/include/asm-x86. Propagating that
all the way to the end user is hardly friendly (it''s bad enough in the
hypercall interface).

Add an enum at the libxl level with a hopefully descriptive set of names.
Deprecate the use of an integer in xl cfg files.

Signed-off-by: Ian Campbell

diff -r 460b507e15f8 -r bc79b560aafa docs/user/xl-domain-config.markdown
--- a/docs/user/xl-domain-config.markdown	Thu Nov 10 10:18:29 2011 +0000
+++ b/docs/user/xl-domain-config.markdown	Thu Nov 10 10:54:39 2011 +0000
@@ -1,4 +1,4 @@
- # xl Domain Configuration
+# xl Domain Configuration
 
 To create a VM (a domain in Xen terminology, sometimes called a guest)
 with xl requires the provision of a domain config file.  Typically
@@ -338,6 +338,29 @@ accept the defaults for these options wh
    extensions (e.g. Windows XP compatibility mode on more modern
    Windows OS).
 
+### Guest Virtual Time Controls
+
+ * `tsc_mode="MODE"`: Specifies how the TSC (Time Stamp Counter)
+   should be provided to the guest (X86 only). Specifying this option as a
number
+   is deprecated. Options are:
+
+   * `"default"`: guest rdtsc/p executed natively when monotonicity
+     can be guaranteed and emulated otherwise (with frequency scaled
+     if necessary).
+
+   * `"always_emulate"`: guest rdtsc/p always emulated at 1GHz
(kernel
+     and user).
+
+   * `"native"`: guest rdtsc always executed natively (no
+     monotonicity/frequency guarantees); guest rdtscp emulated at
+     native frequency if unsupported by h/w, else executed natively.
+
+   * `"native_paravirt"`: same as `native`, except xen manages
TSC_AUX
+      register so guest can determine when a restore/migration has
+      occurred and assumes guest obtains/uses pvclock-like mechanism
+      to adjust for monotonicity and frequency changes.
+
+
 ### Support for Paravirtualisation of HVM Guests
 
 The following options allow Paravirtualised features (such as devices)
@@ -546,9 +569,6 @@ certainly belong in a more appropriate s
    enables certain other features which are incompatible with
    migration (currently certain TSC modes XXX ?).
 
- * `tsc_mode=VALUE`: Specifies how the TSC (Time Stamp Counter) should
-   be provided to the guest.  XXX ???
-
  * `pci_msitranslate=BOOLEAN`: XXX
 
  * `pci_power_mgmt=BOOLEAN`: XXX
diff -r 460b507e15f8 -r bc79b560aafa tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Thu Nov 10 10:18:29 2011 +0000
+++ b/tools/libxl/libxl_dom.c	Thu Nov 10 10:54:39 2011 +0000
@@ -73,12 +73,29 @@ int libxl__build_pre(libxl__gc *gc, uint
               libxl_domain_build_info *info, libxl__domain_build_state *state)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
+    int tsc_mode;
     xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
     xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
LIBXL_MAXMEM_CONSTANT);
     if (info->type == LIBXL_DOMAIN_TYPE_PV)
         xc_domain_set_memmap_limit(ctx->xch, domid,
                 (info->max_memkb + info->u.pv.slack_memkb));
-    xc_domain_set_tsc_info(ctx->xch, domid, info->tsc_mode, 0, 0, 0);
+    switch (info->tsc_mode) {
+    case LIBXL_TSC_MODE_DEFAULT:
+        tsc_mode = 0;
+        break;
+    case LIBXL_TSC_MODE_ALWAYS_EMULATE:
+        tsc_mode = 1;
+        break;
+    case LIBXL_TSC_MODE_NATIVE:
+        tsc_mode = 2;
+        break;
+    case LIBXL_TSC_MODE_NATIVE_PARAVIRT:
+        tsc_mode = 3;
+        break;
+    default:
+        abort();
+    }
+    xc_domain_set_tsc_info(ctx->xch, domid, tsc_mode, 0, 0, 0);
     if ( info->disable_migrate )
         xc_domain_disable_migrate(ctx->xch, domid);
 
diff -r 460b507e15f8 -r bc79b560aafa tools/libxl/libxl_types.idl
--- a/tools/libxl/libxl_types.idl	Thu Nov 10 10:18:29 2011 +0000
+++ b/tools/libxl/libxl_types.idl	Thu Nov 10 10:54:39 2011 +0000
@@ -85,6 +85,13 @@ libxl_button = Enumeration("button", [
     (2, "SLEEP"),
     ])
 
+libxl_tsc_mode = Enumeration("tsc_mode", [
+    (0, "default"),
+    (1, "always_emulate"),
+    (2, "native"),
+    (3, "native_paravirt"),
+    ])
+
 #
 # Complex libxl types
 #
@@ -154,7 +161,7 @@ libxl_domain_create_info = Struct("domai
 libxl_domain_build_info = Struct("domain_build_info",[
     ("max_vcpus",       integer),
     ("cur_vcpus",       integer),
-    ("tsc_mode",        integer),
+    ("tsc_mode",        libxl_tsc_mode),
     ("max_memkb",       uint32),
     ("target_memkb",    uint32),
     ("video_memkb",     uint32),
diff -r 460b507e15f8 -r bc79b560aafa tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Thu Nov 10 10:18:29 2011 +0000
+++ b/tools/libxl/xl_cmdimpl.c	Thu Nov 10 10:54:39 2011 +0000
@@ -328,7 +328,7 @@ static void printf_info(int domid,
 
     printf("\t(build_info)\n");
     printf("\t(max_vcpus %d)\n", b_info->max_vcpus);
-    printf("\t(tsc_mode %d)\n", b_info->tsc_mode);
+    printf("\t(tsc_mode %s)\n",
libxl_tsc_mode_to_string(b_info->tsc_mode));
     printf("\t(max_memkb %d)\n", b_info->max_memkb);
     printf("\t(target_memkb %d)\n", b_info->target_memkb);
     printf("\t(nomigrate %d)\n", b_info->disable_migrate);
@@ -662,8 +662,23 @@ static void parse_config_data(const char
     if (!xlu_cfg_get_long (config, "nomigrate", &l, 0))
         b_info->disable_migrate = l;
 
-    if (!xlu_cfg_get_long(config, "tsc_mode", &l, 0))
+    if (!xlu_cfg_get_long(config, "tsc_mode", &l, 1)) {
+        fprintf(stderr, "WARNING: specifying \"tsc_mode\" as an
integer is deprecated. "
+                "Please use the named parameter variant.\n");
+        if (l < LIBXL_TSC_MODE_DEFAULT ||
+            l > LIBXL_TSC_MODE_NATIVE_PARAVIRT) {
+            fprintf(stderr, "ERROR: invalid value %ld for
\"tsc_mode\"\n", l);
+            exit (1);
+        }
         b_info->tsc_mode = l;
+    } else if (!xlu_cfg_get_string(config, "tsc_mode", &buf, 0))
{
+        fprintf(stderr, "got a tsc mode string: \"%s\"\n",
buf);
+        if (libxl_tsc_mode_from_string(buf, &b_info->tsc_mode)) {
+            fprintf(stderr, "ERROR: invalid value \"%s\" for
\"tsc_mode\"\n",
+                    buf);
+            exit (1);
+        }
+    }
 
     if (!xlu_cfg_get_long (config, "videoram", &l, 0))
         b_info->video_memkb = l * 1024;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2011-11-10 at 10:55 +0000, Ian Campbell wrote:
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1320922479 0
> # Node ID bc79b560aafa1e4dc42af00e6a326dc651b5636a
> # Parent  460b507e15f864dd6712f5040e36538d6e076ae4
> libxl: use named options for tsc_mode.
> 
> It seems that this knob is expoerted from the hypervisor as a raw
> integer (no symbolic names) documented in xen/include/asm-x86. Propagating
that
> all the way to the end user is hardly friendly (it''s bad enough in
the
> hypercall interface).
I''ve just seen docs/misc/tscmode.txt which is actually a pretty
comprehensive explanation of what''s going on so the above is
unwarranted. I''ll update this patch to include a reference in the
xl-domain-config.
> 
> Add an enum at the libxl level with a hopefully descriptive set of names.
> Deprecate the use of an integer in xl cfg files.
I think the naming at the libxl level is still useful even if the values
are documented.

Ian.
> 
> Signed-off-by: Ian Campbell
> 
> diff -r 460b507e15f8 -r bc79b560aafa docs/user/xl-domain-config.markdown
> --- a/docs/user/xl-domain-config.markdown	Thu Nov 10 10:18:29 2011 +0000
> +++ b/docs/user/xl-domain-config.markdown	Thu Nov 10 10:54:39 2011 +0000
> @@ -1,4 +1,4 @@
> - # xl Domain Configuration
> +# xl Domain Configuration
>  
>  To create a VM (a domain in Xen terminology, sometimes called a guest)
>  with xl requires the provision of a domain config file.  Typically
> @@ -338,6 +338,29 @@ accept the defaults for these options wh
>     extensions (e.g. Windows XP compatibility mode on more modern
>     Windows OS).
>  
> +### Guest Virtual Time Controls
> +
> + * `tsc_mode="MODE"`: Specifies how the TSC (Time Stamp Counter)
> +   should be provided to the guest (X86 only). Specifying this option as a
number
> +   is deprecated. Options are:
> +
> +   * `"default"`: guest rdtsc/p executed natively when
monotonicity
> +     can be guaranteed and emulated otherwise (with frequency scaled
> +     if necessary).
> +
> +   * `"always_emulate"`: guest rdtsc/p always emulated at 1GHz
(kernel
> +     and user).
> +
> +   * `"native"`: guest rdtsc always executed natively (no
> +     monotonicity/frequency guarantees); guest rdtscp emulated at
> +     native frequency if unsupported by h/w, else executed natively.
> +
> +   * `"native_paravirt"`: same as `native`, except xen manages
TSC_AUX
> +      register so guest can determine when a restore/migration has
> +      occurred and assumes guest obtains/uses pvclock-like mechanism
> +      to adjust for monotonicity and frequency changes.
> +
> +
>  ### Support for Paravirtualisation of HVM Guests
>  
>  The following options allow Paravirtualised features (such as devices)
> @@ -546,9 +569,6 @@ certainly belong in a more appropriate s
>     enables certain other features which are incompatible with
>     migration (currently certain TSC modes XXX ?).
>  
> - * `tsc_mode=VALUE`: Specifies how the TSC (Time Stamp Counter) should
> -   be provided to the guest.  XXX ???
> -
>   * `pci_msitranslate=BOOLEAN`: XXX
>  
>   * `pci_power_mgmt=BOOLEAN`: XXX
> diff -r 460b507e15f8 -r bc79b560aafa tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c	Thu Nov 10 10:18:29 2011 +0000
> +++ b/tools/libxl/libxl_dom.c	Thu Nov 10 10:54:39 2011 +0000
> @@ -73,12 +73,29 @@ int libxl__build_pre(libxl__gc *gc, uint
>                libxl_domain_build_info *info, libxl__domain_build_state
*state)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
> +    int tsc_mode;
>      xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus);
>      xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
LIBXL_MAXMEM_CONSTANT);
>      if (info->type == LIBXL_DOMAIN_TYPE_PV)
>          xc_domain_set_memmap_limit(ctx->xch, domid,
>                  (info->max_memkb + info->u.pv.slack_memkb));
> -    xc_domain_set_tsc_info(ctx->xch, domid, info->tsc_mode, 0, 0,
0);
> +    switch (info->tsc_mode) {
> +    case LIBXL_TSC_MODE_DEFAULT:
> +        tsc_mode = 0;
> +        break;
> +    case LIBXL_TSC_MODE_ALWAYS_EMULATE:
> +        tsc_mode = 1;
> +        break;
> +    case LIBXL_TSC_MODE_NATIVE:
> +        tsc_mode = 2;
> +        break;
> +    case LIBXL_TSC_MODE_NATIVE_PARAVIRT:
> +        tsc_mode = 3;
> +        break;
> +    default:
> +        abort();
> +    }
> +    xc_domain_set_tsc_info(ctx->xch, domid, tsc_mode, 0, 0, 0);
>      if ( info->disable_migrate )
>          xc_domain_disable_migrate(ctx->xch, domid);
>  
> diff -r 460b507e15f8 -r bc79b560aafa tools/libxl/libxl_types.idl
> --- a/tools/libxl/libxl_types.idl	Thu Nov 10 10:18:29 2011 +0000
> +++ b/tools/libxl/libxl_types.idl	Thu Nov 10 10:54:39 2011 +0000
> @@ -85,6 +85,13 @@ libxl_button = Enumeration("button", [
>      (2, "SLEEP"),
>      ])
>  
> +libxl_tsc_mode = Enumeration("tsc_mode", [
> +    (0, "default"),
> +    (1, "always_emulate"),
> +    (2, "native"),
> +    (3, "native_paravirt"),
> +    ])
> +
>  #
>  # Complex libxl types
>  #
> @@ -154,7 +161,7 @@ libxl_domain_create_info = Struct("domai
>  libxl_domain_build_info = Struct("domain_build_info",[
>      ("max_vcpus",       integer),
>      ("cur_vcpus",       integer),
> -    ("tsc_mode",        integer),
> +    ("tsc_mode",        libxl_tsc_mode),
>      ("max_memkb",       uint32),
>      ("target_memkb",    uint32),
>      ("video_memkb",     uint32),
> diff -r 460b507e15f8 -r bc79b560aafa tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Thu Nov 10 10:18:29 2011 +0000
> +++ b/tools/libxl/xl_cmdimpl.c	Thu Nov 10 10:54:39 2011 +0000
> @@ -328,7 +328,7 @@ static void printf_info(int domid,
>  
>      printf("\t(build_info)\n");
>      printf("\t(max_vcpus %d)\n", b_info->max_vcpus);
> -    printf("\t(tsc_mode %d)\n", b_info->tsc_mode);
> +    printf("\t(tsc_mode %s)\n",
libxl_tsc_mode_to_string(b_info->tsc_mode));
>      printf("\t(max_memkb %d)\n", b_info->max_memkb);
>      printf("\t(target_memkb %d)\n", b_info->target_memkb);
>      printf("\t(nomigrate %d)\n", b_info->disable_migrate);
> @@ -662,8 +662,23 @@ static void parse_config_data(const char
>      if (!xlu_cfg_get_long (config, "nomigrate", &l, 0))
>          b_info->disable_migrate = l;
>  
> -    if (!xlu_cfg_get_long(config, "tsc_mode", &l, 0))
> +    if (!xlu_cfg_get_long(config, "tsc_mode", &l, 1)) {
> +        fprintf(stderr, "WARNING: specifying \"tsc_mode\"
as an integer is deprecated. "
> +                "Please use the named parameter variant.\n");
> +        if (l < LIBXL_TSC_MODE_DEFAULT ||
> +            l > LIBXL_TSC_MODE_NATIVE_PARAVIRT) {
> +            fprintf(stderr, "ERROR: invalid value %ld for
\"tsc_mode\"\n", l);
> +            exit (1);
> +        }
>          b_info->tsc_mode = l;
> +    } else if (!xlu_cfg_get_string(config, "tsc_mode", &buf,
0)) {
> +        fprintf(stderr, "got a tsc mode string:
\"%s\"\n", buf);
> +        if (libxl_tsc_mode_from_string(buf, &b_info->tsc_mode)) {
> +            fprintf(stderr, "ERROR: invalid value \"%s\"
for \"tsc_mode\"\n",
> +                    buf);
> +            exit (1);
> +        }
> +    }
>  
>      if (!xlu_cfg_get_long (config, "videoram", &l, 0))
>          b_info->video_memkb = l * 1024;
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Subject: [Xen-devel] [PATCH] libxl: use named options for tsc_mode
> 
> # HG changeset patch
> # User Ian Campbell <ian.campbell@citrix.com>
> # Date 1320922479 0
> # Node ID bc79b560aafa1e4dc42af00e6a326dc651b5636a
> # Parent  460b507e15f864dd6712f5040e36538d6e076ae4
> libxl: use named options for tsc_mode.
> 
> It seems that this knob is expoerted from the hypervisor as a raw
> integer (no symbolic names) documented in xen/include/asm-x86. Propagating
that
> all the way to the end user is hardly friendly (it''s bad enough in
the
> hypercall interface).
Thanks for looking at this!
> Add an enum at the libxl level with a hopefully descriptive set of names.
> Deprecate the use of an integer in xl cfg files.
We (Oracle) already have shipped cfg files that use the integer
so would prefer that the deprecation message be removed.  IMHO, to
the vast majority of users, the symbolic names will be gibberish
anyway and are likely to be misspelled.  For knowledgeable folk,
they are nice though, so maybe just support both?
 
> +   * `"always_emulate"`: guest rdtsc/p always emulated at 1GHz
(kernel
> +     and user).
Many guests will "appear" to run at a slower "bogomips" so a
word
or two more here may keep some from panicking.  Maybe:

guest rdtsc/p always emulated and the virtual TSC will appear to
increment (kernel and user) at a fixed 1GHz rate, regardless of
the PCPU HZ rate or power state; this will NOT affect underlying
CPU performance

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, 2011-11-10 at 17:29 +0000, Dan Magenheimer
wrote:
> > From: Ian Campbell [mailto:ian.campbell@citrix.com]
> > Subject: [Xen-devel] [PATCH] libxl: use named options for tsc_mode
> > 
> > # HG changeset patch
> > # User Ian Campbell <ian.campbell@citrix.com>
> > # Date 1320922479 0
> > # Node ID bc79b560aafa1e4dc42af00e6a326dc651b5636a
> > # Parent  460b507e15f864dd6712f5040e36538d6e076ae4
> > libxl: use named options for tsc_mode.
> > 
> > It seems that this knob is expoerted from the hypervisor as a raw
> > integer (no symbolic names) documented in xen/include/asm-x86.
Propagating that
> > all the way to the end user is hardly friendly (it''s bad
enough in the
> > hypercall interface).
> 
> Thanks for looking at this!
> 
> > Add an enum at the libxl level with a hopefully descriptive set of
names.
> > Deprecate the use of an integer in xl cfg files.
> 
> We (Oracle) already have shipped cfg files that use the integer
> so would prefer that the deprecation message be removed.  IMHO, to
> the vast majority of users, the symbolic names will be gibberish
> anyway and are likely to be misspelled.  For knowledgeable folk,
> they are nice though, so maybe just support both?
For my part I can never remember which number is which so I always have
to go look, whereas "always_emulate" etc is generally descriptive of
what I''m looking for. IMHO magic numbers are generally a pretty
terrible
interface.

I could change the message to inform the user which specific option
corresponds to the number which they used. Would that help?

More generally we need to have mechanisms by which we can evolve the
functionality provided by xl, guiding users towards new and improved
options for what they are trying to achieve while allowing us to
eventually remove deprecated options and therefore combat the inevitable
growth of compatibility cruft. I don''t really mind what that mechanism
is but a message explaining what the new option is seems like a simple
solution. Perhaps it could be dropped to "INFO" rather than
"WARNING"?
>  
> > +   * `"always_emulate"`: guest rdtsc/p always emulated at
1GHz (kernel
> > +     and user).
> 
> Many guests will "appear" to run at a slower "bogomips"
so a word
> or two more here may keep some from panicking.  Maybe:
> 
> guest rdtsc/p always emulated and the virtual TSC will appear to
> increment (kernel and user) at a fixed 1GHz rate, regardless of
> the PCPU HZ rate or power state; this will NOT affect underlying
> CPU performance
Presumably the actual act of emulation has a performance impact?

How about:

guest rdtsc/p always emulated and the virtual TSC will appear to
increment (kernel and user) at a fixed 1GHz rate, regardless of the PCPU
HZ rate or power state; Although there is an overhead associated with
emulation this will NOT affect underlying CPU performance.

?

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel