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

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

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.
>

Yes, this is a better place.

> 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?
>

Alternatively, I can leave vtsc_khz zero if it's not set in xl.cfg
and, if xc_domain_set_tsc_info() passes a zero vtsc_khz to hypervisor,
the latter will just use the host tsc rate (which was the original logic),

> Then the check in libxl just becomes "is vtsc_khz non-zero and is tsc_mode
> not DEFAULT".
>

... and use this check in libxl__domain_build_info_setdefault().

> Don't forget to switch from fprintf to the proper log macros.
>

Of course. Thanks for reminding!

- Haozhong

> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel