| Message ID | 1432732141-33915-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 27, 2015 at 03:09:01PM +0200, Paolo Bonzini wrote: > This makes it possible to specify a watchdog action in a configuration file. > The previous behavior of "-watchdog" is moved to the (implied) "-watchdog > model" suboption. This is already more or less obsolete, since it is possible > to achieve the same effect with "-device", but "-watchdog-action" does not have > an equivalent. > > One alternative implementation is possible, namely to add an "action" > property to each watchdog device. However, boards often have embedded > watchdog devices; even if they currently don't, these should call > watchdog_perform_action() so that they are affected by -watchdog-action. > (This is listed in our BiteSizedTasks wiki page). > > For these boards, "-watchdog action=foo" has two advantages: > > 1) it is much easier to use than a "-global" option, and can be > configured globally for all boards. > > 2) it is harder to add a property to a device than it is to just > s/qemu_system_reset_request/watchdog_perform_action/; in some cases, > the devices are not even qdev-ified at all. The chance of the conversion > happening then would basically be zero if one had to add a property as > a prerequisite. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tweak brackets in help messages [Eric] > fix missing error message for -watchdog action [Richard] > --- > docs/qdev-device-use.txt | 4 ++++ > qemu-options.hx | 37 +++++++++++++++-------------- > vl.c | 61 +++++++++++++++++++++++++++++++++++------------- > 3 files changed, 69 insertions(+), 33 deletions(-) > > diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt > index 136d271..1906b7d 100644 > --- a/docs/qdev-device-use.txt > +++ b/docs/qdev-device-use.txt > @@ -365,6 +365,10 @@ The old way to define a guest watchdog device is -watchdog DEVNAME. > The new way is -device DEVNAME. For PCI devices, you can add > bus=PCI-BUS,addr=DEVFN to control the PCI device address, as usual. > > +No matter if you use -watchdog or -device to define the guest watchdog > +device, you can specify the watchdog action with "-watchdog-action ACTION" > +or "-watchdog action=ACTION". > + > === Host Device Assignment === > > QEMU supports assigning host PCI devices (qemu-kvm only at this time) > diff --git a/qemu-options.hx b/qemu-options.hx > index ec356f6..628f282 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3115,12 +3115,20 @@ when the shift value is high (how high depends on the host machine). > ETEXI > > DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \ > - "-watchdog i6300esb|ib700\n" \ > - " enable virtual hardware watchdog [default=none]\n", > + "-watchdog [[model=]i6300esb|ib700]\n" \ > + " [,action=reset|shutdown|poweroff|pause|debug|none]\n" \ > + " enable virtual hardware watchdog (default: model=none,action=reset)\n", > + QEMU_ARCH_ALL) > +DEF("watchdog-action", HAS_ARG, QEMU_OPTION_watchdog_action, \ > + "-watchdog-action reset|shutdown|poweroff|pause|debug|none\n" \ > + " action when watchdog fires (default: reset)\n", > QEMU_ARCH_ALL) > STEXI > -@item -watchdog @var{model} > +@item -watchdog [[model=]@var{model}][,action=@var{action}] > @findex -watchdog > +@itemx -watchdog-action @var{action} > +@findex -watchdog-action > + > Create a virtual hardware watchdog device. Once enabled (by a guest > action), the watchdog must be periodically polled by an agent inside > the guest or else the guest will be restarted. > @@ -3129,22 +3137,16 @@ The @var{model} is the model of hardware watchdog to emulate. Choices > for model are: @code{ib700} (iBASE 700) which is a very simple ISA > watchdog with a single timer, or @code{i6300esb} (Intel 6300ESB I/O > controller hub) which is a much more featureful PCI-based dual-timer > -watchdog. Choose a model for which your guest has drivers. > - > -Use @code{-watchdog help} to list available hardware models. Only one > +watchdog. Choose a model for which your guest has drivers; > +use @code{-watchdog help} to list available hardware models. Only one > watchdog can be enabled for a guest. > -ETEXI > - > -DEF("watchdog-action", HAS_ARG, QEMU_OPTION_watchdog_action, \ > - "-watchdog-action reset|shutdown|poweroff|pause|debug|none\n" \ > - " action when watchdog fires [default=reset]\n", > - QEMU_ARCH_ALL) > -STEXI > -@item -watchdog-action @var{action} > -@findex -watchdog-action > > The @var{action} controls what QEMU will do when the watchdog timer > -expires. > +expires. It can be specified directly in the @option{-watchdog} option, > +or as the argument of a separate @option{-watchdog-action} parameter. > +The effect is the same. The watchdog action can be specified even if > +the watchdog is enabled through the @option{-device} option. > + > The default is > @code{reset} (forcefully reset the guest). > Other possible actions are: > @@ -3162,8 +3164,9 @@ situations where the watchdog would have expired, and thus > Examples: > > @table @code > -@item -watchdog i6300esb -watchdog-action pause > +@item -watchdog i6300esb,action=pause > @item -watchdog ib700 > +@item -device ib700 -watchdog action=poweroff > @end table > ETEXI > > diff --git a/vl.c b/vl.c > index 9a04ad4..55363fe 100644 > --- a/vl.c > +++ b/vl.c > @@ -168,7 +168,6 @@ static int no_reboot; > int no_shutdown = 0; > int cursor_hide = 1; > int graphic_rotate = 0; > -const char *watchdog; > QEMUOptionRom option_rom[MAX_OPTION_ROMS]; > int nb_option_roms; > int semihosting_enabled = 0; > @@ -475,6 +474,23 @@ static QemuOptsList qemu_icount_opts = { > }, > }; > > +static QemuOptsList qemu_watchdog_opts = { > + .name = "watchdog", > + .implied_opt_name = "model", > + .merge_lists = true, > + .head = QTAILQ_HEAD_INITIALIZER(qemu_watchdog_opts.head), > + .desc = { > + { > + .name = "model", > + .type = QEMU_OPT_STRING, > + }, { > + .name = "action", > + .type = QEMU_OPT_STRING, > + }, > + { /* end of list */ } > + }, > +}; > + > static QemuOptsList qemu_semihosting_config_opts = { > .name = "semihosting-config", > .implied_opt_name = "enable", > @@ -1224,6 +1240,26 @@ static void configure_msg(QemuOpts *opts) > enable_timestamp_msg = qemu_opt_get_bool(opts, "timestamp", true); > } > > +static void configure_watchdog(QemuOpts *opts) > +{ > + const char *s; > + > + s = qemu_opt_get(opts, "model"); > + if (s) { > + int i = select_watchdog(s); > + if (i != 0) { > + exit (i <= 1 ? 1 : 0); > + } > + } > + s = qemu_opt_get(opts, "action"); > + if (s) { > + int i = select_watchdog_action(s); > + if (i != 0) { > + exit (i <= 1 ? 1 : 0); > + } > + } > +} > + > /***********************************************************/ > /* USB devices */ > > @@ -2739,7 +2775,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) > > int main(int argc, char **argv, char **envp) > { > - int i; > int snapshot, linux_boot; > const char *initrd_filename; > const char *kernel_filename, *kernel_cmdline; > @@ -2815,6 +2850,7 @@ int main(int argc, char **argv, char **envp) > qemu_add_opts(&qemu_name_opts); > qemu_add_opts(&qemu_numa_opts); > qemu_add_opts(&qemu_icount_opts); > + qemu_add_opts(&qemu_watchdog_opts); > qemu_add_opts(&qemu_semihosting_config_opts); > > runstate_init(); > @@ -3343,18 +3379,15 @@ int main(int argc, char **argv, char **envp) > } > break; > case QEMU_OPTION_watchdog: > - if (watchdog) { > - fprintf(stderr, > - "qemu: only one watchdog option may be given\n"); > - return 1; > + olist = qemu_find_opts("watchdog"); > + opts = qemu_opts_parse(olist, optarg, 1); > + if (!opts) { > + exit(1); > } > - watchdog = optarg; > break; > case QEMU_OPTION_watchdog_action: > - if (select_watchdog_action(optarg) == -1) { > - fprintf(stderr, "Unknown -watchdog-action parameter\n"); > - exit(1); > - } > + qemu_opts_set(qemu_find_opts("watchdog"), 0, "action", optarg, > + &error_abort); > break; > case QEMU_OPTION_virtiocon: > add_device_config(DEV_VIRTCON, optarg); > @@ -4227,11 +4260,7 @@ int main(int argc, char **argv, char **envp) > select_vgahw(vga_model); > } > > - if (watchdog) { > - i = select_watchdog(watchdog); > - if (i > 0) > - exit (i == 1 ? 1 : 0); > - } > + configure_watchdog(qemu_find_opts_singleton("watchdog")); > > if (machine_class->compat_props) { > qdev_prop_register_global_list(machine_class->compat_props); > -- ACK. Rich.
On 05/27/2015 07:09 AM, Paolo Bonzini wrote: > This makes it possible to specify a watchdog action in a configuration file. > The previous behavior of "-watchdog" is moved to the (implied) "-watchdog > model" suboption. This is already more or less obsolete, since it is possible > to achieve the same effect with "-device", but "-watchdog-action" does not have > an equivalent. > > One alternative implementation is possible, namely to add an "action" > property to each watchdog device. However, boards often have embedded > watchdog devices; even if they currently don't, these should call > watchdog_perform_action() so that they are affected by -watchdog-action. > (This is listed in our BiteSizedTasks wiki page). > > For these boards, "-watchdog action=foo" has two advantages: > > 1) it is much easier to use than a "-global" option, and can be > configured globally for all boards. > > 2) it is harder to add a property to a device than it is to just > s/qemu_system_reset_request/watchdog_perform_action/; in some cases, > the devices are not even qdev-ified at all. The chance of the conversion > happening then would basically be zero if one had to add a property as > a prerequisite. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > tweak brackets in help messages [Eric] > fix missing error message for -watchdog action [Richard] > --- > docs/qdev-device-use.txt | 4 ++++ > qemu-options.hx | 37 +++++++++++++++-------------- > vl.c | 61 +++++++++++++++++++++++++++++++++++------------- > 3 files changed, 69 insertions(+), 33 deletions(-) > > DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \ > - "-watchdog i6300esb|ib700\n" \ > - " enable virtual hardware watchdog [default=none]\n", > + "-watchdog [[model=]i6300esb|ib700]\n" \ > + " [,action=reset|shutdown|poweroff|pause|debug|none]\n" \ > + " enable virtual hardware watchdog (default: model=none,action=reset)\n", > + QEMU_ARCH_ALL) > +DEF("watchdog-action", HAS_ARG, QEMU_OPTION_watchdog_action, \ > + "-watchdog-action reset|shutdown|poweroff|pause|debug|none\n" \ > + " action when watchdog fires (default: reset)\n", > QEMU_ARCH_ALL) > STEXI > -@item -watchdog @var{model} > +@item -watchdog [[model=]@var{model}][,action=@var{action}] Bikeshedding here: Hmm - this says I can supply -watchdog with no arguments. It also implies that '-watchdog ,action=...' would work. Maybe we split it into two lines: -watchdog [model=]@var{model} -watchdog [[model=]@var{model},]action=@var{action} But that gets more verbose than necessary. I can live with it as-is.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 27/05/2015 16:17, Eric Blake wrote: > Bikeshedding here: > > Hmm - this says I can supply -watchdog with no arguments. It also > implies that '-watchdog ,action=...' would work. Maybe we split it > into two lines: > > -watchdog [model=]@var{model} -watchdog > [[model=]@var{model},]action=@var{action} > > But that gets more verbose than necessary. I can live with it > as-is. True. However, this also matches several other -help lines. Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJVZddeAAoJEL/70l94x66De4wIAItEol7BBN5mg+totRjv31LL tcizi1vV2FqjUwiN5/kN1ZSWvoyTXi4bH9nNwkoCd+Q+e3UrxdpD7KG+c20hs+EB 9N6kDJPj4YGLaGoJEzhsSNqCq02c1c7sLtC71CQV6dVFhxA68CU7HkdcuDNix6MH OUHAHITSo02OEsmJxaF75zblm0zGIx5NYPDzT6c+cvgkVzYj/1ZYHyKjG7zuCLNH OXdM0xpajAoq0i4iLr1nMAx9TQbnBKNWLrUx/B5+0DDmlYMwE//RgunQ6gr78Zax GUpdJxexIj7zkdGKkgOcAyAOwWr7pPf3icnmh2Y4ZzGen/SY1+TAznFzJzLZTjc= =Deb/ -----END PGP SIGNATURE-----
Note: depends on Gerd's "[PATCH] QemuOpts: increase number of vm_config_groups". Without it, I get "ran out of space in vm_config_groups". Paolo Bonzini <pbonzini@redhat.com> writes: > This makes it possible to specify a watchdog action in a configuration file. > The previous behavior of "-watchdog" is moved to the (implied) "-watchdog > model" suboption. This is already more or less obsolete, since it is possible > to achieve the same effect with "-device", but "-watchdog-action" does not have > an equivalent. Adding -watchdog was a mistake. > One alternative implementation is possible, namely to add an "action" > property to each watchdog device. Yes, this is possible. Current global watchdog_action gets replaced by a watchdog device property, passed to watchdog_perform_action() as a parameter. What kvm_arch_handle_exit() should pass on KVM_EXIT_WATCHDOG isn't immediately obvious :) > However, boards often have embedded > watchdog devices; even if they currently don't, these should call > watchdog_perform_action() so that they are affected by -watchdog-action. > (This is listed in our BiteSizedTasks wiki page). You'd have to configure their watchdog action with -global, because that's how we set onboard device properties. > For these boards, "-watchdog action=foo" has two advantages: > > 1) it is much easier to use than a "-global" option, and can be > configured globally for all boards. > > 2) it is harder to add a property to a device than it is to just > s/qemu_system_reset_request/watchdog_perform_action/; in some cases, > the devices are not even qdev-ified at all. The chance of the conversion > happening then would basically be zero if one had to add a property as > a prerequisite. The advantages equally apply to existing -watchdog-action. Since using multiple watchdogs is weird, and with different actions even weirder, a single, shared watchdog action suffices. Let's take a step back. Stated objective: ability to configure watchdog in a configuration file. Have already: ability to configure watchdog device there. Example: [device] driver = "ib700" Note: unlike -balloon virtio, -watchdog does *not* expand into the equivalent -device. "-balloon virtio -watchdog ib700 -writeconfig /dev/stdout" produces [device] driver = "virtio-balloon" -balloon is pure syntactic sugar, -watchdog is alternative syntax. Missing part: ability to configure the watchdog action. Proposed solution: 1. Convert -watchdog to QemuOpts, with implied_opt_name "model". This makes -watchdog available in configuration files, like this: [watchdog] model = "ib700" "-balloon virtio -watchdog ib700 -writeconfig /dev/stdout" now produces [device] driver = "virtio-balloon" [watchdog] model = "ib700" Digs us deeper into the "alternative syntax" hole. The new QemuOptsList has .merge_lists = true, i.e. multiple definitions get merged. That one's always great fun. Before the patch, multiple -watchdog get rejected with "qemu: only one watchdog option may be given"[*]. Afterwards, all but one are silently ignored, and predicting which of multiple -watchdog is used is left as an exercise for the reader ;-P Of the three -watchdog behaviors "reject more than one watchdog", "just add them all whether it makes sense or not" and "add at most one, silently ignore the rest", this gives us the worst. 2. Add an "action" parameter to QemuOptsList "watchdog" This provides the missing part. -watchdog-action A becomes sugar for -watchdog action=A. Should be spelled out in the commit message. Remember, all the "watchdog" options get merged. So what does "--watchdog ib700,action=reset --watchdog i6300esb" do? Spoiler: adds i6300 with action reset[**]. Welcome again to the QemuOpts swamp, hope you'll enjoy your stay. If the stated objective is all you want, why not convert -watchdog-action instead of -watchdog? Should be simpler. Just make sure to preserve "last option wins" behavior. [*] Sub-par error message, use of error_report() would fix. [**] Found by trying it out; I'll be hanged if I can predict it without running the code
On 03/06/2015 15:14, Markus Armbruster wrote: >> However, boards often have embedded >> watchdog devices; even if they currently don't, these should call >> watchdog_perform_action() so that they are affected by -watchdog-action. >> (This is listed in our BiteSizedTasks wiki page). > > You'd have to configure their watchdog action with -global, because > that's how we set onboard device properties. Right. That however doesn't work if the watchdog is a device but isn't qdevified, or the watchdog isn't a device and doesn't have a dummy device wrapper around it, aka the KVM_EXIT_WATCHDOG case. Also, it's very hard to discover (e.g. how is one supposed to find a watchdog_action property under ICH9_LPC---not yet upstream, but should be in 2.4). > This makes -watchdog available in configuration files, like this: > > [watchdog] > model = "ib700" > > "-balloon virtio -watchdog ib700 -writeconfig /dev/stdout" now > produces > > [device] > driver = "virtio-balloon" > > [watchdog] > model = "ib700" > > Digs us deeper into the "alternative syntax" hole. True, but consistent with "-drive if=virtio" which doesn't produce a [device] stanza. > Of the three -watchdog behaviors "reject more than one watchdog", > "just add them all whether it makes sense or not" and "add at most > one, silently ignore the rest", this gives us the worst. True, but consistent with the handling of other merge_lists options: the last wins. > If the stated objective is all you want, why not convert > -watchdog-action instead of -watchdog? Should be simpler. Just make > sure to preserve "last option wins" behavior. Because I'm not sure that we won't have other watchdog options in the future. Also, [watchdog] action = "reset" is marginally nicer than any of [watchdog-action] action = "reset" and [machine] watchdog-action = "reset" Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 03/06/2015 15:14, Markus Armbruster wrote: >>> However, boards often have embedded >>> watchdog devices; even if they currently don't, these should call >>> watchdog_perform_action() so that they are affected by -watchdog-action. >>> (This is listed in our BiteSizedTasks wiki page). >> >> You'd have to configure their watchdog action with -global, because >> that's how we set onboard device properties. > > Right. That however doesn't work if the watchdog is a device but isn't > qdevified, or the watchdog isn't a device and doesn't have a dummy > device wrapper around it, aka the KVM_EXIT_WATCHDOG case. > > Also, it's very hard to discover (e.g. how is one supposed to find a > watchdog_action property under ICH9_LPC---not yet upstream, but should > be in 2.4). I completely agree with you that we don't want to turn it into a device property. The original design is just fine. >> This makes -watchdog available in configuration files, like this: >> >> [watchdog] >> model = "ib700" >> >> "-balloon virtio -watchdog ib700 -writeconfig /dev/stdout" now >> produces >> >> [device] >> driver = "virtio-balloon" >> >> [watchdog] >> model = "ib700" >> >> Digs us deeper into the "alternative syntax" hole. > > True, but consistent with "-drive if=virtio" which doesn't produce a > [device] stanza. -drive is level 5 magic, and the need for backward compatibility makes reducing its magic hard. The watchdog options could be level 1. Not doing "pure sugar" adds a level, and merge_lists adds another. If level 3 magic is what it takes to get the user interface and the backward compatibility we want, then so be it. >> Of the three -watchdog behaviors "reject more than one watchdog", >> "just add them all whether it makes sense or not" and "add at most >> one, silently ignore the rest", this gives us the worst. > > True, but consistent with the handling of other merge_lists options: the > last wins. Still a user interface change for the worse. That it's no worse now than other places have always been is a rather weak excuse :) >> If the stated objective is all you want, why not convert >> -watchdog-action instead of -watchdog? Should be simpler. Just make >> sure to preserve "last option wins" behavior. > > Because I'm not sure that we won't have other watchdog options in the > future. I believe what we got here is really just the usual split into frontend (a.k.a. device model) and backend, except the backend is trivial and shared by all watchdog frontends. Device model options should be added to the device model. What we need is a place to hold the shared backend's options, for easy-to-extend backend configuration. Converting -watchdog to QemuOpts creates such a place. So does converting -watchdog-action, except it's not mixed up with the existing *frontend* configuration sugar. > Also, > > [watchdog] > action = "reset" > > is marginally nicer than any of > > [watchdog-action] > action = "reset" > > and > > [machine] > watchdog-action = "reset" I agree that "watchdog-action" is an ugly name for backend configuration. "watchdog" would be fine, but we foolishly burned that on silly command line sugar for the frontend. "watchdog-config"? Precedent: "semihosting-config". Then make -watchdog T pure sugar for -device T, and -watchdog-action A pure sugar for -watchdog-config action=A. Not demands, just ideas.
On 03/06/2015 17:27, Markus Armbruster wrote: > I agree that "watchdog-action" is an ugly name for backend > configuration. "watchdog" would be fine, but we foolishly burned that > on silly command line sugar for the frontend. > > "watchdog-config"? Precedent: "semihosting-config". Sucks, but at least is consistent. It was hard to convince me on this as I was just typing "git send-pull" when I got your review, but you did it. :) Paolo > Then make -watchdog T pure sugar for -device T, and -watchdog-action A > pure sugar for -watchdog-config action=A. > > Not demands, just ideas.
diff --git a/docs/qdev-device-use.txt b/docs/qdev-device-use.txt index 136d271..1906b7d 100644 --- a/docs/qdev-device-use.txt +++ b/docs/qdev-device-use.txt @@ -365,6 +365,10 @@ The old way to define a guest watchdog device is -watchdog DEVNAME. The new way is -device DEVNAME. For PCI devices, you can add bus=PCI-BUS,addr=DEVFN to control the PCI device address, as usual. +No matter if you use -watchdog or -device to define the guest watchdog +device, you can specify the watchdog action with "-watchdog-action ACTION" +or "-watchdog action=ACTION". + === Host Device Assignment === QEMU supports assigning host PCI devices (qemu-kvm only at this time) diff --git a/qemu-options.hx b/qemu-options.hx index ec356f6..628f282 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3115,12 +3115,20 @@ when the shift value is high (how high depends on the host machine). ETEXI DEF("watchdog", HAS_ARG, QEMU_OPTION_watchdog, \ - "-watchdog i6300esb|ib700\n" \ - " enable virtual hardware watchdog [default=none]\n", + "-watchdog [[model=]i6300esb|ib700]\n" \ + " [,action=reset|shutdown|poweroff|pause|debug|none]\n" \ + " enable virtual hardware watchdog (default: model=none,action=reset)\n", + QEMU_ARCH_ALL) +DEF("watchdog-action", HAS_ARG, QEMU_OPTION_watchdog_action, \ + "-watchdog-action reset|shutdown|poweroff|pause|debug|none\n" \ + " action when watchdog fires (default: reset)\n", QEMU_ARCH_ALL) STEXI -@item -watchdog @var{model} +@item -watchdog [[model=]@var{model}][,action=@var{action}] @findex -watchdog +@itemx -watchdog-action @var{action} +@findex -watchdog-action + Create a virtual hardware watchdog device. Once enabled (by a guest action), the watchdog must be periodically polled by an agent inside the guest or else the guest will be restarted. @@ -3129,22 +3137,16 @@ The @var{model} is the model of hardware watchdog to emulate. Choices for model are: @code{ib700} (iBASE 700) which is a very simple ISA watchdog with a single timer, or @code{i6300esb} (Intel 6300ESB I/O controller hub) which is a much more featureful PCI-based dual-timer -watchdog. Choose a model for which your guest has drivers. - -Use @code{-watchdog help} to list available hardware models. Only one +watchdog. Choose a model for which your guest has drivers; +use @code{-watchdog help} to list available hardware models. Only one watchdog can be enabled for a guest. -ETEXI - -DEF("watchdog-action", HAS_ARG, QEMU_OPTION_watchdog_action, \ - "-watchdog-action reset|shutdown|poweroff|pause|debug|none\n" \ - " action when watchdog fires [default=reset]\n", - QEMU_ARCH_ALL) -STEXI -@item -watchdog-action @var{action} -@findex -watchdog-action The @var{action} controls what QEMU will do when the watchdog timer -expires. +expires. It can be specified directly in the @option{-watchdog} option, +or as the argument of a separate @option{-watchdog-action} parameter. +The effect is the same. The watchdog action can be specified even if +the watchdog is enabled through the @option{-device} option. + The default is @code{reset} (forcefully reset the guest). Other possible actions are: @@ -3162,8 +3164,9 @@ situations where the watchdog would have expired, and thus Examples: @table @code -@item -watchdog i6300esb -watchdog-action pause +@item -watchdog i6300esb,action=pause @item -watchdog ib700 +@item -device ib700 -watchdog action=poweroff @end table ETEXI diff --git a/vl.c b/vl.c index 9a04ad4..55363fe 100644 --- a/vl.c +++ b/vl.c @@ -168,7 +168,6 @@ static int no_reboot; int no_shutdown = 0; int cursor_hide = 1; int graphic_rotate = 0; -const char *watchdog; QEMUOptionRom option_rom[MAX_OPTION_ROMS]; int nb_option_roms; int semihosting_enabled = 0; @@ -475,6 +474,23 @@ static QemuOptsList qemu_icount_opts = { }, }; +static QemuOptsList qemu_watchdog_opts = { + .name = "watchdog", + .implied_opt_name = "model", + .merge_lists = true, + .head = QTAILQ_HEAD_INITIALIZER(qemu_watchdog_opts.head), + .desc = { + { + .name = "model", + .type = QEMU_OPT_STRING, + }, { + .name = "action", + .type = QEMU_OPT_STRING, + }, + { /* end of list */ } + }, +}; + static QemuOptsList qemu_semihosting_config_opts = { .name = "semihosting-config", .implied_opt_name = "enable", @@ -1224,6 +1240,26 @@ static void configure_msg(QemuOpts *opts) enable_timestamp_msg = qemu_opt_get_bool(opts, "timestamp", true); } +static void configure_watchdog(QemuOpts *opts) +{ + const char *s; + + s = qemu_opt_get(opts, "model"); + if (s) { + int i = select_watchdog(s); + if (i != 0) { + exit (i <= 1 ? 1 : 0); + } + } + s = qemu_opt_get(opts, "action"); + if (s) { + int i = select_watchdog_action(s); + if (i != 0) { + exit (i <= 1 ? 1 : 0); + } + } +} + /***********************************************************/ /* USB devices */ @@ -2739,7 +2775,6 @@ static void set_memory_options(uint64_t *ram_slots, ram_addr_t *maxram_size) int main(int argc, char **argv, char **envp) { - int i; int snapshot, linux_boot; const char *initrd_filename; const char *kernel_filename, *kernel_cmdline; @@ -2815,6 +2850,7 @@ int main(int argc, char **argv, char **envp) qemu_add_opts(&qemu_name_opts); qemu_add_opts(&qemu_numa_opts); qemu_add_opts(&qemu_icount_opts); + qemu_add_opts(&qemu_watchdog_opts); qemu_add_opts(&qemu_semihosting_config_opts); runstate_init(); @@ -3343,18 +3379,15 @@ int main(int argc, char **argv, char **envp) } break; case QEMU_OPTION_watchdog: - if (watchdog) { - fprintf(stderr, - "qemu: only one watchdog option may be given\n"); - return 1; + olist = qemu_find_opts("watchdog"); + opts = qemu_opts_parse(olist, optarg, 1); + if (!opts) { + exit(1); } - watchdog = optarg; break; case QEMU_OPTION_watchdog_action: - if (select_watchdog_action(optarg) == -1) { - fprintf(stderr, "Unknown -watchdog-action parameter\n"); - exit(1); - } + qemu_opts_set(qemu_find_opts("watchdog"), 0, "action", optarg, + &error_abort); break; case QEMU_OPTION_virtiocon: add_device_config(DEV_VIRTCON, optarg); @@ -4227,11 +4260,7 @@ int main(int argc, char **argv, char **envp) select_vgahw(vga_model); } - if (watchdog) { - i = select_watchdog(watchdog); - if (i > 0) - exit (i == 1 ? 1 : 0); - } + configure_watchdog(qemu_find_opts_singleton("watchdog")); if (machine_class->compat_props) { qdev_prop_register_global_list(machine_class->compat_props);
This makes it possible to specify a watchdog action in a configuration file. The previous behavior of "-watchdog" is moved to the (implied) "-watchdog model" suboption. This is already more or less obsolete, since it is possible to achieve the same effect with "-device", but "-watchdog-action" does not have an equivalent. One alternative implementation is possible, namely to add an "action" property to each watchdog device. However, boards often have embedded watchdog devices; even if they currently don't, these should call watchdog_perform_action() so that they are affected by -watchdog-action. (This is listed in our BiteSizedTasks wiki page). For these boards, "-watchdog action=foo" has two advantages: 1) it is much easier to use than a "-global" option, and can be configured globally for all boards. 2) it is harder to add a property to a device than it is to just s/qemu_system_reset_request/watchdog_perform_action/; in some cases, the devices are not even qdev-ified at all. The chance of the conversion happening then would basically be zero if one had to add a property as a prerequisite. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- tweak brackets in help messages [Eric] fix missing error message for -watchdog action [Richard] --- docs/qdev-device-use.txt | 4 ++++ qemu-options.hx | 37 +++++++++++++++-------------- vl.c | 61 +++++++++++++++++++++++++++++++++++------------- 3 files changed, 69 insertions(+), 33 deletions(-)