qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for


From: Alexander Graf
Subject: Re: [Qemu-ppc] [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for target_bit above 61
Date: Tue, 11 Jun 2013 14:56:38 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.3) Gecko/20120306 Thunderbird/10.0.3

On 06/11/2013 02:47 PM, Bhushan Bharat-R65777 wrote:

-----Original Message-----
From: Alexander Graf [mailto:address@hidden
Sent: Tuesday, June 11, 2013 6:10 PM
To: Bhushan Bharat-R65777
Cc: Wood Scott-B07421; Andreas Färber; address@hidden; qemu-
address@hidden
Subject: Re: [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer for
target_bit above 61

On 06/11/2013 01:40 PM, Bhushan Bharat-R65777 wrote:
-----Original Message-----
From: Alexander Graf [mailto:address@hidden
Sent: Monday, June 10, 2013 11:40 PM
To: Wood Scott-B07421
Cc: Bhushan Bharat-R65777; Andreas Färber; address@hidden; qemu-
address@hidden; Wood Scott-B07421
Subject: Re: [Qemu-devel] [PATCH v2]booke: timer: Deactivate timer
for target_bit above 61


On 10.06.2013, at 19:20, Scott Wood wrote:

On 06/10/2013 09:26:18 AM, Alexander Graf wrote:
On 06/10/2013 02:47 PM, Bhushan Bharat-R65777 wrote:
-----Original Message-----
From: Andreas Färber [mailto:address@hidden
Sent: Monday, June 10, 2013 5:43 PM
To: Bhushan Bharat-R65777
Cc: address@hidden; address@hidden; address@hidden;
Wood
Scott- B07421; Bhushan Bharat-R65777
Subject: Re: [Qemu-devel] [PATCH v2]booke: timer: Deactivate
timer for target_bit above 61 So IIUC we can only allow 63 bits due to
signedness, thus a maximum of (1<<    62), thus target_bit<= 61.
Any chance at least the comment can be worded to explain that any
better? Maybe also use (target-bit + 1>= 63) or period>    INT64_MAX as
condition?
How about this:
      /* QEMU timer supports a maximum timer of INT64_MAX
(0x7fffffff_ffffffff).
       * Run booke fit/wdog timer when
       * ((1ULL<<    target_bit + 1)<    0x40000000_00000000), i.e target_bit
=
61.
       * Also the time with this maximum target_bit (with current range of
       * CPU frequency PowerPC supports) will be many many years. So it is
       * pretty safe to stop the timer above this threshold. */
How about
   /* This timeout will take years to trigger. Treat the timer as
disabled. */
There should be at least a brief mention that it's because the QEMU
timer can't handle larger values,
If it can't handle higher values, maybe it's better to just set the
timer value to INT64_MAX when we detect an overflow? That would make
the code plainly obvious.

What about below comment (a mix of both :)):

      /* Timeout calculated with (target_bit + 1)>   62 will take
       * hundreds of years to trigger. Treat the timer as disabled.
       * Also this timeout is within the qemu supported maximum
       * timeout limit (INT64_MAX.). */
Ok, next question: Why does return disable the timer?
Actually here disabled means _not_ starting the timer. This function will be called to 
start timer initially and then later it is called to restart after every expiry. If we do 
not start then it is as good as stopped/disabled (it is not disabled in TCR). Probably 
saying "do not start qemu timer" or something similar is better than saying 
disabling the timer.

Couldn't you simply make things obvious from the code flow without pulling up assumptions?

Something along the lines of

if (<overflow>) {
    *next = INT64_MAX;
}

qemu_mod_timer(timer, *next);

Then everyone knows what's going on, we can always assume the timer is running and there's no need to understand complex corner cases. It feels more like the timer framework would be the one to decid to ignore timeouts that take years to finish.


Alex




reply via email to

[Prev in Thread] Current Thread [Next in Thread]