Discussion:
[Qemu-devel] [PATCH v2 4/5] qemu-options: Remove deprecated -no-kvm-irqchip
Thomas Huth
2018-05-04 17:01:09 UTC
Permalink
We've never documented this option in our qemu-doc, so apart from the users
that already used the old qemu-kvm fork before, most users should not be
aware of this option at all. It's been marked as deprecated in the source
code for a long time already, and officially marked as deprecated in the
documentation since QEMU v2.10, so it should be fine to remove this now.

Reviewed-by: Markus Armbruster <***@redhat.com>
Signed-off-by: Thomas Huth <***@redhat.com>
---
qemu-doc.texi | 5 -----
qemu-options.hx | 3 ---
vl.c | 5 -----
3 files changed, 13 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index 2047ba5..9db156b 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2786,11 +2786,6 @@ which is the default.

@section System emulator command line arguments

-@subsection -no-kvm-irqchip (since 1.3.0)
-
-The ``-no-kvm-irqchip'' argument is now a synonym for
-setting ``-machine kernel_irqchip=off''.
-
@subsection -no-kvm (since 1.3.0)

The ``-no-kvm'' argument is now a synonym for setting
diff --git a/qemu-options.hx b/qemu-options.hx
index 9c0d60d..870a363 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3917,9 +3917,6 @@ ETEXI
HXCOMM Deprecated by -machine accel=tcg property
DEF("no-kvm", 0, QEMU_OPTION_no_kvm, "", QEMU_ARCH_I386)

-HXCOMM Deprecated by -machine kernel_irqchip=on|off property
-DEF("no-kvm-irqchip", 0, QEMU_OPTION_no_kvm_irqchip, "", QEMU_ARCH_I386)
-
DEF("msg", HAS_ARG, QEMU_OPTION_msg,
"-msg timestamp[=on|off]\n"
" change the format of messages\n"
diff --git a/vl.c b/vl.c
index bbe94ba..d8c3b09 100644
--- a/vl.c
+++ b/vl.c
@@ -3149,11 +3149,6 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
switch(popt->index) {
- case QEMU_OPTION_no_kvm_irqchip: {
- olist = qemu_find_opts("machine");
- qemu_opts_parse_noisily(olist, "kernel_irqchip=off", false);
- break;
- }
case QEMU_OPTION_cpu:
/* hw initialization will check this */
cpu_model = optarg;
--
1.8.3.1
Thomas Huth
2018-05-04 17:01:06 UTC
Permalink
The -tdf options has been removed with d07aa197c5a1556449361a0cbb5108e2,
but apparently I forgot to remove the corresponding two lines from
qemu-options.hx, so this option is still "available" and just silently
ignored. Kill it now for good.

Reviewed-by: Markus Armbruster <***@redhat.com>
Signed-off-by: Thomas Huth <***@redhat.com>
---
qemu-options.hx | 3 ---
1 file changed, 3 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index c611766..44daa10 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3924,9 +3924,6 @@ DEF("no-kvm-pit-reinjection", 0, QEMU_OPTION_no_kvm_pit_reinjection,
HXCOMM Deprecated by -machine kernel_irqchip=on|off property
DEF("no-kvm-irqchip", 0, QEMU_OPTION_no_kvm_irqchip, "", QEMU_ARCH_I386)

-HXCOMM Deprecated (ignored)
-DEF("tdf", 0, QEMU_OPTION_tdf,"", QEMU_ARCH_ALL)
-
DEF("msg", HAS_ARG, QEMU_OPTION_msg,
"-msg timestamp[=on|off]\n"
" change the format of messages\n"
--
1.8.3.1
Thomas Huth
2018-05-04 17:01:10 UTC
Permalink
We've never documented this option in our qemu-doc, so apart from the users
that already used the old qemu-kvm fork before, most users should not be
aware of this option at all. It's been marked as deprecated in the source
code for a long time already, and officially marked as deprecated in the
documentation since QEMU v2.10, so it should be fine to remove this now.

Reviewed-by: Markus Armbruster <***@redhat.com>
Signed-off-by: Thomas Huth <***@redhat.com>
---
qemu-doc.texi | 5 -----
qemu-options.hx | 3 ---
vl.c | 4 ----
3 files changed, 12 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index 9db156b..b6e0a18 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2786,11 +2786,6 @@ which is the default.

@section System emulator command line arguments

-@subsection -no-kvm (since 1.3.0)
-
-The ``-no-kvm'' argument is now a synonym for setting
-``-machine accel=tcg''.
-
@subsection -vnc tls (since 2.5.0)

The ``-vnc tls'' argument is now a synonym for setting
diff --git a/qemu-options.hx b/qemu-options.hx
index 870a363..d77a968 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3914,9 +3914,6 @@ STEXI
Enable FIPS 140-2 compliance mode.
ETEXI

-HXCOMM Deprecated by -machine accel=tcg property
-DEF("no-kvm", 0, QEMU_OPTION_no_kvm, "", QEMU_ARCH_I386)
-
DEF("msg", HAS_ARG, QEMU_OPTION_msg,
"-msg timestamp[=on|off]\n"
" change the format of messages\n"
diff --git a/vl.c b/vl.c
index d8c3b09..2ac41e2 100644
--- a/vl.c
+++ b/vl.c
@@ -3696,10 +3696,6 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
break;
- case QEMU_OPTION_no_kvm:
- olist = qemu_find_opts("machine");
- qemu_opts_parse_noisily(olist, "accel=tcg", false);
- break;
case QEMU_OPTION_accel:
accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
optarg, true);
--
1.8.3.1
Thomas Huth
2018-05-04 17:01:08 UTC
Permalink
Deprecated since the beginning when it was added for compatibility with
the ancient qemu-kvm fork of QEMU, and it even printed out the deprecation
warning since right from the start (i.e. QEMU v1.3.0), so it's really time
to remove this now.

Reviewed-by: Markus Armbruster <***@redhat.com>
Signed-off-by: Thomas Huth <***@redhat.com>
---
qemu-doc.texi | 5 -----
qemu-options.hx | 4 ----
vl.c | 12 ------------
3 files changed, 21 deletions(-)

diff --git a/qemu-doc.texi b/qemu-doc.texi
index 0ed0f19..2047ba5 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2786,11 +2786,6 @@ which is the default.

@section System emulator command line arguments

-@subsection -no-kvm-pit-reinjection (since 1.3.0)
-
-The ``-no-kvm-pit-reinjection'' argument is now a
-synonym for setting ``-global kvm-pit.lost_tick_policy=discard''.
-
@subsection -no-kvm-irqchip (since 1.3.0)

The ``-no-kvm-irqchip'' argument is now a synonym for
diff --git a/qemu-options.hx b/qemu-options.hx
index 44daa10..9c0d60d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3917,10 +3917,6 @@ ETEXI
HXCOMM Deprecated by -machine accel=tcg property
DEF("no-kvm", 0, QEMU_OPTION_no_kvm, "", QEMU_ARCH_I386)

-HXCOMM Deprecated by kvm-pit driver properties
-DEF("no-kvm-pit-reinjection", 0, QEMU_OPTION_no_kvm_pit_reinjection,
- "", QEMU_ARCH_I386)
-
HXCOMM Deprecated by -machine kernel_irqchip=on|off property
DEF("no-kvm-irqchip", 0, QEMU_OPTION_no_kvm_irqchip, "", QEMU_ARCH_I386)

diff --git a/vl.c b/vl.c
index 457e93b..bbe94ba 100644
--- a/vl.c
+++ b/vl.c
@@ -3705,18 +3705,6 @@ int main(int argc, char **argv, char **envp)
olist = qemu_find_opts("machine");
qemu_opts_parse_noisily(olist, "accel=tcg", false);
break;
- case QEMU_OPTION_no_kvm_pit_reinjection: {
- static GlobalProperty kvm_pit_lost_tick_policy = {
- .driver = "kvm-pit",
- .property = "lost_tick_policy",
- .value = "discard",
- };
-
- warn_report("deprecated, replaced by "
- "-global kvm-pit.lost_tick_policy=discard");
- qdev_prop_register_global(&kvm_pit_lost_tick_policy);
- break;
- }
case QEMU_OPTION_accel:
accel_opts = qemu_opts_parse_noisily(qemu_find_opts("accel"),
optarg, true);
--
1.8.3.1
Thomas Huth
2018-05-04 17:01:07 UTC
Permalink
The dangling remainder of the -tdf option revealed a deficiency in our
option parsing: Options that have been declared, but are not supported
in the switch-case statement in vl.c and not handled in the OS-specifc
os_parse_cmd_args() functions are currently silently ignored. We should
rather tell the users that they specified something that we can not
handle, so let's print an error message and exit instead.

Reported-by: Markus Armbruster <***@redhat.com>
Signed-off-by: Thomas Huth <***@redhat.com>
---
include/qemu-common.h | 2 +-
os-posix.c | 6 +++++-
os-win32.c | 4 ++--
vl.c | 5 ++++-
4 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/qemu-common.h b/include/qemu-common.h
index 8a4f63c..85f4749 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -137,7 +137,7 @@ char *qemu_find_file(int type, const char *name);
/* OS specific functions */
void os_setup_early_signal_handling(void);
char *os_find_datadir(void);
-void os_parse_cmd_args(int index, const char *optarg);
+int os_parse_cmd_args(int index, const char *optarg);

#include "qemu/module.h"

diff --git a/os-posix.c b/os-posix.c
index 24eb700..9ce6f74 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -165,7 +165,7 @@ static bool os_parse_runas_uid_gid(const char *optarg)
* Parse OS specific command line options.
* return 0 if option handled, -1 otherwise
*/
-void os_parse_cmd_args(int index, const char *optarg)
+int os_parse_cmd_args(int index, const char *optarg)
{
switch (index) {
#ifdef CONFIG_SLIRP
@@ -199,7 +199,11 @@ void os_parse_cmd_args(int index, const char *optarg)
fips_set_state(true);
break;
#endif
+ default:
+ return -1;
}
+
+ return 0;
}

static void change_process_uid(void)
diff --git a/os-win32.c b/os-win32.c
index 586a7c7..0674f94 100644
--- a/os-win32.c
+++ b/os-win32.c
@@ -93,9 +93,9 @@ void os_set_line_buffering(void)
* Parse OS specific command line options.
* return 0 if option handled, -1 otherwise
*/
-void os_parse_cmd_args(int index, const char *optarg)
+int os_parse_cmd_args(int index, const char *optarg)
{
- return;
+ return -1;
}

int qemu_create_pidfile(const char *filename)
diff --git a/vl.c b/vl.c
index 806eec2..457e93b 100644
--- a/vl.c
+++ b/vl.c
@@ -4042,7 +4042,10 @@ int main(int argc, char **argv, char **envp)
}
break;
default:
- os_parse_cmd_args(popt->index, optarg);
+ if (os_parse_cmd_args(popt->index, optarg)) {
+ error_report("Option not supported in this build");
+ exit(1);
+ }
}
}
}
--
1.8.3.1
Anthony PERARD
2018-05-16 16:49:00 UTC
Permalink
Post by Thomas Huth
diff --git a/vl.c b/vl.c
index 806eec2..457e93b 100644
--- a/vl.c
+++ b/vl.c
@@ -4042,7 +4042,10 @@ int main(int argc, char **argv, char **envp)
}
break;
- os_parse_cmd_args(popt->index, optarg);
+ if (os_parse_cmd_args(popt->index, optarg)) {
+ error_report("Option not supported in this build");
+ exit(1);
+ }
I think that report false errors, I've got:
qemu-system-i386: -no-user-config: Option not supported in this build

Regards,
--
Anthony PERARD
Thomas Huth
2018-05-16 16:51:32 UTC
Permalink
Post by Anthony PERARD
Post by Thomas Huth
diff --git a/vl.c b/vl.c
index 806eec2..457e93b 100644
--- a/vl.c
+++ b/vl.c
@@ -4042,7 +4042,10 @@ int main(int argc, char **argv, char **envp)
}
break;
- os_parse_cmd_args(popt->index, optarg);
+ if (os_parse_cmd_args(popt->index, optarg)) {
+ error_report("Option not supported in this build");
+ exit(1);
+ }
qemu-system-i386: -no-user-config: Option not supported in this build
Yes, sorry for that. Patch is already on the way:

https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg02972.html

Thomas
Paolo Bonzini
2018-05-04 21:57:53 UTC
Permalink
The -no-kvm* options are a remainder of the ancient "qemu-kvm"
fork. They have never been officially documented in our qemu-doc,
they have been marked as deprecated in the sources since a very
long time, and we've marked them as deprecated in our qemu-doc
since QEMU v2.10. So far I haven't seen any complaints that we
should keep them, so it's time to remove these old parameters now.
While I'm at it, this patch series also removes a left-over from
the "-tdf" option (which has been removed with QEMU v2.12) and
fixes a deficiency in the option parsing code that has been
revealed by the remainder of the "-tdf" option.
qemu-options: Remove remainders of the -tdf option
qemu-options: Bail out on unsupported options instead of silently
ignoring them
qemu-options: Remove deprecated -no-kvm-pit-reinjection
qemu-options: Remove deprecated -no-kvm-irqchip
qemu-options: Remove deprecated -no-kvm
I'm not that sure anymore about -no-kvm. It can come in handy for
distros (*cough* RHEL *cough) that only ship a qemu-kvm binary with
default accelerator "-machine accel=kvm:tcg".

Yeah, "-accel tcg" is only a few characters later, but it is a
relatively recent addition.

Paolo
Markus Armbruster
2018-05-07 05:37:47 UTC
Permalink
Post by Paolo Bonzini
The -no-kvm* options are a remainder of the ancient "qemu-kvm"
fork. They have never been officially documented in our qemu-doc,
they have been marked as deprecated in the sources since a very
long time, and we've marked them as deprecated in our qemu-doc
since QEMU v2.10. So far I haven't seen any complaints that we
should keep them, so it's time to remove these old parameters now.
While I'm at it, this patch series also removes a left-over from
the "-tdf" option (which has been removed with QEMU v2.12) and
fixes a deficiency in the option parsing code that has been
revealed by the remainder of the "-tdf" option.
qemu-options: Remove remainders of the -tdf option
qemu-options: Bail out on unsupported options instead of silently
ignoring them
qemu-options: Remove deprecated -no-kvm-pit-reinjection
qemu-options: Remove deprecated -no-kvm-irqchip
qemu-options: Remove deprecated -no-kvm
I'm not that sure anymore about -no-kvm. It can come in handy for
distros (*cough* RHEL *cough) that only ship a qemu-kvm binary with
default accelerator "-machine accel=kvm:tcg".
Yeah, "-accel tcg" is only a few characters later, but it is a
relatively recent addition.
2012 is "relatively recent" in the alternate RHEL universe, perhaps :)


Author: Jan Kiszka <***@siemens.com>
Date: Fri Oct 5 14:51:45 2012 -0300

Emulate qemu-kvms -no-kvm option

Releases of qemu-kvm will be interrupted at qemu 1.3.0.
Users should switch to plain qemu releases.
To avoid breaking scenarios which are setup with command line
options specific to qemu-kvm, port these switches from qemu-kvm
to qemu.git.

Port -no-kvm option.

Signed-off-by: Marcelo Tosatti <***@redhat.com>

diff --git a/qemu-options.hx b/qemu-options.hx
index 628bd44591..fe8f15c541 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2888,6 +2888,9 @@ STEXI
Enable FIPS 140-2 compliance mode.
ETEXI

+HXCOMM Deprecated by -machine accel=tcg property
+DEF("no-kvm", HAS_ARG, QEMU_OPTION_no_kvm, "", QEMU_ARCH_I386)
+
HXCOMM Deprecated by kvm-pit driver properties
DEF("no-kvm-pit-reinjection", HAS_ARG, QEMU_OPTION_no_kvm_pit_reinjection,
"", QEMU_ARCH_I386)
diff --git a/vl.c b/vl.c
index cd7c0fbd52..b39f22ed35 100644
--- a/vl.c
+++ b/vl.c
@@ -3171,6 +3171,10 @@ int main(int argc, char **argv, char **envp)
machine = machine_parse(optarg);
}
break;
+ case QEMU_OPTION_no_kvm:
+ olist = qemu_find_opts("machine");
+ qemu_opts_parse(olist, "accel=tcg", 0);
+ break;
case QEMU_OPTION_no_kvm_pit: {
fprintf(stderr, "Warning: KVM PIT can no longer be disabled "
"separately.\n");
Thomas Huth
2018-05-07 05:45:12 UTC
Permalink
Post by Markus Armbruster
Post by Paolo Bonzini
The -no-kvm* options are a remainder of the ancient "qemu-kvm"
fork. They have never been officially documented in our qemu-doc,
they have been marked as deprecated in the sources since a very
long time, and we've marked them as deprecated in our qemu-doc
since QEMU v2.10. So far I haven't seen any complaints that we
should keep them, so it's time to remove these old parameters now.
While I'm at it, this patch series also removes a left-over from
the "-tdf" option (which has been removed with QEMU v2.12) and
fixes a deficiency in the option parsing code that has been
revealed by the remainder of the "-tdf" option.
qemu-options: Remove remainders of the -tdf option
qemu-options: Bail out on unsupported options instead of silently
ignoring them
qemu-options: Remove deprecated -no-kvm-pit-reinjection
qemu-options: Remove deprecated -no-kvm-irqchip
qemu-options: Remove deprecated -no-kvm
I'm not that sure anymore about -no-kvm. It can come in handy for
distros (*cough* RHEL *cough) that only ship a qemu-kvm binary with
default accelerator "-machine accel=kvm:tcg".
Yeah, "-accel tcg" is only a few characters later, but it is a
relatively recent addition.
2012 is "relatively recent" in the alternate RHEL universe, perhaps :)
That was the "-machine accel=tcg", but Paolo was rather talking about
"-accel tcg", which has been added last year. Yes, we've got three (!)
ways of dealing with accelerators now: "-M accel=", "-accel" and
"-enable-kvm + -no-kvm". We're very userfriendly, aren't we? ;-)

Thomas
Markus Armbruster
2018-05-07 07:56:32 UTC
Permalink
Post by Thomas Huth
Post by Markus Armbruster
Post by Paolo Bonzini
The -no-kvm* options are a remainder of the ancient "qemu-kvm"
fork. They have never been officially documented in our qemu-doc,
they have been marked as deprecated in the sources since a very
long time, and we've marked them as deprecated in our qemu-doc
since QEMU v2.10. So far I haven't seen any complaints that we
should keep them, so it's time to remove these old parameters now.
While I'm at it, this patch series also removes a left-over from
the "-tdf" option (which has been removed with QEMU v2.12) and
fixes a deficiency in the option parsing code that has been
revealed by the remainder of the "-tdf" option.
qemu-options: Remove remainders of the -tdf option
qemu-options: Bail out on unsupported options instead of silently
ignoring them
qemu-options: Remove deprecated -no-kvm-pit-reinjection
qemu-options: Remove deprecated -no-kvm-irqchip
qemu-options: Remove deprecated -no-kvm
I'm not that sure anymore about -no-kvm. It can come in handy for
distros (*cough* RHEL *cough) that only ship a qemu-kvm binary with
default accelerator "-machine accel=kvm:tcg".
Yeah, "-accel tcg" is only a few characters later, but it is a
relatively recent addition.
2012 is "relatively recent" in the alternate RHEL universe, perhaps :)
That was the "-machine accel=tcg", but Paolo was rather talking about
"-accel tcg", which has been added last year. Yes, we've got three (!)
ways of dealing with accelerators now: "-M accel=", "-accel" and
"-enable-kvm + -no-kvm". We're very userfriendly, aren't we? ;-)
Ugh. Apparently, vl.c still is a playground without adult supervision.
Paolo Bonzini
2018-05-07 11:02:02 UTC
Permalink
Post by Markus Armbruster
Post by Thomas Huth
Post by Markus Armbruster
Post by Paolo Bonzini
I'm not that sure anymore about -no-kvm. It can come in handy for
distros (*cough* RHEL *cough) that only ship a qemu-kvm binary with
default accelerator "-machine accel=kvm:tcg".
Yeah, "-accel tcg" is only a few characters later, but it is a
relatively recent addition.
2012 is "relatively recent" in the alternate RHEL universe, perhaps :)
That was the "-machine accel=tcg", but Paolo was rather talking about
"-accel tcg", which has been added last year. Yes, we've got three (!)
ways of dealing with accelerators now: "-M accel=", "-accel" and
"-enable-kvm + -no-kvm". We're very userfriendly, aren't we? ;-)
Ugh. Apparently, vl.c still is a playground without adult supervision.
I'm not sure I get it... "-accel" was added in order to provide
accelerator options. If anything, "-machine accel" (added back in 2010)
is the one that makes little sense these days.

Paolo
Markus Armbruster
2018-05-07 11:56:56 UTC
Permalink
Post by Paolo Bonzini
Post by Markus Armbruster
Post by Thomas Huth
Post by Markus Armbruster
Post by Paolo Bonzini
I'm not that sure anymore about -no-kvm. It can come in handy for
distros (*cough* RHEL *cough) that only ship a qemu-kvm binary with
default accelerator "-machine accel=kvm:tcg".
Yeah, "-accel tcg" is only a few characters later, but it is a
relatively recent addition.
2012 is "relatively recent" in the alternate RHEL universe, perhaps :)
That was the "-machine accel=tcg", but Paolo was rather talking about
"-accel tcg", which has been added last year. Yes, we've got three (!)
ways of dealing with accelerators now: "-M accel=", "-accel" and
"-enable-kvm + -no-kvm". We're very userfriendly, aren't we? ;-)
Ugh. Apparently, vl.c still is a playground without adult supervision.
I'm not sure I get it... "-accel" was added in order to provide
accelerator options. If anything, "-machine accel" (added back in 2010)
is the one that makes little sense these days.
Adding more and more ways to do the same stuff does not improve an
interface. Interface design needs to be *opinionated*. If we decide
-machine accel=tcg isn't a nice interface, by all means create a better
one, but as replacement[*], not as addition.

Furthermore:

tcg: add options for enabling MTTCG

We know there will be cases where MTTCG won't work until additional work
is done in the front/back ends to support. It will however be useful to
be able to turn it on.

As a result MTTCG will default to off unless the combination is
supported. However the user can turn it on for the sake of testing.

Signed-off-by: KONRAD Frederic <***@greensocs.com>
[AJB: move to -accel tcg,thread=multi|single, defaults]
Signed-off-by: Alex Bennée <***@linaro.org>
Reviewed-by: Richard Henderson <***@twiddle.net>

I'm sorry, but this us sub-par. Yes, the commit is also about "enabling
MTTCG", but it also adds a new way to select accelerators, without ever
spelling that out. It should've been split, and properly described.


[*] Replacements generally involve deprecation and a grace period.
Paolo Bonzini
2018-05-07 12:44:42 UTC
Permalink
Post by Markus Armbruster
Adding more and more ways to do the same stuff does not improve an
interface. Interface design needs to be *opinionated*. If we decide
-machine accel=tcg isn't a nice interface, by all means create a better
one, but as replacement[*], not as addition.
tcg: add options for enabling MTTCG
We know there will be cases where MTTCG won't work until additional work
is done in the front/back ends to support. It will however be useful to
be able to turn it on.
As a result MTTCG will default to off unless the combination is
supported. However the user can turn it on for the sake of testing.
[AJB: move to -accel tcg,thread=multi|single, defaults]
I'm sorry, but this us sub-par. Yes, the commit is also about "enabling
MTTCG", but it also adds a new way to select accelerators, without ever
spelling that out. It should've been split, and properly described.
Perhaps we can deprecate "-M accel" then, and also while we're at it
move kernel_irqchip from -machine to "-accel kvm" where it belongs?

Paolo
Markus Armbruster
2018-05-07 16:50:54 UTC
Permalink
Post by Paolo Bonzini
Post by Markus Armbruster
Adding more and more ways to do the same stuff does not improve an
interface. Interface design needs to be *opinionated*. If we decide
-machine accel=tcg isn't a nice interface, by all means create a better
one, but as replacement[*], not as addition.
tcg: add options for enabling MTTCG
We know there will be cases where MTTCG won't work until additional work
is done in the front/back ends to support. It will however be useful to
be able to turn it on.
As a result MTTCG will default to off unless the combination is
supported. However the user can turn it on for the sake of testing.
[AJB: move to -accel tcg,thread=multi|single, defaults]
I'm sorry, but this us sub-par. Yes, the commit is also about "enabling
MTTCG", but it also adds a new way to select accelerators, without ever
spelling that out. It should've been split, and properly described.
Perhaps we can deprecate "-M accel" then, and also while we're at it
move kernel_irqchip from -machine to "-accel kvm" where it belongs?
Sounds good to me.
Paolo Bonzini
2018-05-07 17:01:11 UTC
Permalink
Post by Markus Armbruster
Post by Paolo Bonzini
Post by Markus Armbruster
Adding more and more ways to do the same stuff does not improve an
interface. Interface design needs to be *opinionated*. If we decide
-machine accel=tcg isn't a nice interface, by all means create a better
one, but as replacement[*], not as addition.
tcg: add options for enabling MTTCG
We know there will be cases where MTTCG won't work until additional work
is done in the front/back ends to support. It will however be useful to
be able to turn it on.
As a result MTTCG will default to off unless the combination is
supported. However the user can turn it on for the sake of testing.
[AJB: move to -accel tcg,thread=multi|single, defaults]
I'm sorry, but this us sub-par. Yes, the commit is also about "enabling
MTTCG", but it also adds a new way to select accelerators, without ever
spelling that out. It should've been split, and properly described.
Perhaps we can deprecate "-M accel" then, and also while we're at it
move kernel_irqchip from -machine to "-accel kvm" where it belongs?
Sounds good to me.
Thomas, here's one for you! :)

Paolo
Thomas Huth
2018-05-16 16:59:03 UTC
Permalink
Post by Paolo Bonzini
Post by Markus Armbruster
Post by Paolo Bonzini
Post by Markus Armbruster
Adding more and more ways to do the same stuff does not improve an
interface. Interface design needs to be *opinionated*. If we decide
-machine accel=tcg isn't a nice interface, by all means create a better
one, but as replacement[*], not as addition.
tcg: add options for enabling MTTCG
We know there will be cases where MTTCG won't work until additional work
is done in the front/back ends to support. It will however be useful to
be able to turn it on.
As a result MTTCG will default to off unless the combination is
supported. However the user can turn it on for the sake of testing.
[AJB: move to -accel tcg,thread=multi|single, defaults]
I'm sorry, but this us sub-par. Yes, the commit is also about "enabling
MTTCG", but it also adds a new way to select accelerators, without ever
spelling that out. It should've been split, and properly described.
Perhaps we can deprecate "-M accel" then, and also while we're at it
move kernel_irqchip from -machine to "-accel kvm" where it belongs?
Sounds good to me.
Thomas, here's one for you! :)
I like the idea of deprecating "-machine accel=xxx" - but that will also
be a bigger change, since "-accel" is internally setting the accel
option of the machine again...

Also libvirt is still using "-machine accel=..." as far as I know, so
we've got to make sure that this gets changed there, too...

Thomas

Paolo Bonzini
2018-05-07 11:00:15 UTC
Permalink
Post by Markus Armbruster
Post by Paolo Bonzini
I'm not that sure anymore about -no-kvm. It can come in handy for
distros (*cough* RHEL *cough) that only ship a qemu-kvm binary with
default accelerator "-machine accel=kvm:tcg".
Yeah, "-accel tcg" is only a few characters later, but it is a
relatively recent addition.
2012 is "relatively recent" in the alternate RHEL universe, perhaps :)
Well, that's for "-machine accel=tcg" which is quite a mouthful.

"-accel tcg" was added in 2.9, about a year ago (commit 8d4e9146b3,
"tcg: add options for enabling MTTCG", 2017-02-24).

Paolo
Post by Markus Armbruster
Date: Fri Oct 5 14:51:45 2012 -0300
Emulate qemu-kvms -no-kvm option
Releases of qemu-kvm will be interrupted at qemu 1.3.0.
Users should switch to plain qemu releases.
To avoid breaking scenarios which are setup with command line
options specific to qemu-kvm, port these switches from qemu-kvm
to qemu.git.
Port -no-kvm option.
Markus Armbruster
2018-05-07 11:47:39 UTC
Permalink
Post by Paolo Bonzini
Post by Markus Armbruster
Post by Paolo Bonzini
I'm not that sure anymore about -no-kvm. It can come in handy for
distros (*cough* RHEL *cough) that only ship a qemu-kvm binary with
default accelerator "-machine accel=kvm:tcg".
Yeah, "-accel tcg" is only a few characters later, but it is a
relatively recent addition.
2012 is "relatively recent" in the alternate RHEL universe, perhaps :)
Well, that's for "-machine accel=tcg" which is quite a mouthful.
"-M accel=tcg" is a whopping *two* characters more than "-accel tcg".
Post by Paolo Bonzini
"-accel tcg" was added in 2.9, about a year ago (commit 8d4e9146b3,
"tcg: add options for enabling MTTCG", 2017-02-24).
Yes, I confused the two.
Thomas Huth
2018-05-07 05:09:02 UTC
Permalink
Post by Paolo Bonzini
The -no-kvm* options are a remainder of the ancient "qemu-kvm"
fork. They have never been officially documented in our qemu-doc,
they have been marked as deprecated in the sources since a very
long time, and we've marked them as deprecated in our qemu-doc
since QEMU v2.10. So far I haven't seen any complaints that we
should keep them, so it's time to remove these old parameters now.
While I'm at it, this patch series also removes a left-over from
the "-tdf" option (which has been removed with QEMU v2.12) and
fixes a deficiency in the option parsing code that has been
revealed by the remainder of the "-tdf" option.
qemu-options: Remove remainders of the -tdf option
qemu-options: Bail out on unsupported options instead of silently
ignoring them
qemu-options: Remove deprecated -no-kvm-pit-reinjection
qemu-options: Remove deprecated -no-kvm-irqchip
qemu-options: Remove deprecated -no-kvm
I'm not that sure anymore about -no-kvm. It can come in handy for
distros (*cough* RHEL *cough) that only ship a qemu-kvm binary with
default accelerator "-machine accel=kvm:tcg".
Actually, I expected someone to complain like this :-) That's also why I
kept the patches separate instead of doing one big remove-all-no-kvm
parameters patch. I also remember seeing some people still using this
option (in bug tickets etc.), so it's fine for me if we decide to keep
-no-kvm for now.

Question is, should we keep it just for some more few releases and then
remove it? In that case we should maybe add a patch that prints out a
warning message if somebody still tries to use it. Or do we rather want
to keep it around forever? In that case we should likely remove the
entry from the deprecation list.

Thomas
Paolo Bonzini
2018-05-04 21:58:10 UTC
Permalink
The -no-kvm* options are a remainder of the ancient "qemu-kvm"
fork. They have never been officially documented in our qemu-doc,
they have been marked as deprecated in the sources since a very
long time, and we've marked them as deprecated in our qemu-doc
since QEMU v2.10. So far I haven't seen any complaints that we
should keep them, so it's time to remove these old parameters now.
While I'm at it, this patch series also removes a left-over from
the "-tdf" option (which has been removed with QEMU v2.12) and
fixes a deficiency in the option parsing code that has been
revealed by the remainder of the "-tdf" option.
qemu-options: Remove remainders of the -tdf option
qemu-options: Bail out on unsupported options instead of silently
ignoring them
qemu-options: Remove deprecated -no-kvm-pit-reinjection
qemu-options: Remove deprecated -no-kvm-irqchip
qemu-options: Remove deprecated -no-kvm
include/qemu-common.h | 2 +-
os-posix.c | 6 +++++-
os-win32.c | 4 ++--
qemu-doc.texi | 15 ---------------
qemu-options.hx | 13 -------------
vl.c | 26 ++++----------------------
6 files changed, 12 insertions(+), 54 deletions(-)
Queued patches 1-4 waiting for more discussion, thanks.

Paolo
Loading...