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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> 
> 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