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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

Jan


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