Date:	Wed, 22 Apr 2009 12:46:29 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Andreas Herrmann <andreas.herrmann3@....com>
Cc:	Jan Kiszka <jan.kiszka@....de>, Jeff Mahoney <jeffm@...e.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [BUG] IO-APIC + timer doesn't work!


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

> On Sat, Apr 18, 2009 at 10:46:32PM +0200, Jan Kiszka wrote:
> > Ingo Molnar wrote:
> > > * Jan Kiszka <jan.kiszka@....de> wrote:
> > > 
> > >> Ingo Molnar wrote:
> > >>> * Jeff Mahoney <jeffm@...e.com> 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

------------------->
>From ff3bb72cc6af05828504b964ff1d1bd193524b63 Mon Sep 17 00:00:00 2001
From: Jan Kiszka <jan.kiszka@....de>
Date: Sat, 18 Apr 2009 10:33:14 +0200
Subject: [PATCH] x86, hpet: fix "IO-APIC + timer doesn't work!"

Jeff Mahoney wrote:

> I saw this while booting 2.6.30-rc1, -rc2, and today's git, on one of
> my development nodes. This output is with apic=debug. With noapic,
> it still hung. Both outputs follow.
>
> git bisect leads to commit 8d6f0c8214928f7c5083dd54ecb69c5d615b516e,
> but I'm not seeing anything obvious there. Backing just that change
> out doesn't fix it.
>
>  enabled ExtINT on CPU#0
>  ESR value before enabling vector: 0x00000004  after: 0x00000000
>  ENABLING IO-APIC IRQs
>  ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=0 pin2=0
>  ..MP-BIOS bug: 8254 timer not connected to IO-APIC
>  ...trying to set up timer (IRQ0) through the 8259A ...
>  ..... (found apic 0 pin 0) ...
>  ....... failed.
>  ...trying to set up timer as Virtual Wire IRQ...
>  ..... failed.
>  ...trying to set up timer as ExtINT IRQ...
>  ..... failed :(.
>  Kernel panic - not syncing: IO-APIC + timer doesn't work!  Boot with apic=debug and send a report.  Then try booting with the 'noapic' option.

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.

Lets try a quirk: write 'delta' a second time.

 [1] http://permalink.gmane.org/gmane.comp.emulators.qemu/41570

[ Impact: fix rare boot panic ]

Tested-by: Jeff Mahoney <jeffm@...e.com>
Signed-off-by: Ingo Molnar <mingo@...e.hu>
---
 arch/x86/kernel/hpet.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists