[PATCH] x86: clear HPET configuration registers on startup

[PATCH] x86: clear HPET configuration registers on startup

[Jan Beulich]

While Linux itself has been calling hpet_disable() for quite a while,
having e.g. a secondary (kexec) kernel depend on such behavior of the
primary (crashed) environment is fragile. It particularly broke until
very recently when the primary environment was Xen based, as that
hypervisor did not clear any of the HPET settings it may have used.

Rather than blindly (and incompletely) clearing certain HPET settings
in hpet_disable(), latch the config register settings during boot and
restore then here.

(Note on the hpet_set_mode() change: Now that we're clearing the level
bit upon initialization, there's no need anymore to do so here.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 arch/x86/kernel/hpet.c |   59 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 8 deletions(-)

--- 3.4-rc1-x86-hpet.orig/arch/x86/kernel/hpet.c
+++ 3.4-rc1-x86-hpet/arch/x86/kernel/hpet.c
@@ -319,8 +319,6 @@ static void hpet_set_mode(enum clock_eve
 		now = hpet_readl(HPET_COUNTER);
 		cmp = now + (unsigned int) delta;
 		cfg = hpet_readl(HPET_Tn_CFG(timer));
-		/* Make sure we use edge triggered interrupts */
-		cfg &= ~HPET_TN_LEVEL;
 		cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC |
 		       HPET_TN_SETVAL | HPET_TN_32BIT;
 		hpet_writel(cfg, HPET_Tn_CFG(timer));
@@ -787,15 +785,16 @@ static int hpet_clocksource_register(voi
 	return 0;
 }
 
+static u32 *hpet_boot_cfg;
+
 /**
  * hpet_enable - Try to setup the HPET timer. Returns 1 on success.
  */
 int __init hpet_enable(void)
 {
-	unsigned long hpet_period;
-	unsigned int id;
+	u32 hpet_period, cfg, id;
 	u64 freq;
-	int i;
+	unsigned int i, last;
 
 	if (!is_hpet_capable())
 		return 0;
@@ -847,15 +846,45 @@ int __init hpet_enable(void)
 	id = hpet_readl(HPET_ID);
 	hpet_print_config();
 
+	last = (id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT;
+
 #ifdef CONFIG_HPET_EMULATE_RTC
 	/*
 	 * The legacy routing mode needs at least two channels, tick timer
 	 * and the rtc emulation channel.
 	 */
-	if (!(id & HPET_ID_NUMBER))
+	if (!last)
 		goto out_nohpet;
 #endif
 
+	cfg = hpet_readl(HPET_CFG);
+	hpet_boot_cfg = kmalloc((last + 2) * sizeof(*hpet_boot_cfg),
+				GFP_KERNEL);
+	if (hpet_boot_cfg)
+		*hpet_boot_cfg = cfg;
+	else
+		pr_warn("HPET initial state will not be saved\n");
+	cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
+	hpet_writel(cfg, HPET_Tn_CFG(i));
+	if (cfg)
+		pr_warn("HPET: Unrecognized bits %#x set in global cfg\n",
+			cfg);
+
+	for (i = 0; i <= last; ++i) {
+		cfg = hpet_readl(HPET_Tn_CFG(i));
+		if (hpet_boot_cfg)
+			hpet_boot_cfg[i + 1] = cfg;
+		cfg &= ~(HPET_TN_ENABLE | HPET_TN_LEVEL | HPET_TN_FSB);
+		hpet_writel(cfg, HPET_Tn_CFG(i));
+		cfg &= ~(HPET_TN_PERIODIC | HPET_TN_PERIODIC_CAP
+			 | HPET_TN_64BIT_CAP | HPET_TN_32BIT | HPET_TN_ROUTE
+			 | HPET_TN_FSB | HPET_TN_FSB_CAP);
+		if (cfg)
+			pr_warn("HPET: Unrecognized bits %#x set in cfg#%u\n",
+				cfg, i);
+	}
+	hpet_print_config();
+
 	if (hpet_clocksource_register())
 		goto out_nohpet;
 
@@ -923,14 +952,28 @@ fs_initcall(hpet_late_init);
 void hpet_disable(void)
 {
 	if (is_hpet_capable() && hpet_virt_address) {
-		unsigned int cfg = hpet_readl(HPET_CFG);
+		unsigned int cfg = hpet_readl(HPET_CFG), id, last;
 
-		if (hpet_legacy_int_enabled) {
+		if (hpet_boot_cfg)
+			cfg = *hpet_boot_cfg;
+		else if (hpet_legacy_int_enabled) {
 			cfg &= ~HPET_CFG_LEGACY;
 			hpet_legacy_int_enabled = 0;
 		}
 		cfg &= ~HPET_CFG_ENABLE;
 		hpet_writel(cfg, HPET_CFG);
+
+		if (!hpet_boot_cfg)
+			return;
+
+		id = hpet_readl(HPET_ID);
+		last = ((id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT);
+
+		for (id = 0; id <= last; ++id)
+			hpet_writel(hpet_boot_cfg[id + 1], HPET_Tn_CFG(id));
+
+		if (*hpet_boot_cfg & HPET_CFG_ENABLE)
+			hpet_writel(*hpet_boot_cfg, HPET_CFG);
 	}
 }
 

[tip:timers/core] x86: Clear HPET configuration registers on startup

[tip-bot for Jan Beulich]

Commit-ID:  396e2c6fed4ff13b53ce0e573105531cf53b0cad
Gitweb:     http://git.kernel.org/tip/396e2c6fed4ff13b53ce0e573105531cf53b0cad
Author:     Jan Beulich <JBeulich@suse.com>
AuthorDate: Mon, 2 Apr 2012 15:15:55 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 7 May 2012 15:06:27 +0200

x86: Clear HPET configuration registers on startup

While Linux itself has been calling hpet_disable() for quite a
while, having e.g. a secondary (kexec) kernel depend on such
behavior of the primary (crashed) environment is fragile. It
particularly broke until very recently when the primary
environment was Xen based, as that hypervisor did not clear any
of the HPET settings it may have used.

Rather than blindly (and incompletely) clearing certain HPET
settings in hpet_disable(), latch the config register settings
during boot and restore then here.

(Note on the hpet_set_mode() change: Now that we're clearing the
level bit upon initialization, there's no need anymore to do so
here.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Link: http://lkml.kernel.org/r/4F79D0BB020000780007C02D@nat28.tlf.novell.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/hpet.c |   59 +++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index ad0de0c..70bce5d 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -319,8 +319,6 @@ static void hpet_set_mode(enum clock_event_mode mode,
 		now = hpet_readl(HPET_COUNTER);
 		cmp = now + (unsigned int) delta;
 		cfg = hpet_readl(HPET_Tn_CFG(timer));
-		/* Make sure we use edge triggered interrupts */
-		cfg &= ~HPET_TN_LEVEL;
 		cfg |= HPET_TN_ENABLE | HPET_TN_PERIODIC |
 		       HPET_TN_SETVAL | HPET_TN_32BIT;
 		hpet_writel(cfg, HPET_Tn_CFG(timer));
@@ -787,15 +785,16 @@ static int hpet_clocksource_register(void)
 	return 0;
 }
 
+static u32 *hpet_boot_cfg;
+
 /**
  * hpet_enable - Try to setup the HPET timer. Returns 1 on success.
  */
 int __init hpet_enable(void)
 {
-	unsigned long hpet_period;
-	unsigned int id;
+	u32 hpet_period, cfg, id;
 	u64 freq;
-	int i;
+	unsigned int i, last;
 
 	if (!is_hpet_capable())
 		return 0;
@@ -847,15 +846,45 @@ int __init hpet_enable(void)
 	id = hpet_readl(HPET_ID);
 	hpet_print_config();
 
+	last = (id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT;
+
 #ifdef CONFIG_HPET_EMULATE_RTC
 	/*
 	 * The legacy routing mode needs at least two channels, tick timer
 	 * and the rtc emulation channel.
 	 */
-	if (!(id & HPET_ID_NUMBER))
+	if (!last)
 		goto out_nohpet;
 #endif
 
+	cfg = hpet_readl(HPET_CFG);
+	hpet_boot_cfg = kmalloc((last + 2) * sizeof(*hpet_boot_cfg),
+				GFP_KERNEL);
+	if (hpet_boot_cfg)
+		*hpet_boot_cfg = cfg;
+	else
+		pr_warn("HPET initial state will not be saved\n");
+	cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
+	hpet_writel(cfg, HPET_Tn_CFG(i));
+	if (cfg)
+		pr_warn("HPET: Unrecognized bits %#x set in global cfg\n",
+			cfg);
+
+	for (i = 0; i <= last; ++i) {
+		cfg = hpet_readl(HPET_Tn_CFG(i));
+		if (hpet_boot_cfg)
+			hpet_boot_cfg[i + 1] = cfg;
+		cfg &= ~(HPET_TN_ENABLE | HPET_TN_LEVEL | HPET_TN_FSB);
+		hpet_writel(cfg, HPET_Tn_CFG(i));
+		cfg &= ~(HPET_TN_PERIODIC | HPET_TN_PERIODIC_CAP
+			 | HPET_TN_64BIT_CAP | HPET_TN_32BIT | HPET_TN_ROUTE
+			 | HPET_TN_FSB | HPET_TN_FSB_CAP);
+		if (cfg)
+			pr_warn("HPET: Unrecognized bits %#x set in cfg#%u\n",
+				cfg, i);
+	}
+	hpet_print_config();
+
 	if (hpet_clocksource_register())
 		goto out_nohpet;
 
@@ -923,14 +952,28 @@ fs_initcall(hpet_late_init);
 void hpet_disable(void)
 {
 	if (is_hpet_capable() && hpet_virt_address) {
-		unsigned int cfg = hpet_readl(HPET_CFG);
+		unsigned int cfg = hpet_readl(HPET_CFG), id, last;
 
-		if (hpet_legacy_int_enabled) {
+		if (hpet_boot_cfg)
+			cfg = *hpet_boot_cfg;
+		else if (hpet_legacy_int_enabled) {
 			cfg &= ~HPET_CFG_LEGACY;
 			hpet_legacy_int_enabled = 0;
 		}
 		cfg &= ~HPET_CFG_ENABLE;
 		hpet_writel(cfg, HPET_CFG);
+
+		if (!hpet_boot_cfg)
+			return;
+
+		id = hpet_readl(HPET_ID);
+		last = ((id & HPET_ID_NUMBER) >> HPET_ID_NUMBER_SHIFT);
+
+		for (id = 0; id <= last; ++id)
+			hpet_writel(hpet_boot_cfg[id + 1], HPET_Tn_CFG(id));
+
+		if (*hpet_boot_cfg & HPET_CFG_ENABLE)
+			hpet_writel(*hpet_boot_cfg, HPET_CFG);
 	}
 }
 

Re: [PATCH] x86: clear HPET configuration registers on startup

[Thomas Gleixner]

On Mon, 2 Apr 2012, Jan Beulich wrote:

Sorry for ignoring this for so long.

> +	cfg = hpet_readl(HPET_CFG);
> +	hpet_boot_cfg = kmalloc((last + 2) * sizeof(*hpet_boot_cfg),
> +				GFP_KERNEL);
> +	if (hpet_boot_cfg)
> +		*hpet_boot_cfg = cfg;
> +	else
> +		pr_warn("HPET initial state will not be saved\n");
> +	cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
> +	hpet_writel(cfg, HPET_Tn_CFG(i));

This wants to be  

> +	hpet_writel(cfg, HPET_CFG);

Right ?

You would have noticed if that code would have run on an AMD SB700
based machine and i would have been != 0 :)

> +	if (cfg)
> +		pr_warn("HPET: Unrecognized bits %#x set in global cfg\n",
> +			cfg);
> +
> +	for (i = 0; i <= last; ++i) {
> +		cfg = hpet_readl(HPET_Tn_CFG(i));
> +		if (hpet_boot_cfg)
> +			hpet_boot_cfg[i + 1] = cfg;
> +		cfg &= ~(HPET_TN_ENABLE | HPET_TN_LEVEL | HPET_TN_FSB);
> +		hpet_writel(cfg, HPET_Tn_CFG(i));
> +		cfg &= ~(HPET_TN_PERIODIC | HPET_TN_PERIODIC_CAP
> +			 | HPET_TN_64BIT_CAP | HPET_TN_32BIT | HPET_TN_ROUTE
> +			 | HPET_TN_FSB | HPET_TN_FSB_CAP);
> +		if (cfg)
> +			pr_warn("HPET: Unrecognized bits %#x set in cfg#%u\n",
> +				cfg, i);
> +	}
> +	hpet_print_config();
> +
>  	if (hpet_clocksource_register())
>  		goto out_nohpet;
>  
> @@ -923,14 +952,28 @@ fs_initcall(hpet_late_init);
>  void hpet_disable(void)
>  {
>  	if (is_hpet_capable() && hpet_virt_address) {
> -		unsigned int cfg = hpet_readl(HPET_CFG);
> +		unsigned int cfg = hpet_readl(HPET_CFG), id, last;
>  
> -		if (hpet_legacy_int_enabled) {
> +		if (hpet_boot_cfg)
> +			cfg = *hpet_boot_cfg;

That restores the setting which you recorded at init time. Why do you
want to do that? There is no point to restore to an eventually borked
state. If we shut down the thing, then we better leave it in a
consistent state rather than something dubious, really.

Thanks,

	tglx

Re: [PATCH] x86: clear HPET configuration registers on startup

[Jan Beulich]

>>> On 25.05.12 at 00:06, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 2 Apr 2012, Jan Beulich wrote:
> 
> Sorry for ignoring this for so long.
> 
>> +	cfg = hpet_readl(HPET_CFG);
>> +	hpet_boot_cfg = kmalloc((last + 2) * sizeof(*hpet_boot_cfg),
>> +				GFP_KERNEL);
>> +	if (hpet_boot_cfg)
>> +		*hpet_boot_cfg = cfg;
>> +	else
>> +		pr_warn("HPET initial state will not be saved\n");
>> +	cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
>> +	hpet_writel(cfg, HPET_Tn_CFG(i));
> 
> This wants to be  
> 
>> +	hpet_writel(cfg, HPET_CFG);
> 
> Right ?

Oh yes, absolutely.

>> @@ -923,14 +952,28 @@ fs_initcall(hpet_late_init);
>>  void hpet_disable(void)
>>  {
>>  	if (is_hpet_capable() && hpet_virt_address) {
>> -		unsigned int cfg = hpet_readl(HPET_CFG);
>> +		unsigned int cfg = hpet_readl(HPET_CFG), id, last;
>>  
>> -		if (hpet_legacy_int_enabled) {
>> +		if (hpet_boot_cfg)
>> +			cfg = *hpet_boot_cfg;
> 
> That restores the setting which you recorded at init time. Why do you
> want to do that? There is no point to restore to an eventually borked
> state. If we shut down the thing, then we better leave it in a
> consistent state rather than something dubious, really.

The problem is that we can't - forward compatibly - say what
is "borked" and what is merely beyond the knowledge of the
kernel. Given the system was able to boot with the original
settings, restoring them seems the safest approach to me.

Besides that it's not the purpose of the patch to get around
firmware bugs, but instead to get the hardware back into
boot-time like state. So I'd really like to merely correct the
error above that you pointed out (which also would seem to
be the most appropriate route given that Linus already
merged the patch), and leave a decision whether you agree
with my position here (or whether you want to further
tweak that code) to you.

Jan

Re: [PATCH] x86: clear HPET configuration registers on startup

[Thomas Gleixner]

On Fri, 25 May 2012, Jan Beulich wrote:
> >>> On 25.05.12 at 00:06, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Mon, 2 Apr 2012, Jan Beulich wrote:
> > 
> > Sorry for ignoring this for so long.
> > 
> >> +	cfg = hpet_readl(HPET_CFG);
> >> +	hpet_boot_cfg = kmalloc((last + 2) * sizeof(*hpet_boot_cfg),
> >> +				GFP_KERNEL);
> >> +	if (hpet_boot_cfg)
> >> +		*hpet_boot_cfg = cfg;
> >> +	else
> >> +		pr_warn("HPET initial state will not be saved\n");
> >> +	cfg &= ~(HPET_CFG_ENABLE | HPET_CFG_LEGACY);
> >> +	hpet_writel(cfg, HPET_Tn_CFG(i));
> > 
> > This wants to be  
> > 
> >> +	hpet_writel(cfg, HPET_CFG);
> > 
> > Right ?
> 
> Oh yes, absolutely.
> 
> >> @@ -923,14 +952,28 @@ fs_initcall(hpet_late_init);
> >>  void hpet_disable(void)
> >>  {
> >>  	if (is_hpet_capable() && hpet_virt_address) {
> >> -		unsigned int cfg = hpet_readl(HPET_CFG);
> >> +		unsigned int cfg = hpet_readl(HPET_CFG), id, last;
> >>  
> >> -		if (hpet_legacy_int_enabled) {
> >> +		if (hpet_boot_cfg)
> >> +			cfg = *hpet_boot_cfg;
> > 
> > That restores the setting which you recorded at init time. Why do you
> > want to do that? There is no point to restore to an eventually borked
> > state. If we shut down the thing, then we better leave it in a
> > consistent state rather than something dubious, really.
> 
> The problem is that we can't - forward compatibly - say what
> is "borked" and what is merely beyond the knowledge of the
> kernel. Given the system was able to boot with the original
> settings, restoring them seems the safest approach to me.
> 
> Besides that it's not the purpose of the patch to get around
> firmware bugs, but instead to get the hardware back into
> boot-time like state. So I'd really like to merely correct the
> error above that you pointed out (which also would seem to
> be the most appropriate route given that Linus already
> merged the patch), and leave a decision whether you agree
> with my position here (or whether you want to further
> tweak that code) to you.

I can see the point, but what I really don't like is restoring to an
eventually enabled state instead of doing it proper and keep the thing
shut down.

Thanks,

	tglx