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