Re: [BUG] IO-APIC + timer doesn't work!

[Ingo Molnar]

* Andreas Herrmann <andreas.herrmann3@xxxxxxx> wrote:

> On Sat, Apr 18, 2009 at 10:46:32PM +0200, Jan Kiszka wrote:
> > Ingo Molnar wrote:
> > > * Jan Kiszka <jan.kiszka@xxxxxx> wrote:
> > > 
> > >> Ingo Molnar wrote:
> > >>> * Jeff Mahoney <jeffm@xxxxxxxx> wrote:
> > >>>
> > >>>>> Hmmmmm. That somehow reminds me of what I thought I had to fix in the
> > >>>>> HPET emulation of QEMU just recently [1] - because of 2.6.30-rc's behavior.
> > >>>>>
> > >>>>> Could you try if writing 'delta' a second time makes any difference on
> > >>>>> that box?
> > >>>>>
> > >>>>> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
> > >>>>> index 648b3a2..523d72b 100644
> > >>>>> --- a/arch/x86/kernel/hpet.c
> > >>>>> +++ b/arch/x86/kernel/hpet.c
> > >>>>> @@ -324,6 +324,7 @@ static void hpet_set_mode(enum clock_event_mode mode,
> > >>>>>  		       HPET_TN_SETVAL | HPET_TN_32BIT;
> > >>>>>  		hpet_writel(cfg, HPET_Tn_CFG(timer));
> > >>>>>  		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> > >>>>> +		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> > >>>>>  		hpet_start_counter();
> > >>>>>  		hpet_print_config();
> > >>>>>  		break;
> > >>>>>
> > >>>> Thanks, Jan.
> > >>>>
> > >>>> That fixed it for me.
> > >>> I've queued it up (and i've got a test-system that might be affected 
> > >>> by a similar problem - it shows a similar crash very rarely), but it 
> > >>> would be nice to know why this duplicate writeout makes a 
> > >>> difference. Jan?
> > >>>
> > >>> 	Ingo
> > >> Well, if you look at the HPET spec [1], you first find the explanation
> > >> of the Tn_VAL_SET_CNF bit (HPET_TN_SETVAL):
> > >>
> > >> "[...] By writing this bit to a 1, the software is then allowed to
> > >> directly set a periodic timer's accumulator."
> > >>
> > >> That may sound like "you write to the comparator register if 0, and if
> > >> 1, you set the accumulator". That's also how HPET was emulated in QEMU
> > >> so far.
> > >>
> > >> But then you read on about changing the period of a running timer:
> > >>
> > >> "If the software resets the main counter, the value in the comparatorâs
> > >> value register needs to reset as well. This can be done by setting the
> > >> Tn_VAL_SET_CNF bit. Again, to avoid race conditions, this should be
> > >> done with the main counter halted. The following usage model is expected:
> > >> 1) Software clears the GLOBAL_ENABLE_CNF bit to prevent any interrupts
> > >> 2) Software Clears the main counter by writing a value of 00000000h to it.
> > >> 3) Software sets the TIMER0_VAL_SET_CNF bit.
> > >> 4) Software writes the new value in the TIMER0_COMPARATOR_VAL register
> > >> 5) Software sets the GLOBAL_ENABLE_CNF bit to enable interrupts."
> > >>
> > >> And that somehow sounds like you only need to write the new period once,
> > >> with Tn_VAL_SET_CNF = 1.
> > >>
> > >> I bet now that both interpretations are implemented in silicon somewhere
> > >> out there - but I'm all ears to learn the right one (and potentially
> > >> re-fix QEMU).
> > >>
> > >> Jan
> > >>
> > >> [1] http://www.intel.com/hardwaredesign/hpetspec_1.pdf
> > > 
> > > i might be a bit slow today, but how does the above transform into:
> > > 
> > >   		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> > > 		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> > > 
> > > ? It sets the same register twice.
> > 
> > No, sorry, I missed to cite also this from the Tn_SET_VAL_CFG
> > explanation: "Software does NOT have to write this bit back to 0 (it
> > automatically clears)." So the second write will already take place
> > without it.
> > 
> > > 
> > > I'm totally happy if it does transform into that under some quirky 
> > > interpretation. Since it solved the problem for Jeff, we'll likely 
> > > add it even if there's no actual explanation ;-) But it would be 
> > > nice to somehow come up with a line of reasoning that ends with:
> > > 
> > >  ... and for that reason, we set the value twice:
> > > 
> > >   		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> > > 		hpet_writel((unsigned long) delta, HPET_Tn_CMP(timer));
> > > 
> > > right?
> > > 
> > > 	Ingo
> > 
> > I'd like to give someone from AMD or Intel or whoever already
> > implemented such a logic a chance to comment on it. If this doesn't
> > happen, you may add:
> 
> I didn't implement logic but checked the AMD 81xx documentation.  And
> this exactly describes that depending on HPET_TN_SETVAL either
> accumulator or comparator is set. That is the reason why my last HPET
> patch broke HPET on that chipset. I've provided a patch to partially
> revert that commit. See
> 
> http://marc.info/?l=linux-kernel&m=124033700530097
> 
> The patch was successfully verified for bugzilla
> http://bugzilla.kernel.org/show_bug.cgi?id=12961
> 
> IMHO it should be applied asap to tip tree.

Applied to tip:x86/urgent, thanks Andreas!

I've also removed the trial baloon patch below - your patch is a 
full replacement for it, correct?

	Ingo

------------------->