| Message ID | 20190408145721.15881-1-sohailalvi2236@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu_system_reset_request() in hw/ corresponding to watchdog that has triggered replaced by watchdog_perform_action(). | expand |
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
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
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(); } }
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(-)