[Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Yasunori Goto]

Hello.

I think there is a possibility that HPET driver will return
insane value due to a SMI interruption (or switching guests by hypervisor).
I found it by reviewing, and I would like to fix it.

Current HPET driver calibrates the adjustment value
by calculation the elapse time in CPU busy loop. 
However this way is too dangerous against SMI interruption.

Here is the calibration code in hpet_calibrate()

 701 static unsigned long hpet_calibrate(struct hpets *hpetp)
             :
             :
 728         do {
 729                 m = read_counter(&hpet->hpet_mc);
 730                 write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
 731         } while (i++, (m - start) < count);
 732 
 733         local_irq_restore(flags);
 734 
 735         return (m - start) / i;

If SMI interruption occurs between 728 to 731, then return value will be
bigger value than correct one. (SMI is not able to be controlled by OS.)


This patch is a simple solution to fix it.
hpet_calibrate() is called 5 times, and one of them is expected as
correct value.

Thanks.


---

hpet_calibrate() has a possibility of miss-calibration due to SMI.
If SMI interrupts in the while loop of calibration, then return value
will be big. This changes it tries 5 times and get minimum value as
correct value.

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>

---

Index: hpet_test/drivers/char/hpet.c
===================================================================
--- hpet_test.orig/drivers/char/hpet.c	2008-12-04 16:24:02.000000000 +0900
+++ hpet_test/drivers/char/hpet.c	2008-12-04 16:34:59.000000000 +0900
@@ -713,7 +713,7 @@
  */
 #define	TICK_CALIBRATE	(1000UL)
 
-static unsigned long hpet_calibrate(struct hpets *hpetp)
+static unsigned long __hpet_calibrate(struct hpets *hpetp)
 {
 	struct hpet_timer __iomem *timer = NULL;
 	unsigned long t, m, count, i, flags, start;
@@ -750,6 +750,17 @@
 	return (m - start) / i;
 }
 
+static unsigned long hpet_calibrate(struct hpets *hpetp)
+{
+	unsigned long ret = ~0UL, i;
+
+	/* Try 5 times to remove impact of SMI.*/
+	for (i = 0; i < 5; i++)
+		ret = min(ret, __hpet_calibrate(hpetp));
+
+	return ret;
+}
+
 int hpet_alloc(struct hpet_data *hdp)
 {
 	u64 cap, mcfg;

-- 
Yasunori Goto 

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Rik van Riel]

Yasunori Goto wrote:

> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This changes it tries 5 times and get minimum value as
> correct value.
> 
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>

Acked-by: Rik van Riel <riel@redhat.com>

-- 
All rights reversed.

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Paul Gortmaker]

On Fri, Mar 13, 2009 at 1:00 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> Hello.
>
> I think there is a possibility that HPET driver will return
> insane value due to a SMI interruption (or switching guests by hypervisor).
> I found it by reviewing, and I would like to fix it.
>
> Current HPET driver calibrates the adjustment value
> by calculation the elapse time in CPU busy loop.
> However this way is too dangerous against SMI interruption.
>
> Here is the calibration code in hpet_calibrate()
>
>  701 static unsigned long hpet_calibrate(struct hpets *hpetp)
>             :
>             :
>  728         do {
>  729                 m = read_counter(&hpet->hpet_mc);
>  730                 write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
>  731         } while (i++, (m - start) < count);
>  732
>  733         local_irq_restore(flags);
>  734
>  735         return (m - start) / i;
>
> If SMI interruption occurs between 728 to 731, then return value will be
> bigger value than correct one. (SMI is not able to be controlled by OS.)
>
>
> This patch is a simple solution to fix it.
> hpet_calibrate() is called 5 times, and one of them is expected as
> correct value.

I've no sense of feel for how long each calibration run would take.
Would doing it 5 times show up as a significant increase in the boot
time for those that care about boot time being as quick as possible?

Paul.

>
> Thanks.
>
>
> ---
>
> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This changes it tries 5 times and get minimum value as
> correct value.
>
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
>
> ---
>
> Index: hpet_test/drivers/char/hpet.c
> ===================================================================
> --- hpet_test.orig/drivers/char/hpet.c  2008-12-04 16:24:02.000000000 +0900
> +++ hpet_test/drivers/char/hpet.c       2008-12-04 16:34:59.000000000 +0900
> @@ -713,7 +713,7 @@
>  */
>  #define        TICK_CALIBRATE  (1000UL)
>
> -static unsigned long hpet_calibrate(struct hpets *hpetp)
> +static unsigned long __hpet_calibrate(struct hpets *hpetp)
>  {
>        struct hpet_timer __iomem *timer = NULL;
>        unsigned long t, m, count, i, flags, start;
> @@ -750,6 +750,17 @@
>        return (m - start) / i;
>  }
>
> +static unsigned long hpet_calibrate(struct hpets *hpetp)
> +{
> +       unsigned long ret = ~0UL, i;
> +
> +       /* Try 5 times to remove impact of SMI.*/
> +       for (i = 0; i < 5; i++)
> +               ret = min(ret, __hpet_calibrate(hpetp));
> +
> +       return ret;
> +}
> +
>  int hpet_alloc(struct hpet_data *hdp)
>  {
>        u64 cap, mcfg;
>
> --
> Yasunori Goto
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Yasunori Goto]

> On Fri, Mar 13, 2009 at 1:00 AM, Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> > Hello.
> >
> > I think there is a possibility that HPET driver will return
> > insane value due to a SMI interruption (or switching guests by hypervisor).
> > I found it by reviewing, and I would like to fix it.
> >
> > Current HPET driver calibrates the adjustment value
> > by calculation the elapse time in CPU busy loop.
> > However this way is too dangerous against SMI interruption.
> >
> > Here is the calibration code in hpet_calibrate()
> >
> > ?701 static unsigned long hpet_calibrate(struct hpets *hpetp)
> > ? ? ? ? ? ? :
> > ? ? ? ? ? ? :
> > ?728 ? ? ? ? do {
> > ?729 ? ? ? ? ? ? ? ? m = read_counter(&hpet->hpet_mc);
> > ?730 ? ? ? ? ? ? ? ? write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
> > ?731 ? ? ? ? } while (i++, (m - start) < count);
> > ?732
> > ?733 ? ? ? ? local_irq_restore(flags);
> > ?734
> > ?735 ? ? ? ? return (m - start) / i;
> >
> > If SMI interruption occurs between 728 to 731, then return value will be
> > bigger value than correct one. (SMI is not able to be controlled by OS.)
> >
> >
> > This patch is a simple solution to fix it.
> > hpet_calibrate() is called 5 times, and one of them is expected as
> > correct value.
> 
> I've no sense of feel for how long each calibration run would take.
> Would doing it 5 times show up as a significant increase in the boot
> time for those that care about boot time being as quick as possible?

Hmm. The loop times is trade off against reliable value....
Though SMI is rare interruption, I don't know how frequent
hypervisor's switch is.

Each calibration of this has 1 milli second.
Do you think 5 msec is too long?
If yes, how is 3 msec? Is it still too long?

Thanks.

-- 
Yasunori Goto 

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Vojtech Pavlik]

On Mon, Mar 16, 2009 at 11:34:55AM +0900, Yasunori Goto wrote:

> > I've no sense of feel for how long each calibration run would take.
> > Would doing it 5 times show up as a significant increase in the boot
> > time for those that care about boot time being as quick as possible?
> 
> Hmm. The loop times is trade off against reliable value....
> Though SMI is rare interruption, I don't know how frequent
> hypervisor's switch is.
> 
> Each calibration of this has 1 milli second.
> Do you think 5 msec is too long?
> If yes, how is 3 msec? Is it still too long?

I would suggest to instead repeat until the total cycle count stops
decreasing: That way we can avoid a SMI and yet in the best case only
run the calibration 2 times.

-- 
Vojtech Pavlik
Director SuSE Labs

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Clemens Ladisch]

Yasunori Goto wrote:
> > I've no sense of feel for how long each calibration run would take.
> > Would doing it 5 times show up as a significant increase in the boot
> > time for those that care about boot time being as quick as possible?
> 
> Hmm. The loop times is trade off against reliable value....
> Though SMI is rare interruption, I don't know how frequent
> hypervisor's switch is.

Could you, just for testing, run the calibration five thousand times or
so instead of five times, and count how often you get insane values?
(And how much delay does such an SMI add?)

> Each calibration of this has 1 milli second.
> Do you think 5 msec is too long?

This shouldn't matter when booting.  Anyway, I think it's possible to
increase TICK_CALIBRATE without losing too much accuracy.


Best regards,
Clemens

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Yasunori Goto]

Sorry for late response.

> Yasunori Goto wrote:
> > > I've no sense of feel for how long each calibration run would take.
> > > Would doing it 5 times show up as a significant increase in the boot
> > > time for those that care about boot time being as quick as possible?
> > 
> > Hmm. The loop times is trade off against reliable value....
> > Though SMI is rare interruption, I don't know how frequent
> > hypervisor's switch is.
> 
> Could you, just for testing, run the calibration five thousand times or
> so instead of five times, and count how often you get insane values?
> (And how much delay does such an SMI add?)

I tried 50000 times. But insane value was nothing.
I think SMI is very rare.

> > Each calibration of this has 1 milli second.
> > Do you think 5 msec is too long?
> 
> This shouldn't matter when booting.  Anyway, I think it's possible to
> increase TICK_CALIBRATE without losing too much accuracy.

Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
then I'll try Vojtech-san's way.


Thanks.
-- 
Yasunori Goto 

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Paul Gortmaker]

Yasunori Goto wrote:
> Sorry for late response.
>
>   
>> Yasunori Goto wrote:
>>     
>>>> I've no sense of feel for how long each calibration run would take.
>>>> Would doing it 5 times show up as a significant increase in the boot
>>>> time for those that care about boot time being as quick as possible?
>>>>         
>>> Hmm. The loop times is trade off against reliable value....
>>> Though SMI is rare interruption, I don't know how frequent
>>> hypervisor's switch is.
>>>       
>> Could you, just for testing, run the calibration five thousand times or
>> so instead of five times, and count how often you get insane values?
>> (And how much delay does such an SMI add?)
>>     
>
> I tried 50000 times. But insane value was nothing.
> I think SMI is very rare.
>
>   
>>> Each calibration of this has 1 milli second.
>>> Do you think 5 msec is too long?
>>>       
>> This shouldn't matter when booting.  Anyway, I think it's possible to
>> increase TICK_CALIBRATE without losing too much accuracy.
>>     
>
> Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
> then I'll try Vojtech-san's way.
>   

That probably makes sense -- if it is extremely rare, and if you get two
values that are the same (within some small delta) then you probably
are 99.99% confident that you have the right data.

Paul.

>
> Thanks.
>   

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Yasunori Goto]

> >>> Each calibration of this has 1 milli second.
> >>> Do you think 5 msec is too long?
> >>>       
> >> This shouldn't matter when booting.  Anyway, I think it's possible to
> >> increase TICK_CALIBRATE without losing too much accuracy.
> >>     
> >
> > Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
> > then I'll try Vojtech-san's way.
> >   
> 
> That probably makes sense -- if it is extremely rare, and if you get two
> values that are the same (within some small delta) then you probably
> are 99.99% confident that you have the right data.

Ok. I'll make it.

Thanks for your comment.


-- 
Yasunori Goto 

[Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)

[Yasunori Goto]

> 
> > >>> Each calibration of this has 1 milli second.
> > >>> Do you think 5 msec is too long?
> > >>>       
> > >> This shouldn't matter when booting.  Anyway, I think it's possible to
> > >> increase TICK_CALIBRATE without losing too much accuracy.
> > >>     
> > >
> > > Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
> > > then I'll try Vojtech-san's way.
> > >   
> > 
> > That probably makes sense -- if it is extremely rare, and if you get two
> > values that are the same (within some small delta) then you probably
> > are 99.99% confident that you have the right data.
> 
> Ok. I'll make it.
> 
> Thanks for your comment.

Here is updated version.

--------

hpet_calibrate() has a possibility of miss-calibration due to SMI.
If SMI interrupts in the while loop of calibration, then return value
will be big. This change calibrates until stabilizing by the return
value with a small value.


Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>


---
 drivers/char/hpet.c |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Index: hpet_test/drivers/char/hpet.c
===================================================================
--- hpet_test.orig/drivers/char/hpet.c	2009-03-12 15:47:45.000000000 +0900
+++ hpet_test/drivers/char/hpet.c	2009-03-18 11:12:42.000000000 +0900
@@ -713,7 +713,7 @@
  */
 #define	TICK_CALIBRATE	(1000UL)
 
-static unsigned long hpet_calibrate(struct hpets *hpetp)
+static unsigned long __hpet_calibrate(struct hpets *hpetp)
 {
 	struct hpet_timer __iomem *timer = NULL;
 	unsigned long t, m, count, i, flags, start;
@@ -750,6 +750,25 @@
 	return (m - start) / i;
 }
 
+static unsigned long hpet_calibrate(struct hpets *hpetp)
+{
+	unsigned long ret = ~0UL, tmp;
+
+	/*
+	 * Try to calibrate until return value becomes stable small value.
+	 * If SMI interruption occurs in calibration loop, the return value
+	 * will be big. This avoids its impact.
+	 */
+	do {
+		tmp = __hpet_calibrate(hpetp);
+		if (ret <= tmp)
+			break;
+		ret = tmp;
+	} while (1);
+
+	return ret;
+}
+
 int hpet_alloc(struct hpet_data *hdp)
 {
 	u64 cap, mcfg;

-- 
Yasunori Goto 

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)

[Clemens Ladisch]

Yasunori Goto wrote:
> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This change calibrates until stabilizing by the return
> value with a small value.
> 
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>

Acked-by: Clemens Ladisch <clemens@ladisch.de>

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)

[Vojtech Pavlik]

On Wed, Mar 18, 2009 at 08:44:30AM +0100, Clemens Ladisch wrote:
> Yasunori Goto wrote:
> > hpet_calibrate() has a possibility of miss-calibration due to SMI.
> > If SMI interrupts in the while loop of calibration, then return value
> > will be big. This change calibrates until stabilizing by the return
> > value with a small value.
> > 
> > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> 
> Acked-by: Clemens Ladisch <clemens@ladisch.de>

Acked-by: Vojtech Pavlik <vojtech@suse.cz>

-- 
Vojtech Pavlik
Director SuSE Labs

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)

[Paul Gortmaker]

Yasunori Goto wrote:
>>>>>> Each calibration of this has 1 milli second.
>>>>>> Do you think 5 msec is too long?
>>>>>>       
>>>>>>             
>>>>> This shouldn't matter when booting.  Anyway, I think it's possible to
>>>>> increase TICK_CALIBRATE without losing too much accuracy.
>>>>>     
>>>>>           
>>>> Hmm. If the person who is trying to reduce boot time for fastboot dislikes this impact,
>>>> then I'll try Vojtech-san's way.
>>>>   
>>>>         
>>> That probably makes sense -- if it is extremely rare, and if you get two
>>> values that are the same (within some small delta) then you probably
>>> are 99.99% confident that you have the right data.
>>>       
>> Ok. I'll make it.
>>
>> Thanks for your comment.
>>     
>
> Here is updated version.
>
> --------
>
> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This change calibrates until stabilizing by the return
> value with a small value.
>
>
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
>
>
> ---
>  drivers/char/hpet.c |   21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> Index: hpet_test/drivers/char/hpet.c
> ===================================================================
> --- hpet_test.orig/drivers/char/hpet.c	2009-03-12 15:47:45.000000000 +0900
> +++ hpet_test/drivers/char/hpet.c	2009-03-18 11:12:42.000000000 +0900
> @@ -713,7 +713,7 @@
>   */
>  #define	TICK_CALIBRATE	(1000UL)
>  
> -static unsigned long hpet_calibrate(struct hpets *hpetp)
> +static unsigned long __hpet_calibrate(struct hpets *hpetp)
>  {
>  	struct hpet_timer __iomem *timer = NULL;
>  	unsigned long t, m, count, i, flags, start;
> @@ -750,6 +750,25 @@
>  	return (m - start) / i;
>  }
>  
> +static unsigned long hpet_calibrate(struct hpets *hpetp)
> +{
> +	unsigned long ret = ~0UL, tmp;
> +
> +	/*
> +	 * Try to calibrate until return value becomes stable small value.
> +	 * If SMI interruption occurs in calibration loop, the return value
> +	 * will be big. This avoids its impact.
> +	 */
> +	do {
> +		tmp = __hpet_calibrate(hpetp);
> +		if (ret <= tmp)
> +			break;
> +		ret = tmp;
> +	} while (1);
>   

For what it is worth, if a situation arose where continued calls to
hpet_calibrate() represented a monotonically decreasing function,
(perhaps some insane power management?) you could be stuck
in this function for an unbounded amount of time.   I don't expect
that should ever happen, but I figured I'd mention it.

Paul.

> +
> +	return ret;
> +}
> +
>  int hpet_alloc(struct hpet_data *hdp)
>  {
>  	u64 cap, mcfg;
>
>   

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)

[Clemens Ladisch]

Paul Gortmaker wrote:
> Yasunori Goto wrote:
> > +	/*
> > +	 * Try to calibrate until return value becomes stable small value.
> > +	 * If SMI interruption occurs in calibration loop, the return value
> > +	 * will be big. This avoids its impact.
> > +	 */
> > +	do {
> > +		tmp = __hpet_calibrate(hpetp);
> > +		if (ret <= tmp)
> > +			break;
> > +		ret = tmp;
> > +	} while (1);
> 
> For what it is worth, if a situation arose where continued calls to
> hpet_calibrate() represented a monotonically decreasing function,
> (perhaps some insane power management?) you could be stuck
> in this function for an unbounded amount of time.

On my machine, the number of iterations of the loop in __hpet_calibarate
(the value of i) is about 400, and since HPET reads/writes go to the
southbridge, it cannot get much higher than about 1000 even on faster
machines.

The value of (m - start) is approximately constant, as long as the loop
runs for about 1 ms, so there cannot be too many distinct return values.

Consequently, the only way for perverse SMIs to produce more
monotonically decreasing calibration values is to introduce delays that
make the loop run longer than 1 ms, i.e., to eat most of the CPU time
for at least several seconds (or longer if you want unbounded time).
Even in the real world ;-), no laptop manufacturer is that insane.


Best regards,
Clemens

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)

[Paul Gortmaker]

Clemens Ladisch wrote:
> Paul Gortmaker wrote:
>   
>> Yasunori Goto wrote:
>>     
>>> +	/*
>>> +	 * Try to calibrate until return value becomes stable small value.
>>> +	 * If SMI interruption occurs in calibration loop, the return value
>>> +	 * will be big. This avoids its impact.
>>> +	 */
>>> +	do {
>>> +		tmp = __hpet_calibrate(hpetp);
>>> +		if (ret <= tmp)
>>> +			break;
>>> +		ret = tmp;
>>> +	} while (1);
>>>       
>> For what it is worth, if a situation arose where continued calls to
>> hpet_calibrate() represented a monotonically decreasing function,
>> (perhaps some insane power management?) you could be stuck
>> in this function for an unbounded amount of time.
>>     
>
> On my machine, the number of iterations of the loop in __hpet_calibarate
> (the value of i) is about 400, and since HPET reads/writes go to the
> southbridge, it cannot get much higher than about 1000 even on faster
> machines.
>
> The value of (m - start) is approximately constant, as long as the loop
> runs for about 1 ms, so there cannot be too many distinct return values.
>
> Consequently, the only way for perverse SMIs to produce more
> monotonically decreasing calibration values is to introduce delays that
> make the loop run longer than 1 ms, i.e., to eat most of the CPU time
> for at least several seconds (or longer if you want unbounded time).
> Even in the real world ;-), no laptop manufacturer is that insane.
>   

Right - as I said, I didn't expect that it could ever happen, and your 
analysis
above seems to help reinforce that, so we probably should be OK.

Thanks,
Paul.

>
> Best regards,
> Clemens
>   

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)

[Andrew Morton]

On Wed, 18 Mar 2009 11:47:04 +0900
Yasunori Goto <y-goto@jp.fujitsu.com> wrote:

> hpet_calibrate() has a possibility of miss-calibration due to SMI.
> If SMI interrupts in the while loop of calibration, then return value
> will be big. This change calibrates until stabilizing by the return
> value with a small value.
> 
> 
> Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> 
> 
> ---
>  drivers/char/hpet.c |   21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> Index: hpet_test/drivers/char/hpet.c
> ===================================================================
> --- hpet_test.orig/drivers/char/hpet.c	2009-03-12 15:47:45.000000000 +0900
> +++ hpet_test/drivers/char/hpet.c	2009-03-18 11:12:42.000000000 +0900
> @@ -713,7 +713,7 @@
>   */
>  #define	TICK_CALIBRATE	(1000UL)
>  
> -static unsigned long hpet_calibrate(struct hpets *hpetp)
> +static unsigned long __hpet_calibrate(struct hpets *hpetp)
>  {
>  	struct hpet_timer __iomem *timer = NULL;
>  	unsigned long t, m, count, i, flags, start;
> @@ -750,6 +750,25 @@
>  	return (m - start) / i;
>  }
>  
> +static unsigned long hpet_calibrate(struct hpets *hpetp)
> +{
> +	unsigned long ret = ~0UL, tmp;
> +
> +	/*
> +	 * Try to calibrate until return value becomes stable small value.
> +	 * If SMI interruption occurs in calibration loop, the return value
> +	 * will be big. This avoids its impact.
> +	 */
> +	do {
> +		tmp = __hpet_calibrate(hpetp);
> +		if (ret <= tmp)
> +			break;
> +		ret = tmp;
> +	} while (1);
> +
> +	return ret;
> +}

Call me paranoid, but I'd like to see a maximum retry count here and an
error message-and-continue if it is exceeded.  To prevent mysterious
boot-time lockups from misbehaving hpets, perhaps?

Also, style nit - I find

	for ( ; ; ) {
		...
	}

to be more readable than

	do {
		...
	} while (1);

and I believe the former is more common.


And

	unsigned long ret = -1;

has the same effect as

	unsigned long ret = ~0UL;

but is more maintainable - it doesn't subtly break if someone changes
the type of `ret'.  (This is a bit of an ugly C trick).

Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)

[Paul Gortmaker]

[Re: [Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI. (take 2)] On 30/03/2009 (Mon 14:06) Andrew Morton wrote:

> On Wed, 18 Mar 2009 11:47:04 +0900
> Yasunori Goto <y-goto@jp.fujitsu.com> wrote:
> 
> > hpet_calibrate() has a possibility of miss-calibration due to SMI.
> > If SMI interrupts in the while loop of calibration, then return value
> > will be big. This change calibrates until stabilizing by the return
> > value with a small value.
> > 
> > 
> > Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>
> > 
> > 
> > ---
> >  drivers/char/hpet.c |   21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > Index: hpet_test/drivers/char/hpet.c
> > ===================================================================
> > --- hpet_test.orig/drivers/char/hpet.c	2009-03-12 15:47:45.000000000 +0900
> > +++ hpet_test/drivers/char/hpet.c	2009-03-18 11:12:42.000000000 +0900
> > @@ -713,7 +713,7 @@
> >   */
> >  #define	TICK_CALIBRATE	(1000UL)
> >  
> > -static unsigned long hpet_calibrate(struct hpets *hpetp)
> > +static unsigned long __hpet_calibrate(struct hpets *hpetp)
> >  {
> >  	struct hpet_timer __iomem *timer = NULL;
> >  	unsigned long t, m, count, i, flags, start;
> > @@ -750,6 +750,25 @@
> >  	return (m - start) / i;
> >  }
> >  
> > +static unsigned long hpet_calibrate(struct hpets *hpetp)
> > +{
> > +	unsigned long ret = ~0UL, tmp;
> > +
> > +	/*
> > +	 * Try to calibrate until return value becomes stable small value.
> > +	 * If SMI interruption occurs in calibration loop, the return value
> > +	 * will be big. This avoids its impact.
> > +	 */
> > +	do {
> > +		tmp = __hpet_calibrate(hpetp);
> > +		if (ret <= tmp)
> > +			break;
> > +		ret = tmp;
> > +	} while (1);
> > +
> > +	return ret;
> > +}
> 
> Call me paranoid, but I'd like to see a maximum retry count here and an
> error message-and-continue if it is exceeded.  To prevent mysterious
> boot-time lockups from misbehaving hpets, perhaps?

I'd mentioned that here:

http://lkml.org/lkml/2009/3/18/180

but the general consensus was that it was impossible and I was just
being paranoid.  :-)

Acked-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Paul.

> 
> Also, style nit - I find
> 
> 	for ( ; ; ) {
> 		...
> 	}
> 
> to be more readable than
> 
> 	do {
> 		...
> 	} while (1);
> 
> and I believe the former is more common.
> 
> 
> And
> 
> 	unsigned long ret = -1;
> 
> has the same effect as
> 
> 	unsigned long ret = ~0UL;
> 
> but is more maintainable - it doesn't subtly break if someone changes
> the type of `ret'.  (This is a bit of an ugly C trick).
> 
> 
> 

[Patch] Fix the possibility of insane return value of hpet_calibrate() against SMI.

[Yasunori Goto]

Hello.

I think there is a possibility that hpet_calibrate() will return 
an insane value when SMI interrupts.

 701 static unsigned long hpet_calibrate(struct hpets *hpetp)
             :
             :
 728         do {
 729                 m = read_counter(&hpet->hpet_mc);
 730                 write_counter(t + m + hpetp->hp_delta, &timer->hpet_compare);
 731         } while (i++, (m - start) < count);
 732 
 733         local_irq_restore(flags);
 734 
 735         return (m - start) / i;

If SMI interrupts between 728 to 731, then return value will be
bigger value than correct one. This is the fix for it. 

I found it by just reviewing about SMI and the codes of timer calibration.
But, I've not encountered this issue, and this issue is very difficult
to produce. So, if I'm something wrong, sorry for noise.

Thanks.


---

hpet_calibrate() has a possibility of miss-calibration due to SMI.
If SMI interrupts in the while loop of calibration, then return value
will be big. This changes it tries 3 times and get minimum value.

Signed-off-by: Yasunori Goto <y-goto@jp.fujitsu.com>

---

Index: hpet_test/drivers/char/hpet.c
===================================================================
--- hpet_test.orig/drivers/char/hpet.c	2008-12-04 16:24:02.000000000 +0900
+++ hpet_test/drivers/char/hpet.c	2008-12-04 16:34:59.000000000 +0900
@@ -713,7 +713,7 @@
  */
 #define	TICK_CALIBRATE	(1000UL)
 
-static unsigned long hpet_calibrate(struct hpets *hpetp)
+static unsigned long __hpet_calibrate(struct hpets *hpetp)
 {
 	struct hpet_timer __iomem *timer = NULL;
 	unsigned long t, m, count, i, flags, start;
@@ -750,6 +750,17 @@
 	return (m - start) / i;
 }
 
+static unsigned long hpet_calibrate(struct hpets *hpetp)
+{
+	unsigned long ret = ~0UL, i;
+
+	/* Try 3 times to remove impact of SMI.*/
+	for (i = 0; i < 3; i++)
+		ret = min(ret, __hpet_calibrate(hpetp));
+
+	return ret;
+}
+
 int hpet_alloc(struct hpet_data *hdp)
 {
 	u64 cap, mcfg;

-- 
Yasunori Goto