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 11:07:17AM +0100, Ian Campbell wrote:
> On Tue, 2015-09-29 at 08:40 +0800, Haozhong Zhang wrote:
> > > > @@ -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.
> 
> libxl__domain_build_info_setdefault would be more usual, I think.
> 

For the benefit of avoiding conflict suggestions, I think Ian is right.
I retract my previous comment.

> Rather than calling get_physinfo in order to give vtsc_khz a specific value
> instead of zero can we not leave it as zero and just not call
>  xc_domain_set_tsc_info() in that case and let the hypervisor default to
> using the host rate?
> 
> Then the check in libxl just becomes "is vtsc_khz non-zero and is tsc_mode
> not DEFAULT".
> 
> Don't forget to switch from fprintf to the proper log macros.
> 
> Ian.