Re: [PATCH v3 3/4] x86/APIC: skip unnecessary parts of __setup_APIC_LVTT()

["Roger Pau Monné" <roger.pau@citrix.com>]

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.