diff mbox

[v2,1/2] Generalize -machine command line option

Message ID 4DD8ECB0.9050803@web.de
State New
Headers show

Commit Message

Jan Kiszka May 22, 2011, 11 a.m. UTC
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(-)

Comments

Ian Campbell May 24, 2011, 4:06 p.m. UTC | #1
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;
Jan Kiszka May 24, 2011, 4:18 p.m. UTC | #2
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
Ian Campbell May 25, 2011, 8:13 a.m. UTC | #3
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
>
Jan Kiszka May 25, 2011, 8:23 a.m. UTC | #4
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
Anthony PERARD May 25, 2011, 10:54 a.m. UTC | #5
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.
Ian Campbell May 26, 2011, 9:04 a.m. UTC | #6
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.
diff mbox

Patch

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;