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

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

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.