[Qemu-devel] [PATCH] ppc: Prevent inifnite loop in decrementer auto-reload.

[Qemu-devel] [PATCH] ppc: Prevent inifnite loop in decrementer auto-reload.

[Roman Kapl]

If the DECAR register is set to 0, QEMU tries to reload the decrementer with
zero in an inifinite loop. According to PPC documentation, the decrementer is
triggered on 1->0 transition, so avoid reloading the decrementer if if is
already zero.

The problem does not manifest under Linux, but it is valid to set DECAR to zero
(and may make sense as part of decrementer initialization when interrupts are
disabled).

Signed-off-by: Roman Kapl <rka@sysgo.com>
---
 hw/ppc/ppc_booke.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
index ab8d026..f8d5c28 100644
--- a/hw/ppc/ppc_booke.c
+++ b/hw/ppc/ppc_booke.c
@@ -198,8 +198,12 @@ static void booke_decr_cb(void *opaque)
     booke_update_irq(cpu);
 
     if (env->spr[SPR_BOOKE_TCR] & TCR_ARE) {
-        /* Auto Reload */
-        cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
+        /* Do not reload 0, it is already there. It would just trigger
+         * the timer again and lead to infinite loop */
+        if(env->spr[SPR_BOOKE_DECAR] != 0) {
+            /* Auto Reload */
+            cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
+        }
     }
 }
 
-- 
2.10.1

Re: [Qemu-devel] [PATCH] ppc: Prevent inifnite loop in decrementer auto-reload.

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Message-id: 20170109112338.5629-1-rka@sysgo.com
Type: series
Subject: [Qemu-devel] [PATCH] ppc: Prevent inifnite loop in decrementer auto-reload.

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20170109112338.5629-1-rka@sysgo.com -> patchew/20170109112338.5629-1-rka@sysgo.com
Switched to a new branch 'test'
571ca4b ppc: Prevent inifnite loop in decrementer auto-reload.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: ppc: Prevent inifnite loop in decrementer auto-reload....
ERROR: space required before the open parenthesis '('
#30: FILE: hw/ppc/ppc_booke.c:203:
+        if(env->spr[SPR_BOOKE_DECAR] != 0) {

total: 1 errors, 0 warnings, 14 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

[Qemu-devel] [PATCH v2] ppc: Prevent inifnite loop in decrementer auto-reload.

[Roman Kapl]

If the DECAR register is set to 0, QEMU tries to reload the decrementer with
zero in an inifinite loop. According to PPC documentation, the decrementer is
triggered on 1->0 transition, so avoid reloading the decrementer if if is
already zero.

The problem does not manifest under Linux, but it is valid to set DECAR to zero
(and may make sense as part of decrementer initialization when interrupts are
disabled).

Signed-off-by: Roman Kapl <rka@sysgo.com>
---
 hw/ppc/ppc_booke.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
index ab8d026..60baffa 100644
--- a/hw/ppc/ppc_booke.c
+++ b/hw/ppc/ppc_booke.c
@@ -198,8 +198,12 @@ static void booke_decr_cb(void *opaque)
     booke_update_irq(cpu);
 
     if (env->spr[SPR_BOOKE_TCR] & TCR_ARE) {
-        /* Auto Reload */
-        cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
+        /* Do not reload 0, it is already there. It would just trigger
+         * the timer again and lead to infinite loop */
+        if (env->spr[SPR_BOOKE_DECAR] != 0) {
+            /* Auto Reload */
+            cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
+        }
     }
 }
 
-- 
2.10.1

Re: [Qemu-devel] [PATCH v2] ppc: Prevent inifnite loop in decrementer auto-reload.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

On 01/09/2017 07:07 AM, Roman Kapl wrote:

s/inifnite/infinite/ in the subject

> If the DECAR register is set to 0, QEMU tries to reload the decrementer with
> zero in an inifinite loop. According to PPC documentation, the decrementer is

s/inifinite/infinite/

> triggered on 1->0 transition, so avoid reloading the decrementer if if is
> already zero.
> 
> The problem does not manifest under Linux, but it is valid to set DECAR to zero
> (and may make sense as part of decrementer initialization when interrupts are
> disabled).

When sending a v2, it's best to do it as a new top-level thread rather
than in-reply-to v1, as some patch tools overlook deeply nested replies.
 More hints on sending patches:
http://wiki.qemu.org/Contribute/SubmitAPatch

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] ppc: Prevent inifnite loop in decrementer auto-reload.

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

On Mon, Jan 09, 2017 at 12:23:38PM +0100, Roman Kapl wrote:
> If the DECAR register is set to 0, QEMU tries to reload the decrementer with
> zero in an inifinite loop. According to PPC documentation, the decrementer is
> triggered on 1->0 transition, so avoid reloading the decrementer if if is
> already zero.
> 
> The problem does not manifest under Linux, but it is valid to set DECAR to zero
> (and may make sense as part of decrementer initialization when interrupts are
> disabled).
> 
> Signed-off-by: Roman Kapl <rka@sysgo.com>

Applied, fixing the coding style nit (no space after if) in the
process.  Please remember to run checkpatch.pl in future.

> ---
>  hw/ppc/ppc_booke.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
> index ab8d026..f8d5c28 100644
> --- a/hw/ppc/ppc_booke.c
> +++ b/hw/ppc/ppc_booke.c
> @@ -198,8 +198,12 @@ static void booke_decr_cb(void *opaque)
>      booke_update_irq(cpu);
>  
>      if (env->spr[SPR_BOOKE_TCR] & TCR_ARE) {
> -        /* Auto Reload */
> -        cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
> +        /* Do not reload 0, it is already there. It would just trigger
> +         * the timer again and lead to infinite loop */
> +        if(env->spr[SPR_BOOKE_DECAR] != 0) {
> +            /* Auto Reload */
> +            cpu_ppc_store_decr(env, env->spr[SPR_BOOKE_DECAR]);
> +        }
>      }
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]