RE: [Xen-devel] [PATCH] X86: Prefer TSC-deadline timer in Xen

["Wei, Gang" <gang.wei@xxxxxxxxx>]

> From: Jan Beulich [mailto:JBeulich@xxxxxxxxxx], Thursday, October 28, 2010 
> 3:46 PM
> >>> On 28.10.10 at 07:45, "Wei, Gang" <gang.wei@xxxxxxxxx> wrote:
> > --- a/xen/arch/x86/apic.c   Wed Oct 20 17:26:51 2010 +0100
> > +++ b/xen/arch/x86/apic.c   Fri Oct 29 19:24:56 2010 +0800
> > @@ -37,6 +37,15 @@
> >  #include <asm/asm_defns.h> /* for BUILD_SMP_INTERRUPT */
> >  #include <mach_apic.h>
> >  #include <io_ports.h>
> > +
> > +#define APIC_TIMER_MODE_ONESHOT         (0 << 17)
> > +#define APIC_TIMER_MODE_PERIODIC        (1 << 17)
> > +#define APIC_TIMER_MODE_TSC_DEADLINE    (2 << 17)
> > +#define APIC_TIMER_MODE_MASK            (3 << 17)
> > +
> > +static int tdt_enabled;
> > +static int tdt_disable;
> > +boolean_param("tdt_off", tdt_disable);
> 
> It would be more natural to call the parameter just "tdt", and
> use a non-zero initialized variable that gets set to zero when
> the user passes "tdt=off" (or another of the boolean false
> indicators). Perhaps you could even get away with just the
> single "tdt_enabled" variable then.

Rename the parameter should be ok. But I prefer to keep two variable there to 
avoid check both tdt_enabled & boot_cpu_has(X86_FEATURE_TSC_DEADLINE) 
everywhere.
I would like to accept user input "tdt=off", and replace tdt_disable with 
tdt_enable.

> > @@ -1360,12 +1382,24 @@ int reprogram_timer(s_time_t timeout)
> >      if ( !cpu_has_apic )
> >          return 1;
> >
> > -    if ( timeout && ((expire = timeout - NOW()) > 0) )
> > -        apic_tmict = min_t(u64, (bus_scale * expire) >> 18, UINT_MAX);
> > -
> > -    apic_write(APIC_TMICT, (unsigned long)apic_tmict);
> > -
> > -    return apic_tmict || !timeout;
> > +    if ( tdt_enabled )
> > +    {
> > +        u64 tsc = 0;
> 
> Is zero really a proper "no-timeout" indicator here?

Yes, it is. Writing zero to MSR_IA32_TSC_DEADLINE will disarm the tdt according 
to SDM.

> > +
> > +        if ( timeout )
> > +            tsc = stime2tsc(timeout);
> > +
> > +        wrmsrl(MSR_IA32_TSC_DEADLINE, tsc);
> > +    }
> > +    else
> > +    {
> > +        if ( timeout && ((expire = timeout - NOW()) > 0) )
> > +            apic_tmict = min_t(u64, (bus_scale * expire) >> 18,
> UINT_MAX);
> > +
> > +        apic_write(APIC_TMICT, (unsigned long)apic_tmict);
> > +    }
> > +
> > +    return apic_tmict || !timeout || tdt_enabled;
> 
> How can this always be successful if tdt_enabled?

If tdt_enabled, there are only three cases: 1st, timeout=0, then write 0 to tdt 
msr to stop timer, return successful; 2nd, timeout <= NOW(), a tsc value less 
than or equal current tsc will be written to tdt msr, then a expiring interrupt 
will be generated right now, return successful; 3rd, timeout > NOW(), a tsc 
value > current tsc will be written to tdt msr, also return successful. No need 
to return failed if tdt_enabled.

Jimmy

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel