machine-specific command line switches

For a different project, I once wrote a patch to organize purely machine-specific command line switches under the hood of the respective machine implementations. Now the MusicPal has precisely that need as well. So I reanimated the patch, and here we go:

The idea is to add two fields to QEMUMachine and process them:
o options_help - a string that is inserted under a separate section of the "qemu -h" output.
o parse_option - a callback invoked if a given option was not handled by the generic code. It returns -1 if the option is unkown, 0 if it is know but comes without an argument, and 1 when the argument was consumed.

Index: b/vl.c
===================================================================

vl.c +3 -8

@@ -7141,6 +7141,8 @@ static int main_loop(void)
 
 static void help(int exitcode)
 {
+    QEMUMachine *m;
+
    printf("QEMU PC emulator version " QEMU_VERSION ", Copyright (c) 2003-2008 Fabrice Bellard\n"
           "usage: %s [options] [disk_image]\n"
           "\n"

@@ -7275,14 +7277,7 @@ static void help(int exitcode)
           "-clock          force the use of the given methods for timer alarm.\n"
           "                To see what timers are available use -clock ?\n"
           "-startdate      select initial date of the clock\n"
-           "\n"
-           "During emulation, the following keys are useful:\n"
-           "ctrl-alt-f      toggle full screen\n"
-           "ctrl-alt-n      switch to virtual console 'n'\n"
-           "ctrl-alt        toggle mouse and keyboard grab\n"
-           "\n"
-           "When using -nographic, press 'ctrl-a h' to get some help.\n"
-           ,
+           "\n",
           "qemu",
           DEFAULT_RAM_SIZE,
 #ifndef _WIN32

vl.c +2 -2

@@ -7673,7 +7679,7 @@ int main(int argc, char **argv)
    const char *gdbstub_port;
 #endif
    uint32_t boot_devices_bitmap = 0;
-    int i;
+    int i, result;
    int snapshot, linux_boot, net_boot;
    const char *initrd_filename;
    const char *kernel_filename, *kernel_cmdline;

@@ -7692,7 +7698,7 @@ int main(int argc, char **argv)
    const char *parallel_devices[MAX_PARALLEL_PORTS];
    int parallel_device_index;
    const char *loadvm = NULL;
-    QEMUMachine *machine;
+    QEMUMachine *machine, *m;
    const char *cpu_model;
    const char *usb_devices[MAX_USB_CMDLINE];
    int usb_devices_index;

Quoted 5 lines · Jan Kiszka

For a different project, I once wrote a patch to organize purely machine-specific command line switches under the hood of the respective machine implementations. Now the MusicPal has precisely that need as well. So I reanimated the patch, and here we go:

Quoted 7 lines

The idea is to add two fields to QEMUMachine and process them:
o options_help - a string that is inserted under a separate section of the "qemu -h" output.
o parse_option - a callback invoked if a given option was not handled by the generic code. It returns -1 if the option is unkown, 0 if it is know but comes without an argument, and 1 when the argument was consumed.

This would be quite useful for QEMUAccel too.

So...

hw/boards.h typedef void QEMUMachineInitFunc(ram_add · line 24

Quoted 12 lines

hw/boards.h +4 -0

+typedef int QEMUMachineParseOption(const char *optname, const char *optarg);
+
 typedef struct QEMUMachine {
    const char *name;
    const char *desc;
    QEMUMachineInitFunc *init;
 #define RAMSIZE_FIXED  (1 << 0)
    ram_addr_t ram_require;
+    const char *options_help;
+    QEMUMachineParseOption *parse_option;
    struct QEMUMachine *next;
 } QEMUMachine;

Why don't turn the naming into a more generic one? Maybe QEMUOpts, or smth like this.

Quoted 32 lines

Index: b/vl.c
===================================================================

vl.c +3 -8

@@ -7141,6 +7141,8 @@ static int main_loop(void)
 
 static void help(int exitcode)
 {
+    QEMUMachine *m;
+
    printf("QEMU PC emulator version " QEMU_VERSION ", Copyright (c) 2003-2008 Fabrice Bellard\n"
           "usage: %s [options] [disk_image]\n"
           "\n"

@@ -7275,14 +7277,7 @@ static void help(int exitcode)
           "-clock          force the use of the given methods for timer alarm.\n"
           "                To see what timers are available use -clock ?\n"
           "-startdate      select initial date of the clock\n"
-           "\n"
-           "During emulation, the following keys are useful:\n"
-           "ctrl-alt-f      toggle full screen\n"
-           "ctrl-alt-n      switch to virtual console 'n'\n"
-           "ctrl-alt        toggle mouse and keyboard grab\n"
-           "\n"
-           "When using -nographic, press 'ctrl-a h' to get some help.\n"
-           ,
+           "\n",
           "qemu",
           DEFAULT_RAM_SIZE,
 #ifndef _WIN32

@@ -7291,6 +7286,17 @@ static void help(int exitcode)
 #endif
           DEFAULT_GDBSTUB_PORT,

vl.c static void help(int exitcode) · line 7293

Quoted 6 lines

vl.c +5 -0

           "/tmp/qemu.log");
+    for (m = first_machine; m != NULL; m = m->next) {
+        if (m->options_help)
+            printf("Options specific to %s machine:\n%s\n",
+                   m->name, m->options_help);
+    }

So, If I understand correctly what you mean here, This will print out specific options for every registered machine, right?

It does not sound correct, since we won't have support for more than one in the same binary anyway. It looks correct, tough, if we think

that we're not representing 'machines', but anything dumping specific options here. But in this case, wouldn't it be better to leave the whole help string
to the user of the interface, instead of just using m->name and m->help?

Quoted 23 lines

vl.c +2 -2

@@ -7673,7 +7679,7 @@ int main(int argc, char **argv)
    const char *gdbstub_port;
 #endif
    uint32_t boot_devices_bitmap = 0;
-    int i;
+    int i, result;
    int snapshot, linux_boot, net_boot;
    const char *initrd_filename;
    const char *kernel_filename, *kernel_cmdline;

@@ -7692,7 +7698,7 @@ int main(int argc, char **argv)
    const char *parallel_devices[MAX_PARALLEL_PORTS];
    int parallel_device_index;
    const char *loadvm = NULL;
-    QEMUMachine *machine;
+    QEMUMachine *machine, *m;
    const char *cpu_model;
    const char *usb_devices[MAX_USB_CMDLINE];
    int usb_devices_index;

@@ -7784,6 +7790,21 @@ int main(int argc, char **argv)
            /* Treat --foo the same as -foo.  */
            if (r[1] == '-')
                r++;

vl.c int main(int argc, char **argv) · line 7796

Quoted 4 lines

vl.c +4 -0

+
+            result = -1;
+            for (m = first_machine; m != NULL; m = m->next) {
+                if (m->parse_option) {

I don't like this very much. There's no point in having specific options without this parse_option anyway. So it would be better to check for it before displaying the options at all, and simplify the code here.

vl.c int main(int argc, char **argv) · line 7807

Quoted 12 lines

vl.c +11 -0

+                    result = m->parse_option(r,
+                        (optind < argc) ? argv[optind] : NULL);
+                    if (result >= 0)
+                        break;
+                }
+            }
+            if (result >= 0) {
+                optind += result;
+                continue;
+            }
+

Other than the commented, it all looks very good.

Quoted 66 lines · Glauber Costa

Quoted 5 lines · Jan Kiszka

For a different project, I once wrote a patch to organize purely machine-specific command line switches under the hood of the respective machine implementations. Now the MusicPal has precisely that need as well. So I reanimated the patch, and here we go:

Quoted 7 lines

The idea is to add two fields to QEMUMachine and process them:
o options_help - a string that is inserted under a separate section of the "qemu -h" output.
o parse_option - a callback invoked if a given option was not handled by the generic code. It returns -1 if the option is unkown, 0 if it is know but comes without an argument, and 1 when the argument was consumed.

This would be quite useful for QEMUAccel too.

So...

hw/boards.h typedef void QEMUMachineInitFunc(ram_add · line 24

Quoted 12 lines

hw/boards.h +4 -0

+typedef int QEMUMachineParseOption(const char *optname, const char *optarg);
+
 typedef struct QEMUMachine {
    const char *name;
    const char *desc;
    QEMUMachineInitFunc *init;
 #define RAMSIZE_FIXED  (1 << 0)
    ram_addr_t ram_require;
+    const char *options_help;
+    QEMUMachineParseOption *parse_option;
    struct QEMUMachine *next;
 } QEMUMachine;

Why don't turn the naming into a more generic one? Maybe QEMUOpts, or smth like this.

Quoted 32 lines

Index: b/vl.c
===================================================================

vl.c +3 -8

@@ -7141,6 +7141,8 @@ static int main_loop(void)
 
 static void help(int exitcode)
 {
+    QEMUMachine *m;
+
    printf("QEMU PC emulator version " QEMU_VERSION ", Copyright (c) 2003-2008 Fabrice Bellard\n"
           "usage: %s [options] [disk_image]\n"
           "\n"

@@ -7275,14 +7277,7 @@ static void help(int exitcode)
           "-clock          force the use of the given methods for timer alarm.\n"
           "                To see what timers are available use -clock ?\n"
           "-startdate      select initial date of the clock\n"
-           "\n"
-           "During emulation, the following keys are useful:\n"
-           "ctrl-alt-f      toggle full screen\n"
-           "ctrl-alt-n      switch to virtual console 'n'\n"
-           "ctrl-alt        toggle mouse and keyboard grab\n"
-           "\n"
-           "When using -nographic, press 'ctrl-a h' to get some help.\n"
-           ,
+           "\n",
           "qemu",
           DEFAULT_RAM_SIZE,
 #ifndef _WIN32

@@ -7291,6 +7286,17 @@ static void help(int exitcode)
 #endif
           DEFAULT_GDBSTUB_PORT,

Quoted 8 lines

vl.c static void help(int exitcode) · line 7293

Quoted 6 lines

vl.c +5 -0

           "/tmp/qemu.log");
+    for (m = first_machine; m != NULL; m = m->next) {
+        if (m->options_help)
+            printf("Options specific to %s machine:\n%s\n",
+                   m->name, m->options_help);
+    }

So, If I understand correctly what you mean here, This will print out specific options for every registered machine, right?

Right.

Quoted 2 lines

It does not sound correct, since we won't have support for more than one in the same binary anyway. It looks correct, tough, if we think

I was thinking of such examples:

$ arm-softmmu/qemu-system-arm -M ?
Supported machines are:
integratorcp ARM Integrator/CP (ARM926EJ-S) (default)
versatilepb ARM Versatile/PB (ARM926EJ-S)
versatileab ARM Versatile/AB (ARM926EJ-S)
realview   ARM RealView Emulation Baseboard (ARM926EJ-S)
akita      Akita PDA (PXA270)
spitz      Spitz PDA (PXA270)
borzoi     Borzoi PDA (PXA270)
terrier    Terrier PDA (PXA270)
cheetah    Palm Tungsten|E aka. Cheetah PDA (OMAP310)
n800       Nokia N800 tablet aka. RX-34 (OMAP2420)
n810       Nokia N810 tablet aka. RX-44 (OMAP2420)
lm3s811evb Stellaris LM3S811EVB
lm3s6965evb Stellaris LM3S6965EVB
connex     Gumstix Connex (PXA255)
verdex     Gumstix Verdex (PXA270)
mainstone  Mainstone II (PXA27x)
musicpal   Marvell 88w8618 / MusicPal (ARM926EJ-S)

And only the latter one needs a special switch here. Thus the user should know that this switch is not interpreted if some other machine is picked.

Quoted 4 lines

that we're not representing 'machines', but anything dumping specific options here. But in this case, wouldn't it be better to leave the whole help string
to the user of the interface, instead of just using m->name and m->help?

Nevertheless, I do agree that a non-machine oriented abstraction would be even nicer in order to organize all those specific options without the help of increasing #ifdef'ery. However, yet no smart idea came to my mind to handle all cases (per-arch, per-machine, per-accelerator, per-whatever - and all this combined).

Quoted 8 lines

vl.c int main(int argc, char **argv) · line 7796

Quoted 4 lines

vl.c +4 -0

+
+            result = -1;
+            for (m = first_machine; m != NULL; m = m->next) {
+                if (m->parse_option) {

I don't like this very much. There's no point in having specific options without this parse_option anyway. So it would be better to check for it before displaying the options at all, and simplify the code here.

Don't understand your concern yet. If machine provides options_help, it is supposed to provide parse_options and vice versa. Thus you don't dump help about non-existent services - unless someone messes up the machine definition.

OK, let's try again and do some brainstorming about more flexible option handling. My initial idea was machine focused (both for the MusicPal and here @work), thus I stuffed the information into the machine descriptor. However, my scenario would be fine as well when the machines register some additional options (in what ever form). The same could be done by arch-common initialization functions for per-arch options or by an accelerator initializer for its special switches. Sounds feasible?