| Message ID | c679a11a-d2b5-403a-5341-3e00ac91ff45@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: further improve timer freq calibration accuracy | expand |
On Mon, Feb 14, 2022 at 10:25:31AM +0100, Jan Beulich wrote: > In TDT mode there's no point writing TDCR or TMICT, while outside of > that mode there's no need for the MFENCE. > > No change intended to overall functioning. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> I've got some comments below, now that the current proposal is bad, but I think we could simplify a bit more. > --- > v2: New. > > --- a/xen/arch/x86/apic.c > +++ b/xen/arch/x86/apic.c > @@ -1059,24 +1059,25 @@ static void __setup_APIC_LVTT(unsigned i > { > unsigned int lvtt_value, tmp_value; > > - /* NB. Xen uses local APIC timer in one-shot mode. */ > - lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR; > - > if ( tdt_enabled ) > { > - lvtt_value &= (~APIC_TIMER_MODE_MASK); > - lvtt_value |= APIC_TIMER_MODE_TSC_DEADLINE; > + lvtt_value = APIC_TIMER_MODE_TSC_DEADLINE | LOCAL_TIMER_VECTOR; > + apic_write(APIC_LVTT, lvtt_value); > + > + /* > + * See Intel SDM: TSC-Deadline Mode chapter. In xAPIC mode, > + * writing to the APIC LVTT and TSC_DEADLINE MSR isn't serialized. > + * According to Intel, MFENCE can do the serialization here. > + */ > + asm volatile( "mfence" : : : "memory" ); > + > + return; > } > > + /* NB. Xen uses local APIC timer in one-shot mode. */ > + lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR; While here I wouldn't mind if you replaced the comment(s) here with APIC_TIMER_MODE_ONESHOT. I think that's clearer. I wouldn't mind if you did something like: unsigned int lvtt_value = (tdt_enabled ? APIC_TIMER_MODE_TSC_DEADLINE : APIC_TIMER_MODE_ONESHOT) | LOCAL_TIMER_VECTOR; apic_write(APIC_LVTT, lvtt_value); if ( tdt_enabled ) { MFENCE; return; } Thanks, Roger.
On 11.03.2022 15:05, Roger Pau Monné wrote: > On Mon, Feb 14, 2022 at 10:25:31AM +0100, Jan Beulich wrote: >> In TDT mode there's no point writing TDCR or TMICT, while outside of >> that mode there's no need for the MFENCE. >> >> No change intended to overall functioning. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > I've got some comments below, now that the current proposal is bad, > but I think we could simplify a bit more. I'm struggling with the sentence; perhaps s/now/not/ was meant? >> --- a/xen/arch/x86/apic.c >> +++ b/xen/arch/x86/apic.c >> @@ -1059,24 +1059,25 @@ static void __setup_APIC_LVTT(unsigned i >> { >> unsigned int lvtt_value, tmp_value; >> >> - /* NB. Xen uses local APIC timer in one-shot mode. */ >> - lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR; >> - >> if ( tdt_enabled ) >> { >> - lvtt_value &= (~APIC_TIMER_MODE_MASK); >> - lvtt_value |= APIC_TIMER_MODE_TSC_DEADLINE; >> + lvtt_value = APIC_TIMER_MODE_TSC_DEADLINE | LOCAL_TIMER_VECTOR; >> + apic_write(APIC_LVTT, lvtt_value); >> + >> + /* >> + * See Intel SDM: TSC-Deadline Mode chapter. In xAPIC mode, >> + * writing to the APIC LVTT and TSC_DEADLINE MSR isn't serialized. >> + * According to Intel, MFENCE can do the serialization here. >> + */ >> + asm volatile( "mfence" : : : "memory" ); >> + >> + return; >> } >> >> + /* NB. Xen uses local APIC timer in one-shot mode. */ >> + lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR; > > While here I wouldn't mind if you replaced the comment(s) here with > APIC_TIMER_MODE_ONESHOT. I think that's clearer. > > I wouldn't mind if you did something like: > > unsigned int lvtt_value = (tdt_enabled ? APIC_TIMER_MODE_TSC_DEADLINE > : APIC_TIMER_MODE_ONESHOT) | > LOCAL_TIMER_VECTOR; I'm happy to switch to using APIC_TIMER_MODE_ONESHOT, but ... > apic_write(APIC_LVTT, lvtt_value); > > if ( tdt_enabled ) > { > MFENCE; > return; > } ... I'd prefer to stick to just a single tdt_enabled conditional. But then I'm also unclear about your use of "comment(s)" - what is the (optional?) plural referring to? Jan
On Mon, Mar 14, 2022 at 09:25:07AM +0100, Jan Beulich wrote: > On 11.03.2022 15:05, Roger Pau Monné wrote: > > On Mon, Feb 14, 2022 at 10:25:31AM +0100, Jan Beulich wrote: > >> In TDT mode there's no point writing TDCR or TMICT, while outside of > >> that mode there's no need for the MFENCE. > >> > >> No change intended to overall functioning. > >> > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > > > I've got some comments below, now that the current proposal is bad, > > but I think we could simplify a bit more. > > I'm struggling with the sentence; perhaps s/now/not/ was meant? Indeed, s/now/not/ is what I meant. > >> --- a/xen/arch/x86/apic.c > >> +++ b/xen/arch/x86/apic.c > >> @@ -1059,24 +1059,25 @@ static void __setup_APIC_LVTT(unsigned i > >> { > >> unsigned int lvtt_value, tmp_value; > >> > >> - /* NB. Xen uses local APIC timer in one-shot mode. */ > >> - lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR; > >> - > >> if ( tdt_enabled ) > >> { > >> - lvtt_value &= (~APIC_TIMER_MODE_MASK); > >> - lvtt_value |= APIC_TIMER_MODE_TSC_DEADLINE; > >> + lvtt_value = APIC_TIMER_MODE_TSC_DEADLINE | LOCAL_TIMER_VECTOR; > >> + apic_write(APIC_LVTT, lvtt_value); > >> + > >> + /* > >> + * See Intel SDM: TSC-Deadline Mode chapter. In xAPIC mode, > >> + * writing to the APIC LVTT and TSC_DEADLINE MSR isn't serialized. > >> + * According to Intel, MFENCE can do the serialization here. > >> + */ > >> + asm volatile( "mfence" : : : "memory" ); > >> + > >> + return; > >> } > >> > >> + /* NB. Xen uses local APIC timer in one-shot mode. */ > >> + lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR; > > > > While here I wouldn't mind if you replaced the comment(s) here with > > APIC_TIMER_MODE_ONESHOT. I think that's clearer. > > > > I wouldn't mind if you did something like: > > > > unsigned int lvtt_value = (tdt_enabled ? APIC_TIMER_MODE_TSC_DEADLINE > > : APIC_TIMER_MODE_ONESHOT) | > > LOCAL_TIMER_VECTOR; > > I'm happy to switch to using APIC_TIMER_MODE_ONESHOT, but ... > > > apic_write(APIC_LVTT, lvtt_value); > > > > if ( tdt_enabled ) > > { > > MFENCE; > > return; > > } > > ... I'd prefer to stick to just a single tdt_enabled conditional. > But then I'm also unclear about your use of "comment(s)" - what is > the (optional?) plural referring to? I considered the switch to use APIC_TIMER_MODE_ONESHOT one comment, while the switch to set lvtt_value only once another one. I'm fine if you want to leave the layout as-is while using APIC_TIMER_MODE_ONESHOT. Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
--- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -1059,24 +1059,25 @@ static void __setup_APIC_LVTT(unsigned i { unsigned int lvtt_value, tmp_value; - /* NB. Xen uses local APIC timer in one-shot mode. */ - lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR; - if ( tdt_enabled ) { - lvtt_value &= (~APIC_TIMER_MODE_MASK); - lvtt_value |= APIC_TIMER_MODE_TSC_DEADLINE; + lvtt_value = APIC_TIMER_MODE_TSC_DEADLINE | LOCAL_TIMER_VECTOR; + apic_write(APIC_LVTT, lvtt_value); + + /* + * See Intel SDM: TSC-Deadline Mode chapter. In xAPIC mode, + * writing to the APIC LVTT and TSC_DEADLINE MSR isn't serialized. + * According to Intel, MFENCE can do the serialization here. + */ + asm volatile( "mfence" : : : "memory" ); + + return; } + /* NB. Xen uses local APIC timer in one-shot mode. */ + lvtt_value = /*APIC_TIMER_MODE_PERIODIC |*/ LOCAL_TIMER_VECTOR; apic_write(APIC_LVTT, lvtt_value); - /* - * See Intel SDM: TSC-Deadline Mode chapter. In xAPIC mode, - * writing to the APIC LVTT and TSC_DEADLINE MSR isn't serialized. - * According to Intel, MFENCE can do the serialization here. - */ - asm volatile( "mfence" : : : "memory" ); - tmp_value = apic_read(APIC_TDCR); apic_write(APIC_TDCR, tmp_value | APIC_TDR_DIV_1);
In TDT mode there's no point writing TDCR or TMICT, while outside of that mode there's no need for the MFENCE. No change intended to overall functioning. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: New.