[Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Thomas Huth]

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.

Thomas Huth (5):
  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(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/5] qemu-options: Remove remainders of the -tdf option

[Thomas Huth]

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 <armbru@redhat.com>
Signed-off-by: Thomas Huth <thuth@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

[Qemu-devel] [PATCH v2 2/5] qemu-options: Bail out on unsupported options instead of silently ignoring them

[Thomas Huth]

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 <armbru@redhat.com>
Signed-off-by: Thomas Huth <thuth@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

Re: [Qemu-devel] [PATCH v2 2/5] qemu-options: Bail out on unsupported options instead of silently ignoring them

[Anthony PERARD]

On Fri, May 04, 2018 at 07:01:07PM +0200, Thomas Huth wrote:
> 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);
> +                }

I think that report false errors, I've got:
qemu-system-i386: -no-user-config: Option not supported in this build

Regards,

-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH v2 2/5] qemu-options: Bail out on unsupported options instead of silently ignoring them

[Thomas Huth]

On 16.05.2018 18:49, Anthony PERARD wrote:
> On Fri, May 04, 2018 at 07:01:07PM +0200, Thomas Huth wrote:
>> 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);
>> +                }
> 
> I think that report false errors, I've got:
> 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

[Qemu-devel] [PATCH v2 3/5] qemu-options: Remove deprecated -no-kvm-pit-reinjection

[Thomas Huth]

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 <armbru@redhat.com>
Signed-off-by: Thomas Huth <thuth@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

[Qemu-devel] [PATCH v2 4/5] qemu-options: Remove deprecated -no-kvm-irqchip

[Thomas Huth]

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 <armbru@redhat.com>
Signed-off-by: Thomas Huth <thuth@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

[Qemu-devel] [PATCH v2 5/5] qemu-options: Remove deprecated -no-kvm

[Thomas Huth]

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 <armbru@redhat.com>
Signed-off-by: Thomas Huth <thuth@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

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Paolo Bonzini]

On 04/05/2018 19:01, Thomas Huth wrote:
> 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.
> 
> Thomas Huth (5):
>   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

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Thomas Huth]

On 04.05.2018 23:57, Paolo Bonzini wrote:
> On 04/05/2018 19:01, Thomas Huth wrote:
>> 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.
>>
>> Thomas Huth (5):
>>   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

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 04/05/2018 19:01, Thomas Huth wrote:
>> 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.
>> 
>> Thomas Huth (5):
>>   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 <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 <mtosatti@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");

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Thomas Huth]

On 07.05.2018 07:37, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 04/05/2018 19:01, Thomas Huth wrote:
>>> 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.
>>>
>>> Thomas Huth (5):
>>>   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

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 07.05.2018 07:37, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> On 04/05/2018 19:01, Thomas Huth wrote:
>>>> 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.
>>>>
>>>> Thomas Huth (5):
>>>>   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.

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Paolo Bonzini]

On 07/05/2018 09:56, Markus Armbruster wrote:
>>>> 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

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 07/05/2018 09:56, Markus Armbruster wrote:
>>>>> 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 <fred.konrad@greensocs.com>
    [AJB: move to -accel tcg,thread=multi|single, defaults]
    Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
    Reviewed-by: Richard Henderson <rth@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.

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Paolo Bonzini]

On 07/05/2018 13:56, Markus Armbruster wrote:
> 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 <fred.konrad@greensocs.com>
>     [AJB: move to -accel tcg,thread=multi|single, defaults]
>     Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>     Reviewed-by: Richard Henderson <rth@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.

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

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 07/05/2018 13:56, Markus Armbruster wrote:
>> 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 <fred.konrad@greensocs.com>
>>     [AJB: move to -accel tcg,thread=multi|single, defaults]
>>     Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>     Reviewed-by: Richard Henderson <rth@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.
>
> 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.

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Paolo Bonzini]

On 07/05/2018 18:50, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> On 07/05/2018 13:56, Markus Armbruster wrote:
>>> 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 <fred.konrad@greensocs.com>
>>>     [AJB: move to -accel tcg,thread=multi|single, defaults]
>>>     Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>     Reviewed-by: Richard Henderson <rth@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.
>>
>> 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

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Thomas Huth]

On 07.05.2018 19:01, Paolo Bonzini wrote:
> On 07/05/2018 18:50, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> On 07/05/2018 13:56, Markus Armbruster wrote:
>>>> 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 <fred.konrad@greensocs.com>
>>>>     [AJB: move to -accel tcg,thread=multi|single, defaults]
>>>>     Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>     Reviewed-by: Richard Henderson <rth@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.
>>>
>>> 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

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Paolo Bonzini]

On 07/05/2018 07:37, Markus Armbruster wrote:
>> 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

> 
> Author: Jan Kiszka <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 <mtosatti@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 07/05/2018 07:37, Markus Armbruster wrote:
>>> 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".

> "-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.

Re: [Qemu-devel] [PATCH v2 0/5] Removal of deprecated -no-kvm* options

[Paolo Bonzini]

On 04/05/2018 19:01, Thomas Huth wrote:
> 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.
> 
> Thomas Huth (5):
>   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