| Message ID | 20240112100059.965041-6-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] qemu-options: Remove the deprecated -no-hpet option | expand |
Hi Thomas +Laurent & Peter On 12/1/24 11:00, Thomas Huth wrote: > It's been marked as deprecated since QEMU 8.1, so it should be fine > to remove this now. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > docs/about/deprecated.rst | 6 ------ > docs/about/removed-features.rst | 7 +++++++ > system/vl.c | 18 +----------------- > qemu-options.hx | 8 -------- > 4 files changed, 8 insertions(+), 31 deletions(-) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index b50cfe596b..81a5149f1e 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -63,12 +63,6 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``. > However, short-form booleans are deprecated and full explicit ``arg_name=on`` > form is preferred. > > -``-singlestep`` (since 8.1) > -''''''''''''''''''''''''''' > - > -The ``-singlestep`` option has been turned into an accelerator property, > -and given a name that better reflects what it actually does. > -Use ``-accel tcg,one-insn-per-tb=on`` instead. > > User-mode emulator command line arguments > ----------------------------------------- > diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst > index a8546f4787..d5c60ff47f 100644 > --- a/docs/about/removed-features.rst > +++ b/docs/about/removed-features.rst > @@ -482,6 +482,13 @@ Use ``-run-with async-teardown=on`` instead. > > Use ``-run-with chroot=dir`` instead. > > +``-singlestep`` (removed in 9.0) > +'''''''''''''''''''''''''''''''' > + > +The ``-singlestep`` option has been turned into an accelerator property, > +and given a name that better reflects what it actually does. > +Use ``-accel tcg,one-insn-per-tb=on`` instead. > + > > QEMU Machine Protocol (QMP) commands > ------------------------------------ > diff --git a/system/vl.c b/system/vl.c > index c125fb9079..809f867bcc 100644 > --- a/system/vl.c > +++ b/system/vl.c > @@ -181,7 +181,6 @@ static const char *log_file; > static bool list_data_dirs; > static const char *qtest_chrdev; > static const char *qtest_log; > -static bool opt_one_insn_per_tb; > > static int has_defaults = 1; > static int default_audio = 1; > @@ -2308,19 +2307,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp) > qemu_opt_foreach(opts, accelerator_set_property, > accel, > &error_fatal); > - /* > - * If legacy -singlestep option is set, honour it for TCG and > - * silently ignore for any other accelerator (which is how this > - * option has always behaved). > - */ > - if (opt_one_insn_per_tb) { > - /* > - * This will always succeed for TCG, and we want to ignore > - * the error from trying to set a nonexistent property > - * on any other accelerator. > - */ > - object_property_set_bool(OBJECT(accel), "one-insn-per-tb", true, NULL); > - } > + > ret = accel_init_machine(accel, current_machine); > if (ret < 0) { > if (!qtest_with_kvm || ret != -ENOENT) { > @@ -3057,9 +3044,6 @@ void qemu_init(int argc, char **argv) > case QEMU_OPTION_bios: > qdict_put_str(machine_opts_dict, "firmware", optarg); > break; > - case QEMU_OPTION_singlestep: > - opt_one_insn_per_tb = true; > - break; > case QEMU_OPTION_S: > autostart = 0; > break; > diff --git a/qemu-options.hx b/qemu-options.hx > index 9be6beb5a0..033fa873e4 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -4357,14 +4357,6 @@ SRST > from a script. > ERST > > -DEF("singlestep", 0, QEMU_OPTION_singlestep, \ > - "-singlestep deprecated synonym for -accel tcg,one-insn-per-tb=on\n", QEMU_ARCH_ALL) > -SRST > -``-singlestep`` > - This is a deprecated synonym for the TCG accelerator property > - ``one-insn-per-tb``. > -ERST > - > DEF("preconfig", 0, QEMU_OPTION_preconfig, \ > "--preconfig pause QEMU before machine is initialized (experimental)\n", > QEMU_ARCH_ALL) This is the system part, what about the user part? StatusInfo::singlestep was deprecated at the same time, can we remove it? IOW could we complete your patch with this? -- >8 -- diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index b47763330c..a1ae93664a 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -63,17 +63,6 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``. However, short-form booleans are deprecated and full explicit ``arg_name=on`` form is preferred. - -User-mode emulator command line arguments ------------------------------------------ - -``-singlestep`` (since 8.1) -''''''''''''''''''''''''''' - -The ``-singlestep`` option has been given a name that better reflects -what it actually does. For both linux-user and bsd-user, use the -new ``-one-insn-per-tb`` option instead. - QEMU Machine Protocol (QMP) commands ------------------------------------ @@ -145,20 +134,6 @@ accepted incorrect commands will return an error. Users should make sure that all arguments passed to ``device_add`` are consistent with the documented property types. -``StatusInfo`` member ``singlestep`` (since 8.1) -'''''''''''''''''''''''''''''''''''''''''''''''' - -The ``singlestep`` member of the ``StatusInfo`` returned from the -``query-status`` command is deprecated. This member has a confusing -name and it never did what the documentation claimed or what its name -suggests. We do not believe that anybody is actually using the -information provided in this member. - -The information it reports is whether the TCG JIT is in "one -instruction per translated block" mode (which can be set on the -command line or via the HMP, but not via QMP). The information remains -available via the HMP 'info jit' command. - QEMU Machine Protocol (QMP) events ---------------------------------- diff --git a/docs/user/main.rst b/docs/user/main.rst index f478635396..7e7ad07409 100644 --- a/docs/user/main.rst +++ b/docs/user/main.rst @@ -98,9 +98,6 @@ Debug options: This slows down emulation a lot, but can be useful in some situations, such as when trying to analyse the logs produced by the ``-d`` option. -``-singlestep`` - This is a deprecated synonym for the ``-one-insn-per-tb`` option. - Environment variables: QEMU_STRACE @@ -251,6 +248,3 @@ Debug options: Run the emulation with one guest instruction per translation block. This slows down emulation a lot, but can be useful in some situations, such as when trying to analyse the logs produced by the ``-d`` option. - -``-singlestep`` - This is a deprecated synonym for the ``-one-insn-per-tb`` option. diff --git a/qapi/run-state.json b/qapi/run-state.json index ca05502e0a..08bc99cb85 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -106,25 +106,15 @@ # # @running: true if all VCPUs are runnable, false if not runnable # -# @singlestep: true if using TCG with one guest instruction per -# translation block -# # @status: the virtual machine @RunState # # Features: # -# @deprecated: Member 'singlestep' is deprecated (with no -# replacement). -# # Since: 0.14 # -# Notes: @singlestep is enabled on the command line with '-accel -# tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' -# command. ## { 'struct': 'StatusInfo', 'data': {'running': 'bool', - 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, 'status': 'RunState'} } ## @@ -140,7 +130,6 @@ # # -> { "execute": "query-status" } # <- { "return": { "running": true, -# "singlestep": false, # "status": "running" } } ## { 'command': 'query-status', 'returns': 'StatusInfo', diff --git a/bsd-user/main.c b/bsd-user/main.c index 4de226d211..e5efb7b845 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -163,7 +163,6 @@ static void usage(void) " (use '-d help' for a list of log items)\n" "-D logfile write logs to 'logfile' (default stderr)\n" "-one-insn-per-tb run with one guest instruction per emulated TB\n" - "-singlestep deprecated synonym for -one-insn-per-tb\n" "-strace log system calls\n" "-trace [[enable=]<pattern>][,events=<file>][,file=<file>]\n" " specify tracing options\n" @@ -391,7 +390,7 @@ int main(int argc, char **argv) (void) envlist_unsetenv(envlist, "LD_PRELOAD"); } else if (!strcmp(r, "seed")) { seed_optarg = optarg; - } else if (!strcmp(r, "singlestep") || !strcmp(r, "one-insn-per-tb")) { + } else if (!strcmp(r, "one-insn-per-tb")) { opt_one_insn_per_tb = true; } else if (!strcmp(r, "strace")) { do_strace = 1; diff --git a/linux-user/main.c b/linux-user/main.c index 0cdaf30d34..c9470eeccf 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -500,8 +500,6 @@ static const struct qemu_argument arg_table[] = { {"one-insn-per-tb", "QEMU_ONE_INSN_PER_TB", false, handle_arg_one_insn_per_tb, "", "run with one guest instruction per emulated TB"}, - {"singlestep", "QEMU_SINGLESTEP", false, handle_arg_one_insn_per_tb, - "", "deprecated synonym for -one-insn-per-tb"}, {"strace", "QEMU_STRACE", false, handle_arg_strace, "", "log system calls"}, {"seed", "QEMU_RAND_SEED", true, handle_arg_seed, diff --git a/system/runstate.c b/system/runstate.c index fb07b7b71a..d6ab860eca 100644 --- a/system/runstate.c +++ b/system/runstate.c @@ -242,15 +242,7 @@ bool runstate_needs_reset(void) StatusInfo *qmp_query_status(Error **errp) { StatusInfo *info = g_malloc0(sizeof(*info)); - AccelState *accel = current_accel(); - /* - * We ignore errors, which will happen if the accelerator - * is not TCG. "singlestep" is meaningless for other accelerators, - * so we will set the StatusInfo field to false for those. - */ - info->singlestep = object_property_get_bool(OBJECT(accel), - "one-insn-per-tb", NULL); info->running = runstate_is_running(); info->status = current_run_state; diff --git a/tests/qemu-iotests/183.out b/tests/qemu-iotests/183.out index fd9c2e52a5..9277643853 100644 --- a/tests/qemu-iotests/183.out +++ b/tests/qemu-iotests/183.out @@ -30,13 +30,13 @@ read 65536/65536 bytes at offset 0 'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } } {"return": {}} { 'execute': 'query-status' } -{"return": {"status": "postmigrate", "singlestep": false, "running": false}} +{"return": {"status": "postmigrate", "running": false}} === Do some I/O on the destination === { 'execute': 'query-status' } {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESUME"} -{"return": {"status": "running", "singlestep": false, "running": true}} +{"return": {"status": "running", "running": true}} { 'execute': 'human-monitor-command', 'arguments': { 'command-line': 'qemu-io disk "read -P 0x55 0 64k"' } } diff --git a/tests/qemu-iotests/234.out b/tests/qemu-iotests/234.out index 692976d1c6..ac8b64350c 100644 --- a/tests/qemu-iotests/234.out +++ b/tests/qemu-iotests/234.out @@ -15,8 +15,8 @@ Starting migration to B... {"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} completed completed -{"return": {"running": false, "singlestep": false, "status": "postmigrate"}} -{"return": {"running": true, "singlestep": false, "status": "running"}} +{"return": {"running": false, "status": "postmigrate"}} +{"return": {"running": true, "status": "running"}} Add a second parent to drive0-file... {"return": {}} Restart A with -incoming and second parent... @@ -32,5 +32,5 @@ Starting migration back to A... {"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} completed completed -{"return": {"running": true, "singlestep": false, "status": "running"}} -{"return": {"running": false, "singlestep": false, "status": "postmigrate"}} +{"return": {"running": true, "status": "running"}} +{"return": {"running": false, "status": "postmigrate"}} diff --git a/tests/qemu-iotests/262.out b/tests/qemu-iotests/262.out index 8e04c496c4..b8a2d3598d 100644 --- a/tests/qemu-iotests/262.out +++ b/tests/qemu-iotests/262.out @@ -13,5 +13,5 @@ Starting migration to B... {"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} completed completed -{"return": {"running": false, "singlestep": false, "status": "postmigrate"}} -{"return": {"running": true, "singlestep": false, "status": "running"}} +{"return": {"running": false, "status": "postmigrate"}} +{"return": {"running": true, "status": "running"}} diff --git a/tests/qemu-iotests/280.out b/tests/qemu-iotests/280.out index c75f437c00..546dbb4a68 100644 --- a/tests/qemu-iotests/280.out +++ b/tests/qemu-iotests/280.out @@ -12,7 +12,7 @@ Enabling migration QMP events on VM... VM is now stopped: completed {"execute": "query-status", "arguments": {}} -{"return": {"running": false, "singlestep": false, "status": "postmigrate"}} +{"return": {"running": false, "status": "postmigrate"}} === Create a snapshot of the disk image === {"execute": "blockdev-create", "arguments": {"job-id": "job0", "options": {"driver": "file", "filename": "TEST_DIR/PID-top", "size": 0}}} ---
On 12/01/2024 16.39, Philippe Mathieu-Daudé wrote: > Hi Thomas > > +Laurent & Peter > > On 12/1/24 11:00, Thomas Huth wrote: >> It's been marked as deprecated since QEMU 8.1, so it should be fine >> to remove this now. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> docs/about/deprecated.rst | 6 ------ >> docs/about/removed-features.rst | 7 +++++++ >> system/vl.c | 18 +----------------- >> qemu-options.hx | 8 -------- >> 4 files changed, 8 insertions(+), 31 deletions(-) >> >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst >> index b50cfe596b..81a5149f1e 100644 >> --- a/docs/about/deprecated.rst >> +++ b/docs/about/deprecated.rst >> @@ -63,12 +63,6 @@ as short-form boolean values, and passed to plugins as >> ``arg_name=on``. >> However, short-form booleans are deprecated and full explicit >> ``arg_name=on`` >> form is preferred. >> -``-singlestep`` (since 8.1) >> -''''''''''''''''''''''''''' >> - >> -The ``-singlestep`` option has been turned into an accelerator property, >> -and given a name that better reflects what it actually does. >> -Use ``-accel tcg,one-insn-per-tb=on`` instead. >> User-mode emulator command line arguments >> ----------------------------------------- >> diff --git a/docs/about/removed-features.rst >> b/docs/about/removed-features.rst >> index a8546f4787..d5c60ff47f 100644 >> --- a/docs/about/removed-features.rst >> +++ b/docs/about/removed-features.rst >> @@ -482,6 +482,13 @@ Use ``-run-with async-teardown=on`` instead. >> Use ``-run-with chroot=dir`` instead. >> +``-singlestep`` (removed in 9.0) >> +'''''''''''''''''''''''''''''''' >> + >> +The ``-singlestep`` option has been turned into an accelerator property, >> +and given a name that better reflects what it actually does. >> +Use ``-accel tcg,one-insn-per-tb=on`` instead. >> + >> QEMU Machine Protocol (QMP) commands >> ------------------------------------ >> diff --git a/system/vl.c b/system/vl.c >> index c125fb9079..809f867bcc 100644 >> --- a/system/vl.c >> +++ b/system/vl.c >> @@ -181,7 +181,6 @@ static const char *log_file; >> static bool list_data_dirs; >> static const char *qtest_chrdev; >> static const char *qtest_log; >> -static bool opt_one_insn_per_tb; >> static int has_defaults = 1; >> static int default_audio = 1; >> @@ -2308,19 +2307,7 @@ static int do_configure_accelerator(void *opaque, >> QemuOpts *opts, Error **errp) >> qemu_opt_foreach(opts, accelerator_set_property, >> accel, >> &error_fatal); >> - /* >> - * If legacy -singlestep option is set, honour it for TCG and >> - * silently ignore for any other accelerator (which is how this >> - * option has always behaved). >> - */ >> - if (opt_one_insn_per_tb) { >> - /* >> - * This will always succeed for TCG, and we want to ignore >> - * the error from trying to set a nonexistent property >> - * on any other accelerator. >> - */ >> - object_property_set_bool(OBJECT(accel), "one-insn-per-tb", true, >> NULL); >> - } >> + >> ret = accel_init_machine(accel, current_machine); >> if (ret < 0) { >> if (!qtest_with_kvm || ret != -ENOENT) { >> @@ -3057,9 +3044,6 @@ void qemu_init(int argc, char **argv) >> case QEMU_OPTION_bios: >> qdict_put_str(machine_opts_dict, "firmware", optarg); >> break; >> - case QEMU_OPTION_singlestep: >> - opt_one_insn_per_tb = true; >> - break; >> case QEMU_OPTION_S: >> autostart = 0; >> break; >> diff --git a/qemu-options.hx b/qemu-options.hx >> index 9be6beb5a0..033fa873e4 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -4357,14 +4357,6 @@ SRST >> from a script. >> ERST >> -DEF("singlestep", 0, QEMU_OPTION_singlestep, \ >> - "-singlestep deprecated synonym for -accel >> tcg,one-insn-per-tb=on\n", QEMU_ARCH_ALL) >> -SRST >> -``-singlestep`` >> - This is a deprecated synonym for the TCG accelerator property >> - ``one-insn-per-tb``. >> -ERST >> - >> DEF("preconfig", 0, QEMU_OPTION_preconfig, \ >> "--preconfig pause QEMU before machine is initialized >> (experimental)\n", >> QEMU_ARCH_ALL) > > This is the system part, what about the user part? > > StatusInfo::singlestep was deprecated at the same time, > can we remove it? > > IOW could we complete your patch with this? > > -- >8 -- > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index b47763330c..a1ae93664a 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -63,17 +63,6 @@ as short-form boolean values, and passed to plugins as > ``arg_name=on``. > However, short-form booleans are deprecated and full explicit ``arg_name=on`` > form is preferred. > > - > -User-mode emulator command line arguments > ------------------------------------------ > - > -``-singlestep`` (since 8.1) > -''''''''''''''''''''''''''' > - > -The ``-singlestep`` option has been given a name that better reflects > -what it actually does. For both linux-user and bsd-user, use the > -new ``-one-insn-per-tb`` option instead. > - > QEMU Machine Protocol (QMP) commands > ------------------------------------ > > @@ -145,20 +134,6 @@ accepted incorrect commands will return an error. Users > should make sure that > all arguments passed to ``device_add`` are consistent with the documented > property types. > > -``StatusInfo`` member ``singlestep`` (since 8.1) > -'''''''''''''''''''''''''''''''''''''''''''''''' > - > -The ``singlestep`` member of the ``StatusInfo`` returned from the > -``query-status`` command is deprecated. This member has a confusing > -name and it never did what the documentation claimed or what its name > -suggests. We do not believe that anybody is actually using the > -information provided in this member. > - > -The information it reports is whether the TCG JIT is in "one > -instruction per translated block" mode (which can be set on the > -command line or via the HMP, but not via QMP). The information remains > -available via the HMP 'info jit' command. > - > QEMU Machine Protocol (QMP) events > ---------------------------------- > > diff --git a/docs/user/main.rst b/docs/user/main.rst > index f478635396..7e7ad07409 100644 > --- a/docs/user/main.rst > +++ b/docs/user/main.rst > @@ -98,9 +98,6 @@ Debug options: > This slows down emulation a lot, but can be useful in some situations, > such as when trying to analyse the logs produced by the ``-d`` option. > > -``-singlestep`` > - This is a deprecated synonym for the ``-one-insn-per-tb`` option. > - > Environment variables: > > QEMU_STRACE > @@ -251,6 +248,3 @@ Debug options: > Run the emulation with one guest instruction per translation block. > This slows down emulation a lot, but can be useful in some situations, > such as when trying to analyse the logs produced by the ``-d`` option. > - > -``-singlestep`` > - This is a deprecated synonym for the ``-one-insn-per-tb`` option. > diff --git a/qapi/run-state.json b/qapi/run-state.json > index ca05502e0a..08bc99cb85 100644 > --- a/qapi/run-state.json > +++ b/qapi/run-state.json > @@ -106,25 +106,15 @@ > # > # @running: true if all VCPUs are runnable, false if not runnable > # > -# @singlestep: true if using TCG with one guest instruction per > -# translation block > -# > # @status: the virtual machine @RunState > # > # Features: > # > -# @deprecated: Member 'singlestep' is deprecated (with no > -# replacement). > -# > # Since: 0.14 > # > -# Notes: @singlestep is enabled on the command line with '-accel > -# tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' > -# command. > ## > { 'struct': 'StatusInfo', > 'data': {'running': 'bool', > - 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, > 'status': 'RunState'} } Uh, oh, that's a bigger change already ... can we safely remove the field here without upsetting 3rd party apps that rely on this interface? Anyway, I withdraw my patch for -singlestep ... this should be handled by someone who's more familiar with these pieces of the code. Thomas
On Mon, 15 Jan 2024 at 13:54, Thomas Huth <thuth@redhat.com> wrote: > > On 12/01/2024 16.39, Philippe Mathieu-Daudé wrote: > > Hi Thomas > > > > +Laurent & Peter > > > > On 12/1/24 11:00, Thomas Huth wrote: > >> It's been marked as deprecated since QEMU 8.1, so it should be fine > >> to remove this now. > >> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > StatusInfo::singlestep was deprecated at the same time, > > can we remove it? > > > > IOW could we complete your patch with this? > > diff --git a/qapi/run-state.json b/qapi/run-state.json > > index ca05502e0a..08bc99cb85 100644 > > --- a/qapi/run-state.json > > +++ b/qapi/run-state.json > > @@ -106,25 +106,15 @@ > > # > > # @running: true if all VCPUs are runnable, false if not runnable > > # > > -# @singlestep: true if using TCG with one guest instruction per > > -# translation block > > -# > > # @status: the virtual machine @RunState > > # > > # Features: > > # > > -# @deprecated: Member 'singlestep' is deprecated (with no > > -# replacement). > > -# > > # Since: 0.14 > > # > > -# Notes: @singlestep is enabled on the command line with '-accel > > -# tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' > > -# command. > > ## > > { 'struct': 'StatusInfo', > > 'data': {'running': 'bool', > > - 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, > > 'status': 'RunState'} } > > Uh, oh, that's a bigger change already ... can we safely remove the field > here without upsetting 3rd party apps that rely on this interface? That was the whole point of marking it 'deprecated' in the JSON, I thought? We don't think anybody's using it, we've given fair warning, isn't the next step "remove it"? Markus, you're the expert on QAPI deprecations... -- PMM
On Mon, Jan 15, 2024 at 05:39:19PM +0000, Peter Maydell wrote: > On Mon, 15 Jan 2024 at 13:54, Thomas Huth <thuth@redhat.com> wrote: > > > > On 12/01/2024 16.39, Philippe Mathieu-Daudé wrote: > > > Hi Thomas > > > > > > +Laurent & Peter > > > > > > On 12/1/24 11:00, Thomas Huth wrote: > > >> It's been marked as deprecated since QEMU 8.1, so it should be fine > > >> to remove this now. > > >> > > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > StatusInfo::singlestep was deprecated at the same time, > > > can we remove it? > > > > > > IOW could we complete your patch with this? > > > > diff --git a/qapi/run-state.json b/qapi/run-state.json > > > index ca05502e0a..08bc99cb85 100644 > > > --- a/qapi/run-state.json > > > +++ b/qapi/run-state.json > > > @@ -106,25 +106,15 @@ > > > # > > > # @running: true if all VCPUs are runnable, false if not runnable > > > # > > > -# @singlestep: true if using TCG with one guest instruction per > > > -# translation block > > > -# > > > # @status: the virtual machine @RunState > > > # > > > # Features: > > > # > > > -# @deprecated: Member 'singlestep' is deprecated (with no > > > -# replacement). > > > -# > > > # Since: 0.14 > > > # > > > -# Notes: @singlestep is enabled on the command line with '-accel > > > -# tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' > > > -# command. > > > ## > > > { 'struct': 'StatusInfo', > > > 'data': {'running': 'bool', > > > - 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, > > > 'status': 'RunState'} } > > > > Uh, oh, that's a bigger change already ... can we safely remove the field > > here without upsetting 3rd party apps that rely on this interface? > > That was the whole point of marking it 'deprecated' in the JSON, > I thought? We don't think anybody's using it, we've given fair > warning, isn't the next step "remove it"? Markus, you're the > expert on QAPI deprecations... Yes, it is fine to delete it without thinking further about possible usage, unless someone steps forward quickly with new information that wasn't known when the deprecation was added.... With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Jan 15, 2024 at 05:39:19PM +0000, Peter Maydell wrote: >> On Mon, 15 Jan 2024 at 13:54, Thomas Huth <thuth@redhat.com> wrote: >> > >> > On 12/01/2024 16.39, Philippe Mathieu-Daudé wrote: >> > > Hi Thomas >> > > >> > > +Laurent & Peter >> > > >> > > On 12/1/24 11:00, Thomas Huth wrote: >> > >> It's been marked as deprecated since QEMU 8.1, so it should be fine >> > >> to remove this now. >> > >> >> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> >> > > StatusInfo::singlestep was deprecated at the same time, >> > > can we remove it? >> > > >> > > IOW could we complete your patch with this? >> >> > > diff --git a/qapi/run-state.json b/qapi/run-state.json >> > > index ca05502e0a..08bc99cb85 100644 >> > > --- a/qapi/run-state.json >> > > +++ b/qapi/run-state.json >> > > @@ -106,25 +106,15 @@ >> > > # >> > > # @running: true if all VCPUs are runnable, false if not runnable >> > > # >> > > -# @singlestep: true if using TCG with one guest instruction per >> > > -# translation block >> > > -# >> > > # @status: the virtual machine @RunState >> > > # >> > > # Features: >> > > # >> > > -# @deprecated: Member 'singlestep' is deprecated (with no >> > > -# replacement). >> > > -# >> > > # Since: 0.14 >> > > # >> > > -# Notes: @singlestep is enabled on the command line with '-accel >> > > -# tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' >> > > -# command. >> > > ## >> > > { 'struct': 'StatusInfo', >> > > 'data': {'running': 'bool', >> > > - 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, >> > > 'status': 'RunState'} } >> > >> > Uh, oh, that's a bigger change already ... can we safely remove the field >> > here without upsetting 3rd party apps that rely on this interface? >> >> That was the whole point of marking it 'deprecated' in the JSON, >> I thought? We don't think anybody's using it, we've given fair >> warning, isn't the next step "remove it"? Markus, you're the >> expert on QAPI deprecations... > > Yes, it is fine to delete it without thinking further about possible usage, > unless someone steps forward quickly with new information that wasn't known > when the deprecation was added.... Concur. Supporting data: commit 34c18203d472c5bf969ebd87dc06c7c3a957efc4 Author: Peter Maydell <peter.maydell@linaro.org> Date: Mon Apr 17 17:40:41 2023 +0100 qmp: Deprecate 'singlestep' member of StatusInfo The 'singlestep' member of StatusInfo has never done what the QMP documentation claims it does. What it actually reports is whether TCG is working in "one guest instruction per translation block" mode. We no longer need this field for the HMP 'info status' command, as we've moved that information to 'info jit'. It seems unlikely that anybody is monitoring the state of this obscure TCG setting via QMP, especially since QMP provides no means for changing the setting. So simply deprecate the field, without providing any replacement. Until we do eventually delete the member, correct the misstatements in the QAPI documentation about it. If we do find that there are users for this, then the most likely way we would provide replacement access to the information would be to put the accelerator QOM object at a well-known path such as /machine/accel, which could then be used with the existing qom-set and qom-get commands. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Reviewed-by: Markus Armbruster <armbru@redhat.com> Message-id: 20230417164041.684562-11-peter.maydell@linaro.org diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 6f5e689aa4..d5eda0f566 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -199,6 +199,20 @@ accepted incorrect commands will return an error. Users should make sure that all arguments passed to ``device_add`` are consistent with the documented property types. +``StatusInfo`` member ``singlestep`` (since 8.1) +'''''''''''''''''''''''''''''''''''''''''''''''' + +The ``singlestep`` member of the ``StatusInfo`` returned from the +``query-status`` command is deprecated. This member has a confusing +name and it never did what the documentation claimed or what its name +suggests. We do not believe that anybody is actually using the +information provided in this member. + +The information it reports is whether the TCG JIT is in "one +instruction per translated block" mode (which can be set on the +command line or via the HMP, but not via QMP). The information remains +available via the HMP 'info jit' command. + [...]
On 16/1/24 07:27, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> On Mon, Jan 15, 2024 at 05:39:19PM +0000, Peter Maydell wrote: >>> On Mon, 15 Jan 2024 at 13:54, Thomas Huth <thuth@redhat.com> wrote: >>>> >>>> On 12/01/2024 16.39, Philippe Mathieu-Daudé wrote: >>>>> Hi Thomas >>>>> >>>>> +Laurent & Peter >>>>> >>>>> On 12/1/24 11:00, Thomas Huth wrote: >>>>>> It's been marked as deprecated since QEMU 8.1, so it should be fine >>>>>> to remove this now. >>>>>> >>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> >>>>> StatusInfo::singlestep was deprecated at the same time, >>>>> can we remove it? >>>>> >>>>> IOW could we complete your patch with this? >>> >>>>> diff --git a/qapi/run-state.json b/qapi/run-state.json >>>>> index ca05502e0a..08bc99cb85 100644 >>>>> --- a/qapi/run-state.json >>>>> +++ b/qapi/run-state.json >>>>> @@ -106,25 +106,15 @@ >>>>> # >>>>> # @running: true if all VCPUs are runnable, false if not runnable >>>>> # >>>>> -# @singlestep: true if using TCG with one guest instruction per >>>>> -# translation block >>>>> -# >>>>> # @status: the virtual machine @RunState >>>>> # >>>>> # Features: >>>>> # >>>>> -# @deprecated: Member 'singlestep' is deprecated (with no >>>>> -# replacement). >>>>> -# >>>>> # Since: 0.14 >>>>> # >>>>> -# Notes: @singlestep is enabled on the command line with '-accel >>>>> -# tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' >>>>> -# command. >>>>> ## >>>>> { 'struct': 'StatusInfo', >>>>> 'data': {'running': 'bool', >>>>> - 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, >>>>> 'status': 'RunState'} } >>>> >>>> Uh, oh, that's a bigger change already ... can we safely remove the field >>>> here without upsetting 3rd party apps that rely on this interface? >>> >>> That was the whole point of marking it 'deprecated' in the JSON, >>> I thought? We don't think anybody's using it, we've given fair >>> warning, isn't the next step "remove it"? Markus, you're the >>> expert on QAPI deprecations... >> >> Yes, it is fine to delete it without thinking further about possible usage, >> unless someone steps forward quickly with new information that wasn't known >> when the deprecation was added.... > > Concur. Thanks all for the feedback. Thomas, are you OK to post a v2 with the changes I suggested or do you want me to do it? > Supporting data: > > commit 34c18203d472c5bf969ebd87dc06c7c3a957efc4 > Author: Peter Maydell <peter.maydell@linaro.org> > Date: Mon Apr 17 17:40:41 2023 +0100 > > qmp: Deprecate 'singlestep' member of StatusInfo > > The 'singlestep' member of StatusInfo has never done what the QMP > documentation claims it does. What it actually reports is whether > TCG is working in "one guest instruction per translation block" mode. > > We no longer need this field for the HMP 'info status' command, as > we've moved that information to 'info jit'. It seems unlikely that > anybody is monitoring the state of this obscure TCG setting via QMP, > especially since QMP provides no means for changing the setting. So > simply deprecate the field, without providing any replacement. > > Until we do eventually delete the member, correct the misstatements > in the QAPI documentation about it. > > If we do find that there are users for this, then the most likely way > we would provide replacement access to the information would be to > put the accelerator QOM object at a well-known path such as > /machine/accel, which could then be used with the existing qom-set > and qom-get commands. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Markus Armbruster <armbru@redhat.com> > Message-id: 20230417164041.684562-11-peter.maydell@linaro.org > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 6f5e689aa4..d5eda0f566 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -199,6 +199,20 @@ accepted incorrect commands will return an error. Users should make sure that > all arguments passed to ``device_add`` are consistent with the documented > property types. > > +``StatusInfo`` member ``singlestep`` (since 8.1) > +'''''''''''''''''''''''''''''''''''''''''''''''' > + > +The ``singlestep`` member of the ``StatusInfo`` returned from the > +``query-status`` command is deprecated. This member has a confusing > +name and it never did what the documentation claimed or what its name > +suggests. We do not believe that anybody is actually using the > +information provided in this member. > + > +The information it reports is whether the TCG JIT is in "one > +instruction per translated block" mode (which can be set on the > +command line or via the HMP, but not via QMP). The information remains > +available via the HMP 'info jit' command. > + > [...] >
On 16/01/2024 10.46, Philippe Mathieu-Daudé wrote: > On 16/1/24 07:27, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >>> On Mon, Jan 15, 2024 at 05:39:19PM +0000, Peter Maydell wrote: >>>> On Mon, 15 Jan 2024 at 13:54, Thomas Huth <thuth@redhat.com> wrote: >>>>> >>>>> On 12/01/2024 16.39, Philippe Mathieu-Daudé wrote: >>>>>> Hi Thomas >>>>>> >>>>>> +Laurent & Peter >>>>>> >>>>>> On 12/1/24 11:00, Thomas Huth wrote: >>>>>>> It's been marked as deprecated since QEMU 8.1, so it should be fine >>>>>>> to remove this now. >>>>>>> >>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> >>>>>> StatusInfo::singlestep was deprecated at the same time, >>>>>> can we remove it? >>>>>> >>>>>> IOW could we complete your patch with this? >>>> >>>>>> diff --git a/qapi/run-state.json b/qapi/run-state.json >>>>>> index ca05502e0a..08bc99cb85 100644 >>>>>> --- a/qapi/run-state.json >>>>>> +++ b/qapi/run-state.json >>>>>> @@ -106,25 +106,15 @@ >>>>>> # >>>>>> # @running: true if all VCPUs are runnable, false if not runnable >>>>>> # >>>>>> -# @singlestep: true if using TCG with one guest instruction per >>>>>> -# translation block >>>>>> -# >>>>>> # @status: the virtual machine @RunState >>>>>> # >>>>>> # Features: >>>>>> # >>>>>> -# @deprecated: Member 'singlestep' is deprecated (with no >>>>>> -# replacement). >>>>>> -# >>>>>> # Since: 0.14 >>>>>> # >>>>>> -# Notes: @singlestep is enabled on the command line with '-accel >>>>>> -# tcg,one-insn-per-tb=on', or with the HMP 'one-insn-per-tb' >>>>>> -# command. >>>>>> ## >>>>>> { 'struct': 'StatusInfo', >>>>>> 'data': {'running': 'bool', >>>>>> - 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' >>>>>> ]}, >>>>>> 'status': 'RunState'} } >>>>> >>>>> Uh, oh, that's a bigger change already ... can we safely remove the field >>>>> here without upsetting 3rd party apps that rely on this interface? >>>> >>>> That was the whole point of marking it 'deprecated' in the JSON, >>>> I thought? We don't think anybody's using it, we've given fair >>>> warning, isn't the next step "remove it"? Markus, you're the >>>> expert on QAPI deprecations... >>> >>> Yes, it is fine to delete it without thinking further about possible usage, >>> unless someone steps forward quickly with new information that wasn't known >>> when the deprecation was added.... >> >> Concur. > > Thanks all for the feedback. > > Thomas, are you OK to post a v2 with the changes I suggested > or do you want me to do it? Since your changes were bigger than mine, I think it's just fair if you take credit for the patch. So yes, please go ahead and assemble it as a v2! Thanks! Thomas
Thomas Huth <thuth@redhat.com> writes: > It's been marked as deprecated since QEMU 8.1, so it should be fine > to remove this now. > > Signed-off-by: Thomas Huth <thuth@redhat.com> Should we drop HMP command singlestep, too?
On 17/1/24 13:42, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> It's been marked as deprecated since QEMU 8.1, so it should be fine >> to remove this now. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > Should we drop HMP command singlestep, too? Yes, patch on the way.
On 16/1/24 10:52, Thomas Huth wrote: > On 16/01/2024 10.46, Philippe Mathieu-Daudé wrote: >> Thomas, are you OK to post a v2 with the changes I suggested >> or do you want me to do it? > > Since your changes were bigger than mine, I think it's just fair if you > take credit for the patch. So yes, please go ahead and assemble it as a > v2! Thanks! Sent only respin of this patch based on first 4 (untouched): https://lore.kernel.org/qemu-devel/20240117151430.29235-1-philmd@linaro.org/ (neglected to name it v2)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index b50cfe596b..81a5149f1e 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -63,12 +63,6 @@ as short-form boolean values, and passed to plugins as ``arg_name=on``. However, short-form booleans are deprecated and full explicit ``arg_name=on`` form is preferred. -``-singlestep`` (since 8.1) -''''''''''''''''''''''''''' - -The ``-singlestep`` option has been turned into an accelerator property, -and given a name that better reflects what it actually does. -Use ``-accel tcg,one-insn-per-tb=on`` instead. User-mode emulator command line arguments ----------------------------------------- diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst index a8546f4787..d5c60ff47f 100644 --- a/docs/about/removed-features.rst +++ b/docs/about/removed-features.rst @@ -482,6 +482,13 @@ Use ``-run-with async-teardown=on`` instead. Use ``-run-with chroot=dir`` instead. +``-singlestep`` (removed in 9.0) +'''''''''''''''''''''''''''''''' + +The ``-singlestep`` option has been turned into an accelerator property, +and given a name that better reflects what it actually does. +Use ``-accel tcg,one-insn-per-tb=on`` instead. + QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/system/vl.c b/system/vl.c index c125fb9079..809f867bcc 100644 --- a/system/vl.c +++ b/system/vl.c @@ -181,7 +181,6 @@ static const char *log_file; static bool list_data_dirs; static const char *qtest_chrdev; static const char *qtest_log; -static bool opt_one_insn_per_tb; static int has_defaults = 1; static int default_audio = 1; @@ -2308,19 +2307,7 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp) qemu_opt_foreach(opts, accelerator_set_property, accel, &error_fatal); - /* - * If legacy -singlestep option is set, honour it for TCG and - * silently ignore for any other accelerator (which is how this - * option has always behaved). - */ - if (opt_one_insn_per_tb) { - /* - * This will always succeed for TCG, and we want to ignore - * the error from trying to set a nonexistent property - * on any other accelerator. - */ - object_property_set_bool(OBJECT(accel), "one-insn-per-tb", true, NULL); - } + ret = accel_init_machine(accel, current_machine); if (ret < 0) { if (!qtest_with_kvm || ret != -ENOENT) { @@ -3057,9 +3044,6 @@ void qemu_init(int argc, char **argv) case QEMU_OPTION_bios: qdict_put_str(machine_opts_dict, "firmware", optarg); break; - case QEMU_OPTION_singlestep: - opt_one_insn_per_tb = true; - break; case QEMU_OPTION_S: autostart = 0; break; diff --git a/qemu-options.hx b/qemu-options.hx index 9be6beb5a0..033fa873e4 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4357,14 +4357,6 @@ SRST from a script. ERST -DEF("singlestep", 0, QEMU_OPTION_singlestep, \ - "-singlestep deprecated synonym for -accel tcg,one-insn-per-tb=on\n", QEMU_ARCH_ALL) -SRST -``-singlestep`` - This is a deprecated synonym for the TCG accelerator property - ``one-insn-per-tb``. -ERST - DEF("preconfig", 0, QEMU_OPTION_preconfig, \ "--preconfig pause QEMU before machine is initialized (experimental)\n", QEMU_ARCH_ALL)
It's been marked as deprecated since QEMU 8.1, so it should be fine to remove this now. Signed-off-by: Thomas Huth <thuth@redhat.com> --- docs/about/deprecated.rst | 6 ------ docs/about/removed-features.rst | 7 +++++++ system/vl.c | 18 +----------------- qemu-options.hx | 8 -------- 4 files changed, 8 insertions(+), 31 deletions(-)