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?
>> --- 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
