[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 3/4] x86/APIC: skip unnecessary parts of __setup_APIC_LVTT()
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |