Re: [Xen-devel] [PATCH 04/13] x86/hvm: Setup TSC scaling ratio

[Haozhong Zhang <haozhong.zhang@xxxxxxxxx>]

On Thu, Oct 22, 2015 at 10:05:39AM -0600, Jan Beulich wrote:
> >>> On 22.10.15 at 17:55, <haozhong.zhang@xxxxxxxxx> wrote:
> > On Thu, Oct 22, 2015 at 07:13:07AM -0600, Jan Beulich wrote:
> >> >>> On 28.09.15 at 09:13, <haozhong.zhang@xxxxxxxxx> wrote:
> >> > +    {
> >> > +        printk(XENLOG_WARNING
> >> > +               "Invalid TSC scaling ratio - virtual tsc khz=%lu\n",
> >> > +               khz);
> >> 
> >> Who can issue a call to this function under which conditions? I.e. is
> >> a non-ratelimited printk() okay here? Plus, without identifying the
> >> subject vcpu I don't think the message is of much use beyond your
> >> initial debugging purposes.
> > 
> > hvm_load_cpu_ctxt(), hvm_vcpu_reset_state() and tsc_set_info() call
> > this function. Am I correct that those functions are not called in
> > high rate? But I agree that the warning is useless w/o identifying the
> > subject vcpu and will add it.
> 
> The question isn't whether they're normally called at a high rate,
> but whether a malicious entity could make it so.
>

tsc_set_info() can be called through XEN_DOMCTL_settscinfo by
toolstack, so a malicious entity in toolstack (if any) could cause
problem.

Then I should remove the message here.

> >> And what's the
> >> reason for the dependency on !vtsc (please also see the comment
> >> ahead of tsc_set_info())?
> > 
> > If v->domain->arch.vtsc == 1, guest rdtsc/rdtscp is trapped (setup in
> > tsc_set_info()) and emulated by hypervisor and the hardware TSC
> > scaling is not used in this case.
> 
> But there wouldn't be any harm, I suppose?
>

should be harmless.

> >> > @@ -1981,8 +1983,14 @@ void tsc_set_info(struct domain *d,
> >> >      if ( is_hvm_domain(d) )
> >> >      {
> >> >          hvm_set_rdtsc_exiting(d, d->arch.vtsc);
> >> > -        if ( d->vcpu && d->vcpu[0] && incarnation == 0 )
> >> > +        if ( d->vcpu && d->vcpu[0] )
> >> >          {
> >> > +            if ( !d->arch.vtsc && hvm_funcs.tsc_scaling_supported )
> >> > +                hvm_setup_tsc_scaling(d->vcpu[0]);
> >> 
> >> And what about the other vCPU-s? If you mean this to be along
> >> the lines of the code that follows here, you should put this after
> >> the comment explaining that.
> >>
> > 
> > TSC scaling for other vcpus are set in hvm_vcpu_reset_state(). But I'm
> > not sure it can be moved together with the followed code because of
> > the followed
> >      if ( incarnation )
> >          return;
> > 
> > incarnation != 0 after migration and the setup of TSC scaling is
> > however necessary.
> 
> It seems to me that you could move _all_ your additions to the if()
> body past the comment (perhaps adjusting that one as needed).
>

Good suggestion! I'll move and adapt the comment to the beginning of the 
if-body.

> Jan
> 

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