| Message ID | 1432730126-33400-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, May 27, 2015 at 02:35:26PM +0200, Paolo Bonzini wrote: > 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); > } Is there an error message missing (before the call to exit)? Patch seems OK otherwise. libvirt still uses -watchdog <model> -watchdog-action <action> Should it be changed to use -device? I'm guessing for backwards compatibility with old and current qemu it probably shouldn't be changed for a few years. Rich.
On 27/05/2015 14:41, Richard W.M. Jones wrote: > On Wed, May 27, 2015 at 02:35:26PM +0200, Paolo Bonzini wrote: >> 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); >> } > > Is there an error message missing (before the call to exit)? > > Patch seems OK otherwise. > > libvirt still uses > > -watchdog <model> -watchdog-action <action> > > Should it be changed to use -device? I'm guessing for backwards > compatibility with old and current qemu it probably shouldn't be > changed for a few years. -device has been working forever for watchdogs. Changing libvirt to -device would be possible, but cosmetic. Changing to "-watchdog action=foo" is also entirely cosmetic, and would not work on older QEMUs, so again there's no need. The advantage of this patch would be for -readconfig users, which couldn't specify the watchdog action. Or you could put '[watchdog] action="pause"' in /etc/qemu/qemu.conf if you're into obscure configuration files. Paolo
On 27/05/2015 14:41, Richard W.M. Jones wrote: >> > - 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); >> > } > Is there an error message missing (before the call to exit)? Oh, I forgot to answer this. Any error message is printed by qemu_opts_parse, for example: $ x86_64-softmmu/qemu-system-x86_64 -watchdog foo=bar qemu-system-x86_64: -watchdog foo=bar: Invalid parameter 'foo' Paolo
On Wed, May 27, 2015 at 02:45:29PM +0200, Paolo Bonzini wrote: > > > On 27/05/2015 14:41, Richard W.M. Jones wrote: > >> > - 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); > >> > } > > Is there an error message missing (before the call to exit)? > > Oh, I forgot to answer this. Any error message is printed by > qemu_opts_parse, for example: > > $ x86_64-softmmu/qemu-system-x86_64 -watchdog foo=bar > qemu-system-x86_64: -watchdog foo=bar: Invalid parameter 'foo' Patch seems fine then, ACK. Rich.
On 05/27/2015 06:35 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 the 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> > --- > docs/qdev-device-use.txt | 4 ++++ > qemu-options.hx | 36 ++++++++++++++-------------- > vl.c | 61 +++++++++++++++++++++++++++++++++++------------- > 3 files changed, 68 insertions(+), 33 deletions(-) > > +++ b/qemu-options.hx > @@ -3115,12 +3115,19 @@ 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,action=reset|shutdown|poweroff|pause|debug|none\n" \ Should this be -watchdog [model=]i6300esb|ib700[,action=...none] to make it obvious that ,action= is optional for back-compat with old usage? > + " 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} same here.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 27/05/2015 15:00, Eric Blake wrote: > Should this be > > -watchdog [model=]i6300esb|ib700[,action=...none] > > to make it obvious that ,action= is optional for back-compat with > old usage? Sure, but then it should be -watchdog [[model=]i6300esb|ib700][,action=...none] Paolo -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJVZcB5AAoJEL/70l94x66DyZMIAJN5YCp+NwIxYMieIEssXDsG sZCyu3pv7RG8tx66VnExBXSUxMd4U4JXvDbME9g6jVPie3CNGXbkgE0f7BWAfBbi 1oA07px8Zx0xOMZIMWDJc803viSixHn/UJxFcN00baS+lx9l6a0yRt013Z0sFbNl 0TBSVpYUOM0JNo42QXuSiMClbxLE/DaSS4NO+2uPV9NV1USz3LDM+/bd7BKHmRsb JZhVPXYmmq/ZxSrb+UJprwJOqQ7yHqZeLU0IL76TrWGthRWfMZP7Dg6y5QZgR3cd cM00qERu4iIYH18TwA6jBdoPjgsHMDWvHuwLhrpqOcnl5KKu/F4rKjdXLOvfxHw= =XXtJ -----END PGP SIGNATURE-----
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..719253c 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3115,12 +3115,19 @@ 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,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 +3136,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 +3163,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..b76919e 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 the 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> --- docs/qdev-device-use.txt | 4 ++++ qemu-options.hx | 36 ++++++++++++++-------------- vl.c | 61 +++++++++++++++++++++++++++++++++++------------- 3 files changed, 68 insertions(+), 33 deletions(-)