[Qemu-devel] [PATCH] qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().

[Qemu-devel] [PATCH] qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().

[sohailalvi2236]

From: Sohail Alvi <sohailalvi2236@gmail.com>

---
 hw/arm/musicpal.c        | 3 ++-
 hw/ppc/ppc.c             | 3 ++-
 hw/s390x/ipl.c           | 3 ++-
 hw/timer/etraxfs_timer.c | 3 ++-
 hw/timer/m48t59.c        | 3 ++-
 hw/timer/pxa2xx_timer.c  | 3 ++-
 6 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 93ec3c5698..84593e61cc 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -28,6 +28,7 @@
 #include "sysemu/block-backend.h"
 #include "exec/address-spaces.h"
 #include "ui/pixel_ops.h"
+#include "sysemu/watchdog.h"
 
 #define MP_MISC_BASE            0x80002000
 #define MP_MISC_SIZE            0x00001000
@@ -898,7 +899,7 @@ static void mv88w8618_pit_write(void *opaque, hwaddr offset,
 
     case MP_BOARD_RESET:
         if (value == MP_BOARD_RESET_MAGIC) {
-            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            watchdog_perform_action();
         }
         break;
     }
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index ad20584f26..636d2046f8 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -35,6 +35,7 @@
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
 #include "trace.h"
+#include "sysemu/watchdog.h"
 
 //#define PPC_DEBUG_IRQ
 //#define PPC_DEBUG_TB
@@ -380,7 +381,7 @@ void ppc40x_chip_reset(PowerPCCPU *cpu)
 void ppc40x_system_reset(PowerPCCPU *cpu)
 {
     qemu_log_mask(CPU_LOG_RESET, "Reset PowerPC system\n");
-    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+    watchdog_perform_action();
 }
 
 void store_40x_dbcr0(CPUPPCState *env, uint32_t val)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 51b272e190..4783b41c57 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -27,6 +27,7 @@
 #include "qemu/cutils.h"
 #include "qemu/option.h"
 #include "exec/exec-all.h"
+#include "sysemu/watchdog.h"
 
 #define KERN_IMAGE_START                0x010000UL
 #define LINUX_MAGIC_ADDR                0x010008UL
@@ -547,7 +548,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
         /* ignore -no-reboot, send no event  */
         qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
     } else {
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
     }
     /* as this is triggered by a CPU, make sure to exit the loop */
     if (tcg_enabled()) {
diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
index 2280914b1d..9561113d0e 100644
--- a/hw/timer/etraxfs_timer.c
+++ b/hw/timer/etraxfs_timer.c
@@ -26,6 +26,7 @@
 #include "sysemu/sysemu.h"
 #include "qemu/timer.h"
 #include "hw/ptimer.h"
+#include "sysemu/watchdog.h"
 
 #define D(x)
 
@@ -207,7 +208,7 @@ static void watchdog_hit(void *opaque)
         qemu_irq_raise(t->nmi);
     }
     else
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
 
     t->wd_hits++;
 }
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index ca3ed445de..ebe58e8dc7 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -30,6 +30,7 @@
 #include "hw/sysbus.h"
 #include "exec/address-spaces.h"
 #include "qemu/bcd.h"
+#include "sysemu/watchdog.h"
 
 #include "m48t59-internal.h"
 
@@ -158,7 +159,7 @@ static void watchdog_cb (void *opaque)
 	NVRAM->buffer[0x1FF7] = 0x00;
 	NVRAM->buffer[0x1FFC] &= ~0x40;
         /* May it be a hw CPU Reset instead ? */
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
     } else {
 	qemu_set_irq(NVRAM->IRQ, 1);
 	qemu_set_irq(NVRAM->IRQ, 0);
diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
index a489bf5159..2be680b5df 100644
--- a/hw/timer/pxa2xx_timer.c
+++ b/hw/timer/pxa2xx_timer.c
@@ -14,6 +14,7 @@
 #include "hw/arm/pxa.h"
 #include "hw/sysbus.h"
 #include "qemu/log.h"
+#include "sysemu/watchdog.h"
 
 #define OSMR0	0x00
 #define OSMR1	0x04
@@ -414,7 +415,7 @@ static void pxa2xx_timer_tick(void *opaque)
     if (t->num == 3)
         if (i->reset3 & 1) {
             i->reset3 = 0;
-            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            watchdog_perform_action();
         }
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH] qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().

[sohailalvi2236]

From: Sohail Alvi <sohailalvi2236@gmail.com>

---
 hw/arm/musicpal.c        | 3 ++-
 hw/ppc/ppc.c             | 3 ++-
 hw/s390x/ipl.c           | 3 ++-
 hw/timer/etraxfs_timer.c | 3 ++-
 hw/timer/m48t59.c        | 3 ++-
 hw/timer/pxa2xx_timer.c  | 3 ++-
 6 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 93ec3c5698..84593e61cc 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -28,6 +28,7 @@
 #include "sysemu/block-backend.h"
 #include "exec/address-spaces.h"
 #include "ui/pixel_ops.h"
+#include "sysemu/watchdog.h"
 
 #define MP_MISC_BASE            0x80002000
 #define MP_MISC_SIZE            0x00001000
@@ -898,7 +899,7 @@ static void mv88w8618_pit_write(void *opaque, hwaddr offset,
 
     case MP_BOARD_RESET:
         if (value == MP_BOARD_RESET_MAGIC) {
-            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            watchdog_perform_action();
         }
         break;
     }
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index ad20584f26..636d2046f8 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -35,6 +35,7 @@
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
 #include "trace.h"
+#include "sysemu/watchdog.h"
 
 //#define PPC_DEBUG_IRQ
 //#define PPC_DEBUG_TB
@@ -380,7 +381,7 @@ void ppc40x_chip_reset(PowerPCCPU *cpu)
 void ppc40x_system_reset(PowerPCCPU *cpu)
 {
     qemu_log_mask(CPU_LOG_RESET, "Reset PowerPC system\n");
-    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+    watchdog_perform_action();
 }
 
 void store_40x_dbcr0(CPUPPCState *env, uint32_t val)
diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 51b272e190..4783b41c57 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -27,6 +27,7 @@
 #include "qemu/cutils.h"
 #include "qemu/option.h"
 #include "exec/exec-all.h"
+#include "sysemu/watchdog.h"
 
 #define KERN_IMAGE_START                0x010000UL
 #define LINUX_MAGIC_ADDR                0x010008UL
@@ -547,7 +548,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
         /* ignore -no-reboot, send no event  */
         qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
     } else {
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
     }
     /* as this is triggered by a CPU, make sure to exit the loop */
     if (tcg_enabled()) {
diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
index 2280914b1d..9561113d0e 100644
--- a/hw/timer/etraxfs_timer.c
+++ b/hw/timer/etraxfs_timer.c
@@ -26,6 +26,7 @@
 #include "sysemu/sysemu.h"
 #include "qemu/timer.h"
 #include "hw/ptimer.h"
+#include "sysemu/watchdog.h"
 
 #define D(x)
 
@@ -207,7 +208,7 @@ static void watchdog_hit(void *opaque)
         qemu_irq_raise(t->nmi);
     }
     else
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
 
     t->wd_hits++;
 }
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index ca3ed445de..ebe58e8dc7 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -30,6 +30,7 @@
 #include "hw/sysbus.h"
 #include "exec/address-spaces.h"
 #include "qemu/bcd.h"
+#include "sysemu/watchdog.h"
 
 #include "m48t59-internal.h"
 
@@ -158,7 +159,7 @@ static void watchdog_cb (void *opaque)
 	NVRAM->buffer[0x1FF7] = 0x00;
 	NVRAM->buffer[0x1FFC] &= ~0x40;
         /* May it be a hw CPU Reset instead ? */
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
     } else {
 	qemu_set_irq(NVRAM->IRQ, 1);
 	qemu_set_irq(NVRAM->IRQ, 0);
diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
index a489bf5159..2be680b5df 100644
--- a/hw/timer/pxa2xx_timer.c
+++ b/hw/timer/pxa2xx_timer.c
@@ -14,6 +14,7 @@
 #include "hw/arm/pxa.h"
 #include "hw/sysbus.h"
 #include "qemu/log.h"
+#include "sysemu/watchdog.h"
 
 #define OSMR0	0x00
 #define OSMR1	0x04
@@ -414,7 +415,7 @@ static void pxa2xx_timer_tick(void *opaque)
     if (t->num == 3)
         if (i->reset3 & 1) {
             i->reset3 = 0;
-            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            watchdog_perform_action();
         }
 }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH] qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().

[Peter Maydell]

On Mon, 8 Apr 2019 at 15:59, <sohailalvi2236@gmail.com> wrote:
>
> From: Sohail Alvi <sohailalvi2236@gmail.com>
>
> ---
>  hw/arm/musicpal.c        | 3 ++-
>  hw/ppc/ppc.c             | 3 ++-
>  hw/s390x/ipl.c           | 3 ++-
>  hw/timer/etraxfs_timer.c | 3 ++-
>  hw/timer/m48t59.c        | 3 ++-
>  hw/timer/pxa2xx_timer.c  | 3 ++-
>  6 files changed, 12 insertions(+), 6 deletions(-)

Hi; thanks for this patch. Unfortunately I think you've
misunderstood the suggestion from the bitesize tasks page.
(This is our fault, because the bitesize tasks list
has accumulated a lot of tasks that really require
some understanding of exactly what is being done, but
don't make it clear in their description.)

In this case the key phrase is "corresponding to a
watchdog that has triggered". Not all calls to
qemu_system_reset_request() are calls made by
watchdog devices when their timer has triggered.
If the call is not a watchdog timer timeout then
it doesn't need changing. Some of the places you've
changed here are watchdog timeouts, but some are not.

I would also suggest that you have one patch per
device you're changing -- all of these devices
are parts of different machines which have different
guest CPU architectures. That means that the people
who can review and test the change are different
for each device. Also if we have a patch per device
it means that if there turns out to be a problem
with the change for one of the devices it's easy
to back that change out without affecting the others.

I would also suggest that your commit message should
have just a short description in the Subject line,
and a more detailed description in the main body
of the commit message. You can look at some of
the commit messages in our git history to get an
idea of our preferred style.

> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 93ec3c5698..84593e61cc 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/block-backend.h"
>  #include "exec/address-spaces.h"
>  #include "ui/pixel_ops.h"
> +#include "sysemu/watchdog.h"
>
>  #define MP_MISC_BASE            0x80002000
>  #define MP_MISC_SIZE            0x00001000
> @@ -898,7 +899,7 @@ static void mv88w8618_pit_write(void *opaque, hwaddr offset,
>
>      case MP_BOARD_RESET:
>          if (value == MP_BOARD_RESET_MAGIC) {
> -            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +            watchdog_perform_action();
>          }

This isn't a watchdog timeout -- it's just a device
register that allows the guest to request a board
reset.

>          break;
>      }
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index ad20584f26..636d2046f8 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -35,6 +35,7 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_ppc.h"
>  #include "trace.h"
> +#include "sysemu/watchdog.h"
>
>  //#define PPC_DEBUG_IRQ
>  //#define PPC_DEBUG_TB
> @@ -380,7 +381,7 @@ void ppc40x_chip_reset(PowerPCCPU *cpu)
>  void ppc40x_system_reset(PowerPCCPU *cpu)
>  {
>      qemu_log_mask(CPU_LOG_RESET, "Reset PowerPC system\n");
> -    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +    watchdog_perform_action();
>  }

This isn't a watchdog either.

>
>  void store_40x_dbcr0(CPUPPCState *env, uint32_t val)
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 51b272e190..4783b41c57 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -27,6 +27,7 @@
>  #include "qemu/cutils.h"
>  #include "qemu/option.h"
>  #include "exec/exec-all.h"
> +#include "sysemu/watchdog.h"
>
>  #define KERN_IMAGE_START                0x010000UL
>  #define LINUX_MAGIC_ADDR                0x010008UL
> @@ -547,7 +548,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
>          /* ignore -no-reboot, send no event  */
>          qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
>      } else {
> -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        watchdog_perform_action();

Nor is this.

>      }
>      /* as this is triggered by a CPU, make sure to exit the loop */
>      if (tcg_enabled()) {
> diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
> index 2280914b1d..9561113d0e 100644
> --- a/hw/timer/etraxfs_timer.c
> +++ b/hw/timer/etraxfs_timer.c
> @@ -26,6 +26,7 @@
>  #include "sysemu/sysemu.h"
>  #include "qemu/timer.h"
>  #include "hw/ptimer.h"
> +#include "sysemu/watchdog.h"
>
>  #define D(x)
>
> @@ -207,7 +208,7 @@ static void watchdog_hit(void *opaque)
>          qemu_irq_raise(t->nmi);
>      }
>      else
> -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        watchdog_perform_action();

This one really is a watchdog timeout, so this change
is OK. (If you run your patch through scripts/checkpatch.pl
you'll find that checkpatch advises adding some braces to
this if statement, though.)


>      t->wd_hits++;
>  }
> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
> index ca3ed445de..ebe58e8dc7 100644
> --- a/hw/timer/m48t59.c
> +++ b/hw/timer/m48t59.c
> @@ -30,6 +30,7 @@
>  #include "hw/sysbus.h"
>  #include "exec/address-spaces.h"
>  #include "qemu/bcd.h"
> +#include "sysemu/watchdog.h"
>
>  #include "m48t59-internal.h"
>
> @@ -158,7 +159,7 @@ static void watchdog_cb (void *opaque)
>         NVRAM->buffer[0x1FF7] = 0x00;
>         NVRAM->buffer[0x1FFC] &= ~0x40;
>          /* May it be a hw CPU Reset instead ? */
> -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        watchdog_perform_action();
>      } else {
>         qemu_set_irq(NVRAM->IRQ, 1);
>         qemu_set_irq(NVRAM->IRQ, 0);

Yes, this is a watchdog.

> diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
> index a489bf5159..2be680b5df 100644
> --- a/hw/timer/pxa2xx_timer.c
> +++ b/hw/timer/pxa2xx_timer.c
> @@ -14,6 +14,7 @@
>  #include "hw/arm/pxa.h"
>  #include "hw/sysbus.h"
>  #include "qemu/log.h"
> +#include "sysemu/watchdog.h"
>
>  #define OSMR0  0x00
>  #define OSMR1  0x04
> @@ -414,7 +415,7 @@ static void pxa2xx_timer_tick(void *opaque)
>      if (t->num == 3)
>          if (i->reset3 & 1) {
>              i->reset3 = 0;
> -            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +            watchdog_perform_action();
>          }

This is a watchdog too.

>  }
>
> --
> 2.17.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH] qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().

[Peter Maydell]

On Mon, 8 Apr 2019 at 15:59, <sohailalvi2236@gmail.com> wrote:
>
> From: Sohail Alvi <sohailalvi2236@gmail.com>
>
> ---
>  hw/arm/musicpal.c        | 3 ++-
>  hw/ppc/ppc.c             | 3 ++-
>  hw/s390x/ipl.c           | 3 ++-
>  hw/timer/etraxfs_timer.c | 3 ++-
>  hw/timer/m48t59.c        | 3 ++-
>  hw/timer/pxa2xx_timer.c  | 3 ++-
>  6 files changed, 12 insertions(+), 6 deletions(-)

Hi; thanks for this patch. Unfortunately I think you've
misunderstood the suggestion from the bitesize tasks page.
(This is our fault, because the bitesize tasks list
has accumulated a lot of tasks that really require
some understanding of exactly what is being done, but
don't make it clear in their description.)

In this case the key phrase is "corresponding to a
watchdog that has triggered". Not all calls to
qemu_system_reset_request() are calls made by
watchdog devices when their timer has triggered.
If the call is not a watchdog timer timeout then
it doesn't need changing. Some of the places you've
changed here are watchdog timeouts, but some are not.

I would also suggest that you have one patch per
device you're changing -- all of these devices
are parts of different machines which have different
guest CPU architectures. That means that the people
who can review and test the change are different
for each device. Also if we have a patch per device
it means that if there turns out to be a problem
with the change for one of the devices it's easy
to back that change out without affecting the others.

I would also suggest that your commit message should
have just a short description in the Subject line,
and a more detailed description in the main body
of the commit message. You can look at some of
the commit messages in our git history to get an
idea of our preferred style.

> diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
> index 93ec3c5698..84593e61cc 100644
> --- a/hw/arm/musicpal.c
> +++ b/hw/arm/musicpal.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/block-backend.h"
>  #include "exec/address-spaces.h"
>  #include "ui/pixel_ops.h"
> +#include "sysemu/watchdog.h"
>
>  #define MP_MISC_BASE            0x80002000
>  #define MP_MISC_SIZE            0x00001000
> @@ -898,7 +899,7 @@ static void mv88w8618_pit_write(void *opaque, hwaddr offset,
>
>      case MP_BOARD_RESET:
>          if (value == MP_BOARD_RESET_MAGIC) {
> -            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +            watchdog_perform_action();
>          }

This isn't a watchdog timeout -- it's just a device
register that allows the guest to request a board
reset.

>          break;
>      }
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index ad20584f26..636d2046f8 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -35,6 +35,7 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_ppc.h"
>  #include "trace.h"
> +#include "sysemu/watchdog.h"
>
>  //#define PPC_DEBUG_IRQ
>  //#define PPC_DEBUG_TB
> @@ -380,7 +381,7 @@ void ppc40x_chip_reset(PowerPCCPU *cpu)
>  void ppc40x_system_reset(PowerPCCPU *cpu)
>  {
>      qemu_log_mask(CPU_LOG_RESET, "Reset PowerPC system\n");
> -    qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +    watchdog_perform_action();
>  }

This isn't a watchdog either.

>
>  void store_40x_dbcr0(CPUPPCState *env, uint32_t val)
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 51b272e190..4783b41c57 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -27,6 +27,7 @@
>  #include "qemu/cutils.h"
>  #include "qemu/option.h"
>  #include "exec/exec-all.h"
> +#include "sysemu/watchdog.h"
>
>  #define KERN_IMAGE_START                0x010000UL
>  #define LINUX_MAGIC_ADDR                0x010008UL
> @@ -547,7 +548,7 @@ void s390_ipl_reset_request(CPUState *cs, enum s390_reset reset_type)
>          /* ignore -no-reboot, send no event  */
>          qemu_system_reset_request(SHUTDOWN_CAUSE_SUBSYSTEM_RESET);
>      } else {
> -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        watchdog_perform_action();

Nor is this.

>      }
>      /* as this is triggered by a CPU, make sure to exit the loop */
>      if (tcg_enabled()) {
> diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
> index 2280914b1d..9561113d0e 100644
> --- a/hw/timer/etraxfs_timer.c
> +++ b/hw/timer/etraxfs_timer.c
> @@ -26,6 +26,7 @@
>  #include "sysemu/sysemu.h"
>  #include "qemu/timer.h"
>  #include "hw/ptimer.h"
> +#include "sysemu/watchdog.h"
>
>  #define D(x)
>
> @@ -207,7 +208,7 @@ static void watchdog_hit(void *opaque)
>          qemu_irq_raise(t->nmi);
>      }
>      else
> -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        watchdog_perform_action();

This one really is a watchdog timeout, so this change
is OK. (If you run your patch through scripts/checkpatch.pl
you'll find that checkpatch advises adding some braces to
this if statement, though.)


>      t->wd_hits++;
>  }
> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
> index ca3ed445de..ebe58e8dc7 100644
> --- a/hw/timer/m48t59.c
> +++ b/hw/timer/m48t59.c
> @@ -30,6 +30,7 @@
>  #include "hw/sysbus.h"
>  #include "exec/address-spaces.h"
>  #include "qemu/bcd.h"
> +#include "sysemu/watchdog.h"
>
>  #include "m48t59-internal.h"
>
> @@ -158,7 +159,7 @@ static void watchdog_cb (void *opaque)
>         NVRAM->buffer[0x1FF7] = 0x00;
>         NVRAM->buffer[0x1FFC] &= ~0x40;
>          /* May it be a hw CPU Reset instead ? */
> -        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +        watchdog_perform_action();
>      } else {
>         qemu_set_irq(NVRAM->IRQ, 1);
>         qemu_set_irq(NVRAM->IRQ, 0);

Yes, this is a watchdog.

> diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
> index a489bf5159..2be680b5df 100644
> --- a/hw/timer/pxa2xx_timer.c
> +++ b/hw/timer/pxa2xx_timer.c
> @@ -14,6 +14,7 @@
>  #include "hw/arm/pxa.h"
>  #include "hw/sysbus.h"
>  #include "qemu/log.h"
> +#include "sysemu/watchdog.h"
>
>  #define OSMR0  0x00
>  #define OSMR1  0x04
> @@ -414,7 +415,7 @@ static void pxa2xx_timer_tick(void *opaque)
>      if (t->num == 3)
>          if (i->reset3 & 1) {
>              i->reset3 = 0;
> -            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +            watchdog_perform_action();
>          }

This is a watchdog too.

>  }
>
> --
> 2.17.1

thanks
-- PMM

[Qemu-devel] [PATCH 02/5] hw/timer: qemu_system_reset() replaced by watchdog_perform_action()

[sohailalvi2236]

From: Sohail Alvi <sohailalvi2236@gmail.com>

Signed-off-by: SohailAlvi <sohailalvi2236@gmail.com>

The watchdog_perform_action() function has been added in place of qemu_system_reset where watchdog was triggered.
Changes have been made according to the previous suggestions given by Peter
Maydell.

The patch was tested with scripts/checkpatch.pl and no style errors were
found.
---
 hw/timer/etraxfs_timer.c | 3 ++-
 hw/timer/m48t59.c        | 3 ++-
 hw/timer/pxa2xx_timer.c  | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
index 2280914b1d..9561113d0e 100644
--- a/hw/timer/etraxfs_timer.c
+++ b/hw/timer/etraxfs_timer.c
@@ -26,6 +26,7 @@
 #include "sysemu/sysemu.h"
 #include "qemu/timer.h"
 #include "hw/ptimer.h"
+#include "sysemu/watchdog.h"
 
 #define D(x)
 
@@ -207,7 +208,7 @@ static void watchdog_hit(void *opaque)
         qemu_irq_raise(t->nmi);
     }
     else
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
 
     t->wd_hits++;
 }
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index ca3ed445de..ebe58e8dc7 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -30,6 +30,7 @@
 #include "hw/sysbus.h"
 #include "exec/address-spaces.h"
 #include "qemu/bcd.h"
+#include "sysemu/watchdog.h"
 
 #include "m48t59-internal.h"
 
@@ -158,7 +159,7 @@ static void watchdog_cb (void *opaque)
 	NVRAM->buffer[0x1FF7] = 0x00;
 	NVRAM->buffer[0x1FFC] &= ~0x40;
         /* May it be a hw CPU Reset instead ? */
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
     } else {
 	qemu_set_irq(NVRAM->IRQ, 1);
 	qemu_set_irq(NVRAM->IRQ, 0);
diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
index a489bf5159..2be680b5df 100644
--- a/hw/timer/pxa2xx_timer.c
+++ b/hw/timer/pxa2xx_timer.c
@@ -14,6 +14,7 @@
 #include "hw/arm/pxa.h"
 #include "hw/sysbus.h"
 #include "qemu/log.h"
+#include "sysemu/watchdog.h"
 
 #define OSMR0	0x00
 #define OSMR1	0x04
@@ -414,7 +415,7 @@ static void pxa2xx_timer_tick(void *opaque)
     if (t->num == 3)
         if (i->reset3 & 1) {
             i->reset3 = 0;
-            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            watchdog_perform_action();
         }
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH 02/5] hw/timer: qemu_system_reset() replaced by watchdog_perform_action()

[sohailalvi2236]

From: Sohail Alvi <sohailalvi2236@gmail.com>

Signed-off-by: SohailAlvi <sohailalvi2236@gmail.com>

The watchdog_perform_action() function has been added in place of qemu_system_reset where watchdog was triggered.
Changes have been made according to the previous suggestions given by Peter
Maydell.

The patch was tested with scripts/checkpatch.pl and no style errors were
found.
---
 hw/timer/etraxfs_timer.c | 3 ++-
 hw/timer/m48t59.c        | 3 ++-
 hw/timer/pxa2xx_timer.c  | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/timer/etraxfs_timer.c b/hw/timer/etraxfs_timer.c
index 2280914b1d..9561113d0e 100644
--- a/hw/timer/etraxfs_timer.c
+++ b/hw/timer/etraxfs_timer.c
@@ -26,6 +26,7 @@
 #include "sysemu/sysemu.h"
 #include "qemu/timer.h"
 #include "hw/ptimer.h"
+#include "sysemu/watchdog.h"
 
 #define D(x)
 
@@ -207,7 +208,7 @@ static void watchdog_hit(void *opaque)
         qemu_irq_raise(t->nmi);
     }
     else
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
 
     t->wd_hits++;
 }
diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
index ca3ed445de..ebe58e8dc7 100644
--- a/hw/timer/m48t59.c
+++ b/hw/timer/m48t59.c
@@ -30,6 +30,7 @@
 #include "hw/sysbus.h"
 #include "exec/address-spaces.h"
 #include "qemu/bcd.h"
+#include "sysemu/watchdog.h"
 
 #include "m48t59-internal.h"
 
@@ -158,7 +159,7 @@ static void watchdog_cb (void *opaque)
 	NVRAM->buffer[0x1FF7] = 0x00;
 	NVRAM->buffer[0x1FFC] &= ~0x40;
         /* May it be a hw CPU Reset instead ? */
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+        watchdog_perform_action();
     } else {
 	qemu_set_irq(NVRAM->IRQ, 1);
 	qemu_set_irq(NVRAM->IRQ, 0);
diff --git a/hw/timer/pxa2xx_timer.c b/hw/timer/pxa2xx_timer.c
index a489bf5159..2be680b5df 100644
--- a/hw/timer/pxa2xx_timer.c
+++ b/hw/timer/pxa2xx_timer.c
@@ -14,6 +14,7 @@
 #include "hw/arm/pxa.h"
 #include "hw/sysbus.h"
 #include "qemu/log.h"
+#include "sysemu/watchdog.h"
 
 #define OSMR0	0x00
 #define OSMR1	0x04
@@ -414,7 +415,7 @@ static void pxa2xx_timer_tick(void *opaque)
     if (t->num == 3)
         if (i->reset3 & 1) {
             i->reset3 = 0;
-            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            watchdog_perform_action();
         }
 }
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH] qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190408145721.15881-1-sohailalvi2236@gmail.com/



Hi,

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

Message-id: 20190408145721.15881-1-sohailalvi2236@gmail.com
Subject: [Qemu-devel] [PATCH] qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   f55a585d10..2c57310627  master     -> master
 * [new tag]               patchew/20190408145721.15881-1-sohailalvi2236@gmail.com -> patchew/20190408145721.15881-1-sohailalvi2236@gmail.com
Switched to a new branch 'test'
05686f9b91 qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().

=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 90 lines checked

Commit 05686f9b910d (qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().) 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


The full log is available at
http://patchew.org/logs/20190408145721.15881-1-sohailalvi2236@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190408145721.15881-1-sohailalvi2236@gmail.com/



Hi,

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

Message-id: 20190408145721.15881-1-sohailalvi2236@gmail.com
Subject: [Qemu-devel] [PATCH] qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   f55a585d10..2c57310627  master     -> master
 * [new tag]               patchew/20190408145721.15881-1-sohailalvi2236@gmail.com -> patchew/20190408145721.15881-1-sohailalvi2236@gmail.com
Switched to a new branch 'test'
05686f9b91 qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().

=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 90 lines checked

Commit 05686f9b910d (qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action().) 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


The full log is available at
http://patchew.org/logs/20190408145721.15881-1-sohailalvi2236@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com