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 <[email protected]>
> >
> > 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é <[email protected]>
Thanks, Roger.
