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

[Ian Campbell <ian.campbell@citrix.com>]

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.

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.