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

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Sep 29, 2015 at 08:40:23AM +0800, Haozhong Zhang wrote:
> On Mon, Sep 28, 2015 at 03:19:25PM +0100, Wei Liu wrote:
> > On Mon, Sep 28, 2015 at 03:13:58PM +0800, 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 field should be in arch-specific substructure, i.e. "hvm".
> >
> 
> Julien also pointed out this and suggested to moving to an
> arch-specific substructure. I'm going to add a new substructure
> "arch_x86" and move "vtsc_khz" there. Is this good for you?
> 

My initial thought was that this was a feature of HVM. I don't recollect
why I got that idea. I could be wrong.  Does it work with (or intent to
work with) PV too?  If yes, adding it to arch_x86 would be appropriate.
If not, hvm specific is good enough.  Please correct my
misunderstanding, I'm definitely no expert on x86.  

> > >      ("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))
> > > +        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");
> > > +        }
> > 
> > And this hunk (the decision making bit) should be in libxl, not xl.
> > 
> > Consider there are other toolstack that uses libxl, say libvirt.
> >
> 
> Good to know this.
> 
> I'm going to move it to libxl__arch_domain_create() where
> b_info->vtsc_khz is used.
> 

Right, that seems appropriate.

Wei.