Re: [PATCH 13/13] tools/libxl: Add 'vtsc_khz' option to set guest TSC rate

[Julien Grall <julien.grall@citrix.com>]

Hi,

On 28/09/15 08:13, Haozhong Zhang wrote:
> This patch adds an option 'vtsc_khz' to allow users to set vcpu's TSC
> rate in KHz. In the case that tsc_mode = 'default', the default value of
> 'vtsc_khz' option is the host TSC rate which is used when 'vtsc_khz'
> option is set to 0 or does not appear in the configuration. In all other
> cases of tsc_mode, 'vtsc_khz' option is just ignored.
> 
> Another purpose of adding this option is to keep vcpu's TSC rate across
> guest reboot. In existing code, a new domain is created from the
> configuration of the previous domain which was just rebooted. vcpu's TSC
> rate is not stored in the configuration and the host TSC rate is the
> used as vcpu's TSC rate. This works fine unless the previous domain was
> migrated from another host machine with a different host TSC rate than
> the current one.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/libxl_x86.c     |  4 +++-
>  tools/libxl/xl_cmdimpl.c    | 22 ++++++++++++++++++++++
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 9f6ec00..91cb0be 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -413,6 +413,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("vcpu_soft_affinity", Array(libxl_bitmap, "num_vcpu_soft_affinity")),
>      ("numa_placement",  libxl_defbool),
>      ("tsc_mode",        libxl_tsc_mode),
> +    ("vtsc_khz",        uint32),

This is x86 specific, can we begin to move anything arch-specific under
arch_foo? See arch_arm for instance.

Also, you would need to add a new define LIBXL_HAVE_foo to let allow
developer writing app on top of libxl support multiple version of Xen.
See libxl.h

>      ("max_memkb",       MemKB),
>      ("target_memkb",    MemKB),
>      ("video_memkb",     MemKB),
> diff --git a/tools/libxl/libxl_x86.c b/tools/libxl/libxl_x86.c
> index 896f34c..7baaee4 100644
> --- a/tools/libxl/libxl_x86.c
> +++ b/tools/libxl/libxl_x86.c
> @@ -276,6 +276,7 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
>  {
>      int ret = 0;
>      int tsc_mode;
> +    uint32_t vtsc_khz;
>      uint32_t rtc_timeoffset;
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>  
> @@ -300,7 +301,8 @@ int libxl__arch_domain_create(libxl__gc *gc, libxl_domain_config *d_config,
>      default:
>          abort();
>      }
> -    xc_domain_set_tsc_info(ctx->xch, domid, tsc_mode, 0, 0, 0);
> +    vtsc_khz = d_config->b_info.vtsc_khz;
> +    xc_domain_set_tsc_info(ctx->xch, domid, tsc_mode, 0, vtsc_khz, 0);
>      if (libxl_defbool_val(d_config->b_info.disable_migrate))
>          xc_domain_disable_migrate(ctx->xch, domid);
>      rtc_timeoffset = d_config->b_info.rtc_timeoffset;
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2706759..5fabda7 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1462,6 +1462,28 @@ static void parse_config_data(const char *config_source,
>          }
>      }
>  
> +    /* "vtsc_khz" option works only if "tsc_mode" option is
> +     * "default". In this case, if "vtsc_khz" option is set to 0, we
> +     * will reset it to the host TSC rate. In all other cases, we just
> +     * ignore any given value and always set it to 0.
> +     */
> +    if (!xlu_cfg_get_long(config, "vtsc_khz", &l, 0))

This option has to be documented docs/man/xl.cfg.pod.5.

> +        b_info->vtsc_khz = l;
> +    if (b_info->tsc_mode == LIBXL_TSC_MODE_DEFAULT) {
> +        if (b_info->vtsc_khz == 0) {
> +            libxl_physinfo physinfo;
> +            if (!libxl_get_physinfo(ctx, &physinfo))
> +                b_info->vtsc_khz = physinfo.cpu_khz;
> +            else
> +                fprintf(stderr, "WARNING: cannot get host TSC rate.\n");
> +        }
> +    } else {
> +        fprintf(stderr, "WARNING: ignoring \"vtsc_khz\" option. "
> +                "\"vtsc_khz\" option works only if "
> +                "\"tsc_mode\" option is \"default\".\n");
> +        b_info->vtsc_khz = 0;
> +    }
> +
>      if (!xlu_cfg_get_long(config, "rtc_timeoffset", &l, 0))
>          b_info->rtc_timeoffset = l;
>  
> 

Regards,


-- 
Julien Grall