Re: [Qemu-devel] [PATCH v2 1/2] Generalize -machine command line option

[Ian Campbell]

On Sun, 2011-05-22 at 13:00 +0200, Jan Kiszka wrote:
> From: Jan Kiszka <address@hidden>
> 
> -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 <address@hidden>
> Signed-off-by: Jan Kiszka <address@hidden>

"-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
> address@hidden -machine address@hidden
> address@hidden -machine address@hidden,address@hidden,...]]
>  @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:
> address@hidden @option
> address@hidden address@hidden:@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.
> address@hidden 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