All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 1/2] Generalize -machine command line option
@ 2011-05-20 11:18 Jan Kiszka
  2011-05-22 11:00 ` [Qemu-devel] [PATCH v2 " Jan Kiszka
  0 siblings, 1 reply; 12+ messages in thread
From: Jan Kiszka @ 2011-05-20 11:18 UTC (permalink / raw)
  To: Anthony Liguori, qemu-devel; +Cc: Anthony PERARD

-machine somehow suggests that it selects the machine, but it doesn't.
Fix that before this command is set in stone.

Actually, -machine should supersede -M and allow to introduce arbitrary
per-machine options to the command line. That will change the internal
realization again, but we will be able to keep the user interface
stable.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 qemu-config.c   |    5 +++++
 qemu-options.hx |   20 +++++++++++++++-----
 vl.c            |   34 +++++++++++++++++++---------------
 3 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 5d7ffa2..01751b4 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -452,9 +452,14 @@ QemuOptsList qemu_option_rom_opts = {
 
 static QemuOptsList qemu_machine_opts = {
     .name = "machine",
+    .implied_opt_name = "type",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
     .desc = {
         {
+            .name = "type",
+            .type = QEMU_OPT_STRING,
+            .help = "emulated machine"
+        }, {
             .name = "accel",
             .type = QEMU_OPT_STRING,
             .help = "accelerator list",
diff --git a/qemu-options.hx b/qemu-options.hx
index 82e085a..0dbc028 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2033,13 +2033,23 @@ if KVM support is enabled when compiling.
 ETEXI
 
 DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
-    "-machine accel=accel1[:accel2]    use an accelerator (kvm,xen,tcg), default is tcg\n", QEMU_ARCH_ALL)
+    "-machine [type=]name[,prop[=value][,...]]\n"
+    "                selects emulated machine (-machine ? for list)\n"
+    "                property accel=accel1[:accel2[:...]] selects accelerator\n"
+    "                supported accelerators are kvm, xen, tcg (default: tcg)\n",
+    QEMU_ARCH_ALL)
 STEXI
-@item -machine accel=@var{accels}
+@item -machine [type=]@var{name}[,prop=@var{value}[,...]]
 @findex -machine
-This is use to enable an accelerator, in kvm,xen,tcg.
-By default, it use only tcg. If there a more than one accelerator
-specified, the next one is used if the first don't work.
+Select the emulated machine by @var{name}. Use @code{-machine ?} to list
+available machines. Supported machine properties are:
+@table @option
+@item accel=@var{accels1}[:@var{accels2}[:...]]
+This is used to enable an accelerator. Depending on the target architecture,
+kvm, xen, or tcg can be available. By default, tcg is used. If there is more
+than one accelerator specified, the next one is used if the previous one fails
+to initialize.
+@end table
 ETEXI
 
 DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid,
diff --git a/vl.c b/vl.c
index b362871..4560376 100644
--- a/vl.c
+++ b/vl.c
@@ -2144,20 +2144,9 @@ int main(int argc, char **argv, char **envp)
             }
             switch(popt->index) {
             case QEMU_OPTION_M:
-                machine = find_machine(optarg);
-                if (!machine) {
-                    QEMUMachine *m;
-                    printf("Supported machines are:\n");
-                    for(m = first_machine; m != NULL; m = m->next) {
-                        if (m->alias)
-                            printf("%-10s %s (alias of %s)\n",
-                                   m->alias, m->desc, m->name);
-                        printf("%-10s %s%s\n",
-                               m->name, m->desc,
-                               m->is_default ? " (default)" : "");
-                    }
-                    exit(*optarg != '?');
-                }
+                olist = qemu_find_opts("machine");
+                qemu_opts_reset(olist);
+                qemu_opts_parse(olist, optarg, 1);
                 break;
             case QEMU_OPTION_cpu:
                 /* hw initialization will check this */
@@ -2675,11 +2664,26 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_machine:
                 olist = qemu_find_opts("machine");
                 qemu_opts_reset(olist);
-                opts = qemu_opts_parse(olist, optarg, 0);
+                opts = qemu_opts_parse(olist, optarg, 1);
                 if (!opts) {
                     fprintf(stderr, "parse error: %s\n", optarg);
                     exit(1);
                 }
+                optarg = qemu_opt_get(opts, "type");
+                machine = optarg ? find_machine(optarg) : NULL;
+                if (!machine) {
+                    QEMUMachine *m;
+                    printf("Supported machines are:\n");
+                    for (m = first_machine; m != NULL; m = m->next) {
+                        if (m->alias) {
+                            printf("%-10s %s (alias of %s)\n",
+                                   m->alias, m->desc, m->name);
+                        }
+                        printf("%-10s %s%s\n", m->name, m->desc,
+                               m->is_default ? " (default)" : "");
+                    }
+                    exit(!optarg || *optarg != '?');
+                }
                 break;
             case QEMU_OPTION_usb:
                 usb_enabled = 1;

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2 1/2] Generalize -machine command line option
  2011-05-20 11:18 [Qemu-devel] [PATCH 1/2] Generalize -machine command line option Jan Kiszka
@ 2011-05-22 11:00 ` Jan Kiszka
  2011-05-24 16:06   ` Ian Campbell
  0 siblings, 1 reply; 12+ messages in thread
From: Jan Kiszka @ 2011-05-22 11:00 UTC (permalink / raw)
  To: Anthony Liguori, qemu-devel; +Cc: Anthony PERARD

From: Jan Kiszka <jan.kiszka@siemens.com>

-machine somehow suggests that it selects the machine, but it doesn't.
Fix that before this command is set in stone.

Actually, -machine should supersede -M and allow to introduce arbitrary
per-machine options to the command line. That will change the internal
realization again, but we will be able to keep the user interface
stable.

CC: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - fix regression of -M my factoring out machine_parse and using it for
   both old and new command.

 qemu-config.c   |    5 +++++
 qemu-options.hx |   20 +++++++++++++++-----
 vl.c            |   39 ++++++++++++++++++++++++---------------
 3 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 5d7ffa2..01751b4 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -452,9 +452,14 @@ QemuOptsList qemu_option_rom_opts = {
 
 static QemuOptsList qemu_machine_opts = {
     .name = "machine",
+    .implied_opt_name = "type",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
     .desc = {
         {
+            .name = "type",
+            .type = QEMU_OPT_STRING,
+            .help = "emulated machine"
+        }, {
             .name = "accel",
             .type = QEMU_OPT_STRING,
             .help = "accelerator list",
diff --git a/qemu-options.hx b/qemu-options.hx
index 82e085a..0dbc028 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2033,13 +2033,23 @@ if KVM support is enabled when compiling.
 ETEXI
 
 DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
-    "-machine accel=accel1[:accel2]    use an accelerator (kvm,xen,tcg), default is tcg\n", QEMU_ARCH_ALL)
+    "-machine [type=]name[,prop[=value][,...]]\n"
+    "                selects emulated machine (-machine ? for list)\n"
+    "                property accel=accel1[:accel2[:...]] selects accelerator\n"
+    "                supported accelerators are kvm, xen, tcg (default: tcg)\n",
+    QEMU_ARCH_ALL)
 STEXI
-@item -machine accel=@var{accels}
+@item -machine [type=]@var{name}[,prop=@var{value}[,...]]
 @findex -machine
-This is use to enable an accelerator, in kvm,xen,tcg.
-By default, it use only tcg. If there a more than one accelerator
-specified, the next one is used if the first don't work.
+Select the emulated machine by @var{name}. Use @code{-machine ?} to list
+available machines. Supported machine properties are:
+@table @option
+@item accel=@var{accels1}[:@var{accels2}[:...]]
+This is used to enable an accelerator. Depending on the target architecture,
+kvm, xen, or tcg can be available. By default, tcg is used. If there is more
+than one accelerator specified, the next one is used if the previous one fails
+to initialize.
+@end table
 ETEXI
 
 DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid,
diff --git a/vl.c b/vl.c
index b362871..a346c1e 100644
--- a/vl.c
+++ b/vl.c
@@ -1891,6 +1891,27 @@ static int debugcon_parse(const char *devname)
     return 0;
 }
 
+static QEMUMachine *machine_parse(const char *name)
+{
+    QEMUMachine *m, *machine = NULL;
+
+    if (name) {
+        machine = find_machine(name);
+    }
+    if (machine) {
+        return machine;
+    }
+    printf("Supported machines are:\n");
+    for (m = first_machine; m != NULL; m = m->next) {
+        if (m->alias) {
+            printf("%-10s %s (alias of %s)\n", m->alias, m->desc, m->name);
+        }
+        printf("%-10s %s%s\n", m->name, m->desc,
+               m->is_default ? " (default)" : "");
+    }
+    exit(!name || *name != '?');
+}
+
 static int tcg_init(void)
 {
     return 0;
@@ -2144,20 +2165,7 @@ int main(int argc, char **argv, char **envp)
             }
             switch(popt->index) {
             case QEMU_OPTION_M:
-                machine = find_machine(optarg);
-                if (!machine) {
-                    QEMUMachine *m;
-                    printf("Supported machines are:\n");
-                    for(m = first_machine; m != NULL; m = m->next) {
-                        if (m->alias)
-                            printf("%-10s %s (alias of %s)\n",
-                                   m->alias, m->desc, m->name);
-                        printf("%-10s %s%s\n",
-                               m->name, m->desc,
-                               m->is_default ? " (default)" : "");
-                    }
-                    exit(*optarg != '?');
-                }
+                machine = machine_parse(optarg);
                 break;
             case QEMU_OPTION_cpu:
                 /* hw initialization will check this */
@@ -2675,11 +2683,12 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_machine:
                 olist = qemu_find_opts("machine");
                 qemu_opts_reset(olist);
-                opts = qemu_opts_parse(olist, optarg, 0);
+                opts = qemu_opts_parse(olist, optarg, 1);
                 if (!opts) {
                     fprintf(stderr, "parse error: %s\n", optarg);
                     exit(1);
                 }
+                machine = machine_parse(qemu_opt_get(opts, "type"));
                 break;
             case QEMU_OPTION_usb:
                 usb_enabled = 1;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] Generalize -machine command line option
  2011-05-22 11:00 ` [Qemu-devel] [PATCH v2 " Jan Kiszka
@ 2011-05-24 16:06   ` Ian Campbell
  2011-05-24 16:18     ` Jan Kiszka
  0 siblings, 1 reply; 12+ messages in thread
From: Ian Campbell @ 2011-05-24 16:06 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Anthony PERARD, Anthony Liguori, qemu-devel

On Sun, 2011-05-22 at 13:00 +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> -machine somehow suggests that it selects the machine, but it doesn't.
> Fix that before this command is set in stone.
> 
> Actually, -machine should supersede -M and allow to introduce arbitrary
> per-machine options to the command line. That will change the internal
> realization again, but we will be able to keep the user interface
> stable.
> 
> CC: Anthony PERARD <anthony.perard@citrix.com>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

"-machine xenfv" doesn't work for me with this patch, it gives:
        Program received signal SIGSEGV, Segmentation fault.
        hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
        36	xc_hcall_buf.c: No such file or directory.
        	in xc_hcall_buf.c
        (gdb) bt
        #0  hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
        #1  0xb7d53f1d in hypercall_buffer_cache_alloc (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:52
        #2  xc__hypercall_buffer_alloc_pages (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:128
        #3  0xb7d54028 in xc__hypercall_buffer_alloc (xch=0x0, b=0xbffff77c, size=16) at xc_hcall_buf.c:162
        #4  0xb7d42719 in xc_get_hvm_param (handle=0x0, dom=248, param=5, value=0xbffff810) at xc_domain.c:1078
        #5  0x08252777 in xen_hvm_init () at /home/ianc/devel/qemu.git/xen-all.c:803
        #6  0x082921e9 in pc_xen_hvm_init (ram_size=536870912, boot_device=0xbffffabb "cda", kernel_filename=0x0, kernel_cmdline=0x82d648f "", initrd_filename=0x0, cpu_model=0x0) at /home/ianc/devel/qemu.git/hw/pc_piix.c:246
        #7  0x081e3f0e in main (argc=26, argv=0xbffffba4, envp=0xbffffc10) at /home/ianc/devel/qemu.git/vl.c:3162

I suspect this is because xen_init() (which sets xch) is never called,
perhaps because it causes the accelerator to always be tcg? If I use
"-machine xenfv,accel=xen" then it works as expected, -M continues to
work too AFAICT.

It would be nice to retain the behaviour of defaulting to accel=xen for
machine=xenfv. (to be honest I can't quite see where this behaviour came
from, nor what about this patch changes it...)

> ---
> 
> Changes in v2:
>  - fix regression of -M my factoring out machine_parse and using it for
>    both old and new command.
> 
>  qemu-config.c   |    5 +++++
>  qemu-options.hx |   20 +++++++++++++++-----
>  vl.c            |   39 ++++++++++++++++++++++++---------------
>  3 files changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/qemu-config.c b/qemu-config.c
> index 5d7ffa2..01751b4 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -452,9 +452,14 @@ QemuOptsList qemu_option_rom_opts = {
>  
>  static QemuOptsList qemu_machine_opts = {
>      .name = "machine",
> +    .implied_opt_name = "type",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
>      .desc = {
>          {
> +            .name = "type",
> +            .type = QEMU_OPT_STRING,
> +            .help = "emulated machine"
> +        }, {
>              .name = "accel",
>              .type = QEMU_OPT_STRING,
>              .help = "accelerator list",
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 82e085a..0dbc028 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2033,13 +2033,23 @@ if KVM support is enabled when compiling.
>  ETEXI
>  
>  DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> -    "-machine accel=accel1[:accel2]    use an accelerator (kvm,xen,tcg), default is tcg\n", QEMU_ARCH_ALL)
> +    "-machine [type=]name[,prop[=value][,...]]\n"
> +    "                selects emulated machine (-machine ? for list)\n"
> +    "                property accel=accel1[:accel2[:...]] selects accelerator\n"
> +    "                supported accelerators are kvm, xen, tcg (default: tcg)\n",
> +    QEMU_ARCH_ALL)
>  STEXI
> -@item -machine accel=@var{accels}
> +@item -machine [type=]@var{name}[,prop=@var{value}[,...]]
>  @findex -machine
> -This is use to enable an accelerator, in kvm,xen,tcg.
> -By default, it use only tcg. If there a more than one accelerator
> -specified, the next one is used if the first don't work.
> +Select the emulated machine by @var{name}. Use @code{-machine ?} to list
> +available machines. Supported machine properties are:
> +@table @option
> +@item accel=@var{accels1}[:@var{accels2}[:...]]
> +This is used to enable an accelerator. Depending on the target architecture,
> +kvm, xen, or tcg can be available. By default, tcg is used. If there is more
> +than one accelerator specified, the next one is used if the previous one fails
> +to initialize.
> +@end table
>  ETEXI
>  
>  DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid,
> diff --git a/vl.c b/vl.c
> index b362871..a346c1e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1891,6 +1891,27 @@ static int debugcon_parse(const char *devname)
>      return 0;
>  }
>  
> +static QEMUMachine *machine_parse(const char *name)
> +{
> +    QEMUMachine *m, *machine = NULL;
> +
> +    if (name) {
> +        machine = find_machine(name);
> +    }
> +    if (machine) {
> +        return machine;
> +    }
> +    printf("Supported machines are:\n");
> +    for (m = first_machine; m != NULL; m = m->next) {
> +        if (m->alias) {
> +            printf("%-10s %s (alias of %s)\n", m->alias, m->desc, m->name);
> +        }
> +        printf("%-10s %s%s\n", m->name, m->desc,
> +               m->is_default ? " (default)" : "");
> +    }
> +    exit(!name || *name != '?');
> +}
> +
>  static int tcg_init(void)
>  {
>      return 0;
> @@ -2144,20 +2165,7 @@ int main(int argc, char **argv, char **envp)
>              }
>              switch(popt->index) {
>              case QEMU_OPTION_M:
> -                machine = find_machine(optarg);
> -                if (!machine) {
> -                    QEMUMachine *m;
> -                    printf("Supported machines are:\n");
> -                    for(m = first_machine; m != NULL; m = m->next) {
> -                        if (m->alias)
> -                            printf("%-10s %s (alias of %s)\n",
> -                                   m->alias, m->desc, m->name);
> -                        printf("%-10s %s%s\n",
> -                               m->name, m->desc,
> -                               m->is_default ? " (default)" : "");
> -                    }
> -                    exit(*optarg != '?');
> -                }
> +                machine = machine_parse(optarg);
>                  break;
>              case QEMU_OPTION_cpu:
>                  /* hw initialization will check this */
> @@ -2675,11 +2683,12 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_machine:
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_reset(olist);
> -                opts = qemu_opts_parse(olist, optarg, 0);
> +                opts = qemu_opts_parse(olist, optarg, 1);
>                  if (!opts) {
>                      fprintf(stderr, "parse error: %s\n", optarg);
>                      exit(1);
>                  }
> +                machine = machine_parse(qemu_opt_get(opts, "type"));
>                  break;
>              case QEMU_OPTION_usb:
>                  usb_enabled = 1;

-- 
Ian Campbell

"When people are least sure, they are often most dogmatic."
		-- John Kenneth Galbraith

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] Generalize -machine command line option
  2011-05-24 16:06   ` Ian Campbell
@ 2011-05-24 16:18     ` Jan Kiszka
  2011-05-25  7:03       ` [Qemu-devel] [PATCH v3 " Jan Kiszka
  2011-05-25  8:13       ` [Qemu-devel] [PATCH v2 " Ian Campbell
  0 siblings, 2 replies; 12+ messages in thread
From: Jan Kiszka @ 2011-05-24 16:18 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Anthony PERARD, Anthony Liguori, qemu-devel

On 2011-05-24 18:06, Ian Campbell wrote:
> On Sun, 2011-05-22 at 13:00 +0200, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> -machine somehow suggests that it selects the machine, but it doesn't.
>> Fix that before this command is set in stone.
>>
>> Actually, -machine should supersede -M and allow to introduce arbitrary
>> per-machine options to the command line. That will change the internal
>> realization again, but we will be able to keep the user interface
>> stable.
>>
>> CC: Anthony PERARD <anthony.perard@citrix.com>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> "-machine xenfv" doesn't work for me with this patch, it gives:
>         Program received signal SIGSEGV, Segmentation fault.
>         hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
>         36	xc_hcall_buf.c: No such file or directory.
>         	in xc_hcall_buf.c
>         (gdb) bt
>         #0  hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
>         #1  0xb7d53f1d in hypercall_buffer_cache_alloc (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:52
>         #2  xc__hypercall_buffer_alloc_pages (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:128
>         #3  0xb7d54028 in xc__hypercall_buffer_alloc (xch=0x0, b=0xbffff77c, size=16) at xc_hcall_buf.c:162
>         #4  0xb7d42719 in xc_get_hvm_param (handle=0x0, dom=248, param=5, value=0xbffff810) at xc_domain.c:1078
>         #5  0x08252777 in xen_hvm_init () at /home/ianc/devel/qemu.git/xen-all.c:803
>         #6  0x082921e9 in pc_xen_hvm_init (ram_size=536870912, boot_device=0xbffffabb "cda", kernel_filename=0x0, kernel_cmdline=0x82d648f "", initrd_filename=0x0, cpu_model=0x0) at /home/ianc/devel/qemu.git/hw/pc_piix.c:246
>         #7  0x081e3f0e in main (argc=26, argv=0xbffffba4, envp=0xbffffc10) at /home/ianc/devel/qemu.git/vl.c:3162
> 
> I suspect this is because xen_init() (which sets xch) is never called,
> perhaps because it causes the accelerator to always be tcg? If I use
> "-machine xenfv,accel=xen" then it works as expected, -M continues to
> work too AFAICT.
> 
> It would be nice to retain the behaviour of defaulting to accel=xen for
> machine=xenfv. (to be honest I can't quite see where this behaviour came
> from, nor what about this patch changes it...)

Well, first of all I think this revealed a Xen bug because it crashes
when you try to run xenfv with an inappropriate accelerator, no? What is
the result of -machine xenfv,accel=tcg or, without my patch, -M xenfv
-machine accel=tcg?

Then the question is what accel options are actually picked with
-machine xenfv, or why the default options are maybe not considered. I
don't have any Xen around, but I will check how far I can debug this -
or actually try to understand what I coded if nothing helps.

Thanks for testing!
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v3 1/2] Generalize -machine command line option
  2011-05-24 16:18     ` Jan Kiszka
@ 2011-05-25  7:03       ` Jan Kiszka
  2011-05-25  8:17         ` Ian Campbell
  2011-06-07 15:58         ` Jan Kiszka
  2011-05-25  8:13       ` [Qemu-devel] [PATCH v2 " Ian Campbell
  1 sibling, 2 replies; 12+ messages in thread
From: Jan Kiszka @ 2011-05-25  7:03 UTC (permalink / raw)
  To: Anthony Liguori, qemu-devel; +Cc: Anthony PERARD, Ian Campbell

From: Jan Kiszka <jan.kiszka@siemens.com>

-machine somehow suggests that it selects the machine, but it doesn't.
Fix that before this command is set in stone.

Actually, -machine should supersede -M and allow to introduce arbitrary
per-machine options to the command line. That will change the internal
realization again, but we will be able to keep the user interface
stable.

CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v3: 
 - fix regression of default machine options handling, -machine xenfv
   selects accel=xen again
   (I really hope we can clean up the defaults, make them more
   generally useful when switching to some QCFG.)

Changes in v2:
 - fix regression of -M my factoring out machine_parse and using it for
   both old and new command.

 qemu-config.c   |    5 +++++
 qemu-options.hx |   20 +++++++++++++++-----
 vl.c            |   43 ++++++++++++++++++++++++++-----------------
 3 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/qemu-config.c b/qemu-config.c
index 5d7ffa2..01751b4 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -452,9 +452,14 @@ QemuOptsList qemu_option_rom_opts = {
 
 static QemuOptsList qemu_machine_opts = {
     .name = "machine",
+    .implied_opt_name = "type",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
     .desc = {
         {
+            .name = "type",
+            .type = QEMU_OPT_STRING,
+            .help = "emulated machine"
+        }, {
             .name = "accel",
             .type = QEMU_OPT_STRING,
             .help = "accelerator list",
diff --git a/qemu-options.hx b/qemu-options.hx
index 82e085a..0dbc028 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2033,13 +2033,23 @@ if KVM support is enabled when compiling.
 ETEXI
 
 DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
-    "-machine accel=accel1[:accel2]    use an accelerator (kvm,xen,tcg), default is tcg\n", QEMU_ARCH_ALL)
+    "-machine [type=]name[,prop[=value][,...]]\n"
+    "                selects emulated machine (-machine ? for list)\n"
+    "                property accel=accel1[:accel2[:...]] selects accelerator\n"
+    "                supported accelerators are kvm, xen, tcg (default: tcg)\n",
+    QEMU_ARCH_ALL)
 STEXI
-@item -machine accel=@var{accels}
+@item -machine [type=]@var{name}[,prop=@var{value}[,...]]
 @findex -machine
-This is use to enable an accelerator, in kvm,xen,tcg.
-By default, it use only tcg. If there a more than one accelerator
-specified, the next one is used if the first don't work.
+Select the emulated machine by @var{name}. Use @code{-machine ?} to list
+available machines. Supported machine properties are:
+@table @option
+@item accel=@var{accels1}[:@var{accels2}[:...]]
+This is used to enable an accelerator. Depending on the target architecture,
+kvm, xen, or tcg can be available. By default, tcg is used. If there is more
+than one accelerator specified, the next one is used if the previous one fails
+to initialize.
+@end table
 ETEXI
 
 DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid,
diff --git a/vl.c b/vl.c
index b362871..1865a8d 100644
--- a/vl.c
+++ b/vl.c
@@ -1891,6 +1891,27 @@ static int debugcon_parse(const char *devname)
     return 0;
 }
 
+static QEMUMachine *machine_parse(const char *name)
+{
+    QEMUMachine *m, *machine = NULL;
+
+    if (name) {
+        machine = find_machine(name);
+    }
+    if (machine) {
+        return machine;
+    }
+    printf("Supported machines are:\n");
+    for (m = first_machine; m != NULL; m = m->next) {
+        if (m->alias) {
+            printf("%-10s %s (alias of %s)\n", m->alias, m->desc, m->name);
+        }
+        printf("%-10s %s%s\n", m->name, m->desc,
+               m->is_default ? " (default)" : "");
+    }
+    exit(!name || *name != '?');
+}
+
 static int tcg_init(void)
 {
     return 0;
@@ -2144,20 +2165,7 @@ int main(int argc, char **argv, char **envp)
             }
             switch(popt->index) {
             case QEMU_OPTION_M:
-                machine = find_machine(optarg);
-                if (!machine) {
-                    QEMUMachine *m;
-                    printf("Supported machines are:\n");
-                    for(m = first_machine; m != NULL; m = m->next) {
-                        if (m->alias)
-                            printf("%-10s %s (alias of %s)\n",
-                                   m->alias, m->desc, m->name);
-                        printf("%-10s %s%s\n",
-                               m->name, m->desc,
-                               m->is_default ? " (default)" : "");
-                    }
-                    exit(*optarg != '?');
-                }
+                machine = machine_parse(optarg);
                 break;
             case QEMU_OPTION_cpu:
                 /* hw initialization will check this */
@@ -2675,11 +2683,12 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_machine:
                 olist = qemu_find_opts("machine");
                 qemu_opts_reset(olist);
-                opts = qemu_opts_parse(olist, optarg, 0);
+                opts = qemu_opts_parse(olist, optarg, 1);
                 if (!opts) {
                     fprintf(stderr, "parse error: %s\n", optarg);
                     exit(1);
                 }
+                machine = machine_parse(qemu_opt_get(opts, "type"));
                 break;
             case QEMU_OPTION_usb:
                 usb_enabled = 1;
@@ -2941,8 +2950,8 @@ int main(int argc, char **argv, char **envp)
             p = qemu_opt_get(QTAILQ_FIRST(&list->head), "accel");
         }
         if (p == NULL) {
-            opts = qemu_opts_parse(qemu_find_opts("machine"),
-                                   machine->default_machine_opts, 0);
+            qemu_opts_reset(list);
+            opts = qemu_opts_parse(list, machine->default_machine_opts, 0);
             if (!opts) {
                 fprintf(stderr, "parse error for machine %s: %s\n",
                         machine->name, machine->default_machine_opts);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] Generalize -machine command line option
  2011-05-24 16:18     ` Jan Kiszka
  2011-05-25  7:03       ` [Qemu-devel] [PATCH v3 " Jan Kiszka
@ 2011-05-25  8:13       ` Ian Campbell
  2011-05-25  8:23         ` Jan Kiszka
  2011-05-25 10:54         ` Anthony PERARD
  1 sibling, 2 replies; 12+ messages in thread
From: Ian Campbell @ 2011-05-25  8:13 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Anthony PERARD, Anthony Liguori, qemu-devel

On Tue, 2011-05-24 at 18:18 +0200, Jan Kiszka wrote:
> On 2011-05-24 18:06, Ian Campbell wrote:
> > On Sun, 2011-05-22 at 13:00 +0200, Jan Kiszka wrote:
> >> From: Jan Kiszka <jan.kiszka@siemens.com>
> >>
> >> -machine somehow suggests that it selects the machine, but it doesn't.
> >> Fix that before this command is set in stone.
> >>
> >> Actually, -machine should supersede -M and allow to introduce arbitrary
> >> per-machine options to the command line. That will change the internal
> >> realization again, but we will be able to keep the user interface
> >> stable.
> >>
> >> CC: Anthony PERARD <anthony.perard@citrix.com>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > 
> > "-machine xenfv" doesn't work for me with this patch, it gives:
> >         Program received signal SIGSEGV, Segmentation fault.
> >         hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
> >         36	xc_hcall_buf.c: No such file or directory.
> >         	in xc_hcall_buf.c
> >         (gdb) bt
> >         #0  hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
> >         #1  0xb7d53f1d in hypercall_buffer_cache_alloc (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:52
> >         #2  xc__hypercall_buffer_alloc_pages (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:128
> >         #3  0xb7d54028 in xc__hypercall_buffer_alloc (xch=0x0, b=0xbffff77c, size=16) at xc_hcall_buf.c:162
> >         #4  0xb7d42719 in xc_get_hvm_param (handle=0x0, dom=248, param=5, value=0xbffff810) at xc_domain.c:1078
> >         #5  0x08252777 in xen_hvm_init () at /home/ianc/devel/qemu.git/xen-all.c:803
> >         #6  0x082921e9 in pc_xen_hvm_init (ram_size=536870912, boot_device=0xbffffabb "cda", kernel_filename=0x0, kernel_cmdline=0x82d648f "", initrd_filename=0x0, cpu_model=0x0) at /home/ianc/devel/qemu.git/hw/pc_piix.c:246
> >         #7  0x081e3f0e in main (argc=26, argv=0xbffffba4, envp=0xbffffc10) at /home/ianc/devel/qemu.git/vl.c:3162
> > 
> > I suspect this is because xen_init() (which sets xch) is never called,
> > perhaps because it causes the accelerator to always be tcg? If I use
> > "-machine xenfv,accel=xen" then it works as expected, -M continues to
> > work too AFAICT.
> > 
> > It would be nice to retain the behaviour of defaulting to accel=xen for
> > machine=xenfv. (to be honest I can't quite see where this behaviour came
> > from, nor what about this patch changes it...)
> 
> Well, first of all I think this revealed a Xen bug because it crashes
> when you try to run xenfv with an inappropriate accelerator, no? What is
> the result of -machine xenfv,accel=tcg or, without my patch, -M xenfv
> -machine accel=tcg?

Unsurprisingly it crashed...

I'm not sure if this is a bug in the xen side of simply a case of
providing enough rope. I didn't really follow the threads which resulted
in the accel stuff all that closely so I'm not sure what the intention
was, it seems to me that the accel option is invalid with certain
machine types though.

I suppose in theory -machine xenpv,accel=kvm might result in xenner or
something, accel=xenpv,tcg I'm less sure about (perhaps xenner too?).
For -machine xenfv I don't expect anything other than accel=xen makes
much sense.

> Then the question is what accel options are actually picked with
> -machine xenfv, or why the default options are maybe not considered. I
> don't have any Xen around, but I will check how far I can debug this -
> or actually try to understand what I coded if nothing helps.

I saw a follow up patch -- I'll give it a go.

> 
> Thanks for testing!
> Jan
> 

-- 
Ian Campbell
Current Noise: Blind Melon - Soak The Sin

Anti-trust laws should be approached with exactly that attitude.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] Generalize -machine command line option
  2011-05-25  7:03       ` [Qemu-devel] [PATCH v3 " Jan Kiszka
@ 2011-05-25  8:17         ` Ian Campbell
  2011-06-07 15:58         ` Jan Kiszka
  1 sibling, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2011-05-25  8:17 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Anthony PERARD, Anthony Liguori, qemu-devel

On Wed, 2011-05-25 at 09:03 +0200, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> -machine somehow suggests that it selects the machine, but it doesn't.
> Fix that before this command is set in stone.
> 
> Actually, -machine should supersede -M and allow to introduce arbitrary
> per-machine options to the command line. That will change the internal
> realization again, but we will be able to keep the user interface
> stable.
> 
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Ian Campbell <ijc@hellion.org.uk>

Thanks Jan, this one works for me.

Tested-by: Ian Campbell <ian.campbell@citrix.com>

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Changes in v3: 
>  - fix regression of default machine options handling, -machine xenfv
>    selects accel=xen again
>    (I really hope we can clean up the defaults, make them more
>    generally useful when switching to some QCFG.)
> 
> Changes in v2:
>  - fix regression of -M my factoring out machine_parse and using it for
>    both old and new command.
> 
>  qemu-config.c   |    5 +++++
>  qemu-options.hx |   20 +++++++++++++++-----
>  vl.c            |   43 ++++++++++++++++++++++++++-----------------
>  3 files changed, 46 insertions(+), 22 deletions(-)
> 
> diff --git a/qemu-config.c b/qemu-config.c
> index 5d7ffa2..01751b4 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -452,9 +452,14 @@ QemuOptsList qemu_option_rom_opts = {
>  
>  static QemuOptsList qemu_machine_opts = {
>      .name = "machine",
> +    .implied_opt_name = "type",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
>      .desc = {
>          {
> +            .name = "type",
> +            .type = QEMU_OPT_STRING,
> +            .help = "emulated machine"
> +        }, {
>              .name = "accel",
>              .type = QEMU_OPT_STRING,
>              .help = "accelerator list",
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 82e085a..0dbc028 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2033,13 +2033,23 @@ if KVM support is enabled when compiling.
>  ETEXI
>  
>  DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> -    "-machine accel=accel1[:accel2]    use an accelerator (kvm,xen,tcg), default is tcg\n", QEMU_ARCH_ALL)
> +    "-machine [type=]name[,prop[=value][,...]]\n"
> +    "                selects emulated machine (-machine ? for list)\n"
> +    "                property accel=accel1[:accel2[:...]] selects accelerator\n"
> +    "                supported accelerators are kvm, xen, tcg (default: tcg)\n",
> +    QEMU_ARCH_ALL)
>  STEXI
> -@item -machine accel=@var{accels}
> +@item -machine [type=]@var{name}[,prop=@var{value}[,...]]
>  @findex -machine
> -This is use to enable an accelerator, in kvm,xen,tcg.
> -By default, it use only tcg. If there a more than one accelerator
> -specified, the next one is used if the first don't work.
> +Select the emulated machine by @var{name}. Use @code{-machine ?} to list
> +available machines. Supported machine properties are:
> +@table @option
> +@item accel=@var{accels1}[:@var{accels2}[:...]]
> +This is used to enable an accelerator. Depending on the target architecture,
> +kvm, xen, or tcg can be available. By default, tcg is used. If there is more
> +than one accelerator specified, the next one is used if the previous one fails
> +to initialize.
> +@end table
>  ETEXI
>  
>  DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid,
> diff --git a/vl.c b/vl.c
> index b362871..1865a8d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1891,6 +1891,27 @@ static int debugcon_parse(const char *devname)
>      return 0;
>  }
>  
> +static QEMUMachine *machine_parse(const char *name)
> +{
> +    QEMUMachine *m, *machine = NULL;
> +
> +    if (name) {
> +        machine = find_machine(name);
> +    }
> +    if (machine) {
> +        return machine;
> +    }
> +    printf("Supported machines are:\n");
> +    for (m = first_machine; m != NULL; m = m->next) {
> +        if (m->alias) {
> +            printf("%-10s %s (alias of %s)\n", m->alias, m->desc, m->name);
> +        }
> +        printf("%-10s %s%s\n", m->name, m->desc,
> +               m->is_default ? " (default)" : "");
> +    }
> +    exit(!name || *name != '?');
> +}
> +
>  static int tcg_init(void)
>  {
>      return 0;
> @@ -2144,20 +2165,7 @@ int main(int argc, char **argv, char **envp)
>              }
>              switch(popt->index) {
>              case QEMU_OPTION_M:
> -                machine = find_machine(optarg);
> -                if (!machine) {
> -                    QEMUMachine *m;
> -                    printf("Supported machines are:\n");
> -                    for(m = first_machine; m != NULL; m = m->next) {
> -                        if (m->alias)
> -                            printf("%-10s %s (alias of %s)\n",
> -                                   m->alias, m->desc, m->name);
> -                        printf("%-10s %s%s\n",
> -                               m->name, m->desc,
> -                               m->is_default ? " (default)" : "");
> -                    }
> -                    exit(*optarg != '?');
> -                }
> +                machine = machine_parse(optarg);
>                  break;
>              case QEMU_OPTION_cpu:
>                  /* hw initialization will check this */
> @@ -2675,11 +2683,12 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_machine:
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_reset(olist);
> -                opts = qemu_opts_parse(olist, optarg, 0);
> +                opts = qemu_opts_parse(olist, optarg, 1);
>                  if (!opts) {
>                      fprintf(stderr, "parse error: %s\n", optarg);
>                      exit(1);
>                  }
> +                machine = machine_parse(qemu_opt_get(opts, "type"));
>                  break;
>              case QEMU_OPTION_usb:
>                  usb_enabled = 1;
> @@ -2941,8 +2950,8 @@ int main(int argc, char **argv, char **envp)
>              p = qemu_opt_get(QTAILQ_FIRST(&list->head), "accel");
>          }
>          if (p == NULL) {
> -            opts = qemu_opts_parse(qemu_find_opts("machine"),
> -                                   machine->default_machine_opts, 0);
> +            qemu_opts_reset(list);
> +            opts = qemu_opts_parse(list, machine->default_machine_opts, 0);
>              if (!opts) {
>                  fprintf(stderr, "parse error for machine %s: %s\n",
>                          machine->name, machine->default_machine_opts);

-- 
Ian Campbell
Current Noise: Blind Melon - I Wonder

When the going gets weird, the weird turn pro.
		-- Hunter S. Thompson

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] Generalize -machine command line option
  2011-05-25  8:13       ` [Qemu-devel] [PATCH v2 " Ian Campbell
@ 2011-05-25  8:23         ` Jan Kiszka
  2011-05-25  8:34           ` Ian Campbell
  2011-05-25 10:54         ` Anthony PERARD
  1 sibling, 1 reply; 12+ messages in thread
From: Jan Kiszka @ 2011-05-25  8:23 UTC (permalink / raw)
  To: Ian Campbell; +Cc: Anthony PERARD, Anthony Liguori, qemu-devel

On 2011-05-25 10:13, Ian Campbell wrote:
> On Tue, 2011-05-24 at 18:18 +0200, Jan Kiszka wrote:
>> On 2011-05-24 18:06, Ian Campbell wrote:
>>> On Sun, 2011-05-22 at 13:00 +0200, Jan Kiszka wrote:
>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>
>>>> -machine somehow suggests that it selects the machine, but it doesn't.
>>>> Fix that before this command is set in stone.
>>>>
>>>> Actually, -machine should supersede -M and allow to introduce arbitrary
>>>> per-machine options to the command line. That will change the internal
>>>> realization again, but we will be able to keep the user interface
>>>> stable.
>>>>
>>>> CC: Anthony PERARD <anthony.perard@citrix.com>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> "-machine xenfv" doesn't work for me with this patch, it gives:
>>>         Program received signal SIGSEGV, Segmentation fault.
>>>         hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
>>>         36	xc_hcall_buf.c: No such file or directory.
>>>         	in xc_hcall_buf.c
>>>         (gdb) bt
>>>         #0  hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
>>>         #1  0xb7d53f1d in hypercall_buffer_cache_alloc (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:52
>>>         #2  xc__hypercall_buffer_alloc_pages (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:128
>>>         #3  0xb7d54028 in xc__hypercall_buffer_alloc (xch=0x0, b=0xbffff77c, size=16) at xc_hcall_buf.c:162
>>>         #4  0xb7d42719 in xc_get_hvm_param (handle=0x0, dom=248, param=5, value=0xbffff810) at xc_domain.c:1078
>>>         #5  0x08252777 in xen_hvm_init () at /home/ianc/devel/qemu.git/xen-all.c:803
>>>         #6  0x082921e9 in pc_xen_hvm_init (ram_size=536870912, boot_device=0xbffffabb "cda", kernel_filename=0x0, kernel_cmdline=0x82d648f "", initrd_filename=0x0, cpu_model=0x0) at /home/ianc/devel/qemu.git/hw/pc_piix.c:246
>>>         #7  0x081e3f0e in main (argc=26, argv=0xbffffba4, envp=0xbffffc10) at /home/ianc/devel/qemu.git/vl.c:3162
>>>
>>> I suspect this is because xen_init() (which sets xch) is never called,
>>> perhaps because it causes the accelerator to always be tcg? If I use
>>> "-machine xenfv,accel=xen" then it works as expected, -M continues to
>>> work too AFAICT.
>>>
>>> It would be nice to retain the behaviour of defaulting to accel=xen for
>>> machine=xenfv. (to be honest I can't quite see where this behaviour came
>>> from, nor what about this patch changes it...)
>>
>> Well, first of all I think this revealed a Xen bug because it crashes
>> when you try to run xenfv with an inappropriate accelerator, no? What is
>> the result of -machine xenfv,accel=tcg or, without my patch, -M xenfv
>> -machine accel=tcg?
> 
> Unsurprisingly it crashed...
> 
> I'm not sure if this is a bug in the xen side of simply a case of
> providing enough rope. I didn't really follow the threads which resulted
> in the accel stuff all that closely so I'm not sure what the intention
> was, it seems to me that the accel option is invalid with certain
> machine types though.
> 
> I suppose in theory -machine xenpv,accel=kvm might result in xenner or
> something, accel=xenpv,tcg I'm less sure about (perhaps xenner too?).
> For -machine xenfv I don't expect anything other than accel=xen makes
> much sense.

The point is that crashing is always a very poor way of reporting a
misconfiguration to the user. I bet you are able to detect and report
that more gracefully, e.g. during xenfv machine init.

> 
>> Then the question is what accel options are actually picked with
>> -machine xenfv, or why the default options are maybe not considered. I
>> don't have any Xen around, but I will check how far I can debug this -
>> or actually try to understand what I coded if nothing helps.
> 
> I saw a follow up patch -- I'll give it a go.

TIA,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] Generalize -machine command line option
  2011-05-25  8:23         ` Jan Kiszka
@ 2011-05-25  8:34           ` Ian Campbell
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2011-05-25  8:34 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: Anthony PERARD, Anthony Liguori, qemu-devel

On Wed, 2011-05-25 at 10:23 +0200, Jan Kiszka wrote:
> On 2011-05-25 10:13, Ian Campbell wrote:
> > On Tue, 2011-05-24 at 18:18 +0200, Jan Kiszka wrote:
> >> On 2011-05-24 18:06, Ian Campbell wrote:
> >> Well, first of all I think this revealed a Xen bug because it crashes
> >> when you try to run xenfv with an inappropriate accelerator, no? What is
> >> the result of -machine xenfv,accel=tcg or, without my patch, -M xenfv
> >> -machine accel=tcg?
> > 
> > Unsurprisingly it crashed...
> > 
> > I'm not sure if this is a bug in the xen side of simply a case of
> > providing enough rope. I didn't really follow the threads which resulted
> > in the accel stuff all that closely so I'm not sure what the intention
> > was, it seems to me that the accel option is invalid with certain
> > machine types though.
> > 
> > I suppose in theory -machine xenpv,accel=kvm might result in xenner or
> > something, accel=xenpv,tcg I'm less sure about (perhaps xenner too?).
> > For -machine xenfv I don't expect anything other than accel=xen makes
> > much sense.
> 
> The point is that crashing is always a very poor way of reporting a
> misconfiguration to the user. I bet you are able to detect and report
> that more gracefully, e.g. during xenfv machine init.

ACK. How about:

8<------------------------------------

>From b23cadda8da72da480fe7cc8e31ffccd449f4b47 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Wed, 25 May 2011 09:32:07 +0100
Subject: [PATCH] Fail to initialise Xen HVM support gracefully if Xen acceleration not enabled.

This is preferable to the existing segmentation fault due to use of
uninitialised xen_xc context, this is normally inialisted in xen_init which is
not called if xen acceleration is not enabled.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
 xen-all.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/xen-all.c b/xen-all.c
index 377aff7..c04bcbf 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -783,6 +783,12 @@ int xen_hvm_init(void)
     unsigned long ioreq_pfn;
     XenIOState *state;
 
+    if (!xen_xc) {
+        errno = ENOTSUP;
+        perror("xen: xen hvm requires accel=xen");
+        return -errno;
+    }
+
     state = qemu_mallocz(sizeof (XenIOState));
 
     state->xce_handle = xen_xc_evtchn_open(NULL, 0);
-- 
1.7.2.5



-- 
Ian Campbell
Current Noise: Blind Melon - No Rain

Who are you?

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] Generalize -machine command line option
  2011-05-25  8:13       ` [Qemu-devel] [PATCH v2 " Ian Campbell
  2011-05-25  8:23         ` Jan Kiszka
@ 2011-05-25 10:54         ` Anthony PERARD
  2011-05-26  9:04           ` Ian Campbell
  1 sibling, 1 reply; 12+ messages in thread
From: Anthony PERARD @ 2011-05-25 10:54 UTC (permalink / raw)
  To: Jan Kiszka, Ian Campbell; +Cc: qemu-devel

On Wed, 25 May 2011, Ian Campbell wrote:

> On Tue, 2011-05-24 at 18:18 +0200, Jan Kiszka wrote:
> > On 2011-05-24 18:06, Ian Campbell wrote:
> > > On Sun, 2011-05-22 at 13:00 +0200, Jan Kiszka wrote:
> > >> From: Jan Kiszka <jan.kiszka@siemens.com>
> > >>
> > >> -machine somehow suggests that it selects the machine, but it doesn't.
> > >> Fix that before this command is set in stone.
> > >>
> > >> Actually, -machine should supersede -M and allow to introduce arbitrary
> > >> per-machine options to the command line. That will change the internal
> > >> realization again, but we will be able to keep the user interface
> > >> stable.
> > >>
> > >> CC: Anthony PERARD <anthony.perard@citrix.com>
> > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > >
> > > "-machine xenfv" doesn't work for me with this patch, it gives:
> > >         Program received signal SIGSEGV, Segmentation fault.
> > >         hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
> > >         36	xc_hcall_buf.c: No such file or directory.
> > >         	in xc_hcall_buf.c
> > >         (gdb) bt
> > >         #0  hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
> > >         #1  0xb7d53f1d in hypercall_buffer_cache_alloc (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:52
> > >         #2  xc__hypercall_buffer_alloc_pages (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:128
> > >         #3  0xb7d54028 in xc__hypercall_buffer_alloc (xch=0x0, b=0xbffff77c, size=16) at xc_hcall_buf.c:162
> > >         #4  0xb7d42719 in xc_get_hvm_param (handle=0x0, dom=248, param=5, value=0xbffff810) at xc_domain.c:1078
> > >         #5  0x08252777 in xen_hvm_init () at /home/ianc/devel/qemu.git/xen-all.c:803
> > >         #6  0x082921e9 in pc_xen_hvm_init (ram_size=536870912, boot_device=0xbffffabb "cda", kernel_filename=0x0, kernel_cmdline=0x82d648f "", initrd_filename=0x0, cpu_model=0x0) at /home/ianc/devel/qemu.git/hw/pc_piix.c:246
> > >         #7  0x081e3f0e in main (argc=26, argv=0xbffffba4, envp=0xbffffc10) at /home/ianc/devel/qemu.git/vl.c:3162
> > >
> > > I suspect this is because xen_init() (which sets xch) is never called,
> > > perhaps because it causes the accelerator to always be tcg? If I use
> > > "-machine xenfv,accel=xen" then it works as expected, -M continues to
> > > work too AFAICT.
> > >
> > > It would be nice to retain the behaviour of defaulting to accel=xen for
> > > machine=xenfv. (to be honest I can't quite see where this behaviour came
> > > from, nor what about this patch changes it...)
> >
> > Well, first of all I think this revealed a Xen bug because it crashes
> > when you try to run xenfv with an inappropriate accelerator, no? What is
> > the result of -machine xenfv,accel=tcg or, without my patch, -M xenfv
> > -machine accel=tcg?
>
> Unsurprisingly it crashed...
>
> I'm not sure if this is a bug in the xen side of simply a case of
> providing enough rope. I didn't really follow the threads which resulted
> in the accel stuff all that closely so I'm not sure what the intention
> was, it seems to me that the accel option is invalid with certain
> machine types though.

This options was here to initialise the xen stuff early in the QEMU
initilisation, and the purpose was to replace --enable-xen (and
--enable-kvm) at the same occasion.

But since most of the xen stuff have been put in xen_hvm_init, called
by machine{xenfv}->init(), it's could make sense to call xen_init from
xen_hvm_init, or just check if it have been called.

> I suppose in theory -machine xenpv,accel=kvm might result in xenner or
> something, accel=xenpv,tcg I'm less sure about (perhaps xenner too?).
> For -machine xenfv I don't expect anything other than accel=xen makes
> much sense.

But a -machine pc,accel=xen could result in a xenfv machine. We already
use the pc machine for xenfv with few difference. So a -machine
pc,accel=xen:kvm could use the hypervisor present on the machine :)
even if it's not realy useful in many case.


-- 
Anthony PERARD

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/2] Generalize -machine command line option
  2011-05-25 10:54         ` Anthony PERARD
@ 2011-05-26  9:04           ` Ian Campbell
  0 siblings, 0 replies; 12+ messages in thread
From: Ian Campbell @ 2011-05-26  9:04 UTC (permalink / raw)
  To: Anthony PERARD; +Cc: Jan Kiszka, qemu-devel

On Wed, 2011-05-25 at 11:54 +0100, Anthony PERARD wrote:
> On Wed, 25 May 2011, Ian Campbell wrote:
> 
> > On Tue, 2011-05-24 at 18:18 +0200, Jan Kiszka wrote:
> > > On 2011-05-24 18:06, Ian Campbell wrote:
> > > > On Sun, 2011-05-22 at 13:00 +0200, Jan Kiszka wrote:
> > > >> From: Jan Kiszka <jan.kiszka@siemens.com>
> > > >>
> > > >> -machine somehow suggests that it selects the machine, but it doesn't.
> > > >> Fix that before this command is set in stone.
> > > >>
> > > >> Actually, -machine should supersede -M and allow to introduce arbitrary
> > > >> per-machine options to the command line. That will change the internal
> > > >> realization again, but we will be able to keep the user interface
> > > >> stable.
> > > >>
> > > >> CC: Anthony PERARD <anthony.perard@citrix.com>
> > > >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > > >
> > > > "-machine xenfv" doesn't work for me with this patch, it gives:
> > > >         Program received signal SIGSEGV, Segmentation fault.
> > > >         hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
> > > >         36	xc_hcall_buf.c: No such file or directory.
> > > >         	in xc_hcall_buf.c
> > > >         (gdb) bt
> > > >         #0  hypercall_buffer_cache_lock (xch=0x0) at xc_hcall_buf.c:36
> > > >         #1  0xb7d53f1d in hypercall_buffer_cache_alloc (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:52
> > > >         #2  xc__hypercall_buffer_alloc_pages (xch=0x0, b=0xbffff77c, nr_pages=1) at xc_hcall_buf.c:128
> > > >         #3  0xb7d54028 in xc__hypercall_buffer_alloc (xch=0x0, b=0xbffff77c, size=16) at xc_hcall_buf.c:162
> > > >         #4  0xb7d42719 in xc_get_hvm_param (handle=0x0, dom=248, param=5, value=0xbffff810) at xc_domain.c:1078
> > > >         #5  0x08252777 in xen_hvm_init () at /home/ianc/devel/qemu.git/xen-all.c:803
> > > >         #6  0x082921e9 in pc_xen_hvm_init (ram_size=536870912, boot_device=0xbffffabb "cda", kernel_filename=0x0, kernel_cmdline=0x82d648f "", initrd_filename=0x0, cpu_model=0x0) at /home/ianc/devel/qemu.git/hw/pc_piix.c:246
> > > >         #7  0x081e3f0e in main (argc=26, argv=0xbffffba4, envp=0xbffffc10) at /home/ianc/devel/qemu.git/vl.c:3162
> > > >
> > > > I suspect this is because xen_init() (which sets xch) is never called,
> > > > perhaps because it causes the accelerator to always be tcg? If I use
> > > > "-machine xenfv,accel=xen" then it works as expected, -M continues to
> > > > work too AFAICT.
> > > >
> > > > It would be nice to retain the behaviour of defaulting to accel=xen for
> > > > machine=xenfv. (to be honest I can't quite see where this behaviour came
> > > > from, nor what about this patch changes it...)
> > >
> > > Well, first of all I think this revealed a Xen bug because it crashes
> > > when you try to run xenfv with an inappropriate accelerator, no? What is
> > > the result of -machine xenfv,accel=tcg or, without my patch, -M xenfv
> > > -machine accel=tcg?
> >
> > Unsurprisingly it crashed...
> >
> > I'm not sure if this is a bug in the xen side of simply a case of
> > providing enough rope. I didn't really follow the threads which resulted
> > in the accel stuff all that closely so I'm not sure what the intention
> > was, it seems to me that the accel option is invalid with certain
> > machine types though.
> 
> This options was here to initialise the xen stuff early in the QEMU
> initilisation, and the purpose was to replace --enable-xen (and
> --enable-kvm) at the same occasion.
> 
> But since most of the xen stuff have been put in xen_hvm_init, called
> by machine{xenfv}->init(), it's could make sense to call xen_init from
> xen_hvm_init, or just check if it have been called.

I posted a patch to do that check elsewhere in this thread, I did wonder
about moving the call too but I'll leave that up to you.

> > I suppose in theory -machine xenpv,accel=kvm might result in xenner or
> > something, accel=xenpv,tcg I'm less sure about (perhaps xenner too?).
> > For -machine xenfv I don't expect anything other than accel=xen makes
> > much sense.
> 
> But a -machine pc,accel=xen could result in a xenfv machine. We already
> use the pc machine for xenfv with few difference. So a -machine
> pc,accel=xen:kvm could use the hypervisor present on the machine :)
> even if it's not realy useful in many case.

That seems reasonable, I guess.

Ian.

-- 
Ian Campbell
Current Noise: She Said Destroy - No Zen

Tell me, O Octopus, I begs,
Is those things arms, or is they legs?
I marvel at thee, Octopus;
If I were thou, I'd call me us.
		-- Ogden Nash

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v3 1/2] Generalize -machine command line option
  2011-05-25  7:03       ` [Qemu-devel] [PATCH v3 " Jan Kiszka
  2011-05-25  8:17         ` Ian Campbell
@ 2011-06-07 15:58         ` Jan Kiszka
  1 sibling, 0 replies; 12+ messages in thread
From: Jan Kiszka @ 2011-06-07 15:58 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Anthony PERARD, qemu-devel, Ian Campbell

On 2011-05-25 09:03, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> -machine somehow suggests that it selects the machine, but it doesn't.
> Fix that before this command is set in stone.
> 
> Actually, -machine should supersede -M and allow to introduce arbitrary
> per-machine options to the command line. That will change the internal
> realization again, but we will be able to keep the user interface
> stable.

Ping (and also for
http://thread.gmane.org/gmane.comp.emulators.qemu/103348).

Jan

> 
> CC: Anthony PERARD <anthony.perard@citrix.com>
> CC: Ian Campbell <ijc@hellion.org.uk>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Changes in v3: 
>  - fix regression of default machine options handling, -machine xenfv
>    selects accel=xen again
>    (I really hope we can clean up the defaults, make them more
>    generally useful when switching to some QCFG.)
> 
> Changes in v2:
>  - fix regression of -M my factoring out machine_parse and using it for
>    both old and new command.
> 
>  qemu-config.c   |    5 +++++
>  qemu-options.hx |   20 +++++++++++++++-----
>  vl.c            |   43 ++++++++++++++++++++++++++-----------------
>  3 files changed, 46 insertions(+), 22 deletions(-)
> 
> diff --git a/qemu-config.c b/qemu-config.c
> index 5d7ffa2..01751b4 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -452,9 +452,14 @@ QemuOptsList qemu_option_rom_opts = {
>  
>  static QemuOptsList qemu_machine_opts = {
>      .name = "machine",
> +    .implied_opt_name = "type",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
>      .desc = {
>          {
> +            .name = "type",
> +            .type = QEMU_OPT_STRING,
> +            .help = "emulated machine"
> +        }, {
>              .name = "accel",
>              .type = QEMU_OPT_STRING,
>              .help = "accelerator list",
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 82e085a..0dbc028 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2033,13 +2033,23 @@ if KVM support is enabled when compiling.
>  ETEXI
>  
>  DEF("machine", HAS_ARG, QEMU_OPTION_machine, \
> -    "-machine accel=accel1[:accel2]    use an accelerator (kvm,xen,tcg), default is tcg\n", QEMU_ARCH_ALL)
> +    "-machine [type=]name[,prop[=value][,...]]\n"
> +    "                selects emulated machine (-machine ? for list)\n"
> +    "                property accel=accel1[:accel2[:...]] selects accelerator\n"
> +    "                supported accelerators are kvm, xen, tcg (default: tcg)\n",
> +    QEMU_ARCH_ALL)
>  STEXI
> -@item -machine accel=@var{accels}
> +@item -machine [type=]@var{name}[,prop=@var{value}[,...]]
>  @findex -machine
> -This is use to enable an accelerator, in kvm,xen,tcg.
> -By default, it use only tcg. If there a more than one accelerator
> -specified, the next one is used if the first don't work.
> +Select the emulated machine by @var{name}. Use @code{-machine ?} to list
> +available machines. Supported machine properties are:
> +@table @option
> +@item accel=@var{accels1}[:@var{accels2}[:...]]
> +This is used to enable an accelerator. Depending on the target architecture,
> +kvm, xen, or tcg can be available. By default, tcg is used. If there is more
> +than one accelerator specified, the next one is used if the previous one fails
> +to initialize.
> +@end table
>  ETEXI
>  
>  DEF("xen-domid", HAS_ARG, QEMU_OPTION_xen_domid,
> diff --git a/vl.c b/vl.c
> index b362871..1865a8d 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1891,6 +1891,27 @@ static int debugcon_parse(const char *devname)
>      return 0;
>  }
>  
> +static QEMUMachine *machine_parse(const char *name)
> +{
> +    QEMUMachine *m, *machine = NULL;
> +
> +    if (name) {
> +        machine = find_machine(name);
> +    }
> +    if (machine) {
> +        return machine;
> +    }
> +    printf("Supported machines are:\n");
> +    for (m = first_machine; m != NULL; m = m->next) {
> +        if (m->alias) {
> +            printf("%-10s %s (alias of %s)\n", m->alias, m->desc, m->name);
> +        }
> +        printf("%-10s %s%s\n", m->name, m->desc,
> +               m->is_default ? " (default)" : "");
> +    }
> +    exit(!name || *name != '?');
> +}
> +
>  static int tcg_init(void)
>  {
>      return 0;
> @@ -2144,20 +2165,7 @@ int main(int argc, char **argv, char **envp)
>              }
>              switch(popt->index) {
>              case QEMU_OPTION_M:
> -                machine = find_machine(optarg);
> -                if (!machine) {
> -                    QEMUMachine *m;
> -                    printf("Supported machines are:\n");
> -                    for(m = first_machine; m != NULL; m = m->next) {
> -                        if (m->alias)
> -                            printf("%-10s %s (alias of %s)\n",
> -                                   m->alias, m->desc, m->name);
> -                        printf("%-10s %s%s\n",
> -                               m->name, m->desc,
> -                               m->is_default ? " (default)" : "");
> -                    }
> -                    exit(*optarg != '?');
> -                }
> +                machine = machine_parse(optarg);
>                  break;
>              case QEMU_OPTION_cpu:
>                  /* hw initialization will check this */
> @@ -2675,11 +2683,12 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_machine:
>                  olist = qemu_find_opts("machine");
>                  qemu_opts_reset(olist);
> -                opts = qemu_opts_parse(olist, optarg, 0);
> +                opts = qemu_opts_parse(olist, optarg, 1);
>                  if (!opts) {
>                      fprintf(stderr, "parse error: %s\n", optarg);
>                      exit(1);
>                  }
> +                machine = machine_parse(qemu_opt_get(opts, "type"));
>                  break;
>              case QEMU_OPTION_usb:
>                  usb_enabled = 1;
> @@ -2941,8 +2950,8 @@ int main(int argc, char **argv, char **envp)
>              p = qemu_opt_get(QTAILQ_FIRST(&list->head), "accel");
>          }
>          if (p == NULL) {
> -            opts = qemu_opts_parse(qemu_find_opts("machine"),
> -                                   machine->default_machine_opts, 0);
> +            qemu_opts_reset(list);
> +            opts = qemu_opts_parse(list, machine->default_machine_opts, 0);
>              if (!opts) {
>                  fprintf(stderr, "parse error for machine %s: %s\n",
>                          machine->name, machine->default_machine_opts);


-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2011-06-07 15:58 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-20 11:18 [Qemu-devel] [PATCH 1/2] Generalize -machine command line option Jan Kiszka
2011-05-22 11:00 ` [Qemu-devel] [PATCH v2 " Jan Kiszka
2011-05-24 16:06   ` Ian Campbell
2011-05-24 16:18     ` Jan Kiszka
2011-05-25  7:03       ` [Qemu-devel] [PATCH v3 " Jan Kiszka
2011-05-25  8:17         ` Ian Campbell
2011-06-07 15:58         ` Jan Kiszka
2011-05-25  8:13       ` [Qemu-devel] [PATCH v2 " Ian Campbell
2011-05-25  8:23         ` Jan Kiszka
2011-05-25  8:34           ` Ian Campbell
2011-05-25 10:54         ` Anthony PERARD
2011-05-26  9:04           ` Ian Campbell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.