| Message ID | 20221111141323.246267-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] util/qemu-config: Fix "query-command-line-options" to provide the right values | expand |
Thomas Huth <thuth@redhat.com> writes: > The "query-command-line-options" command uses a hand-crafted list > of options that should be returned for the "machine" parameter. > This is pretty much out of sync with reality, for example settings > like "kvm_shadow_mem" or "accel" are not parameters for the machine > anymore. Also, there is no distinction between the targets here, so > e.g. the s390x-specific values like "loadparm" in this list also > show up with the other targets like x86_64. > > Let's fix this now by geting rid of the hand-crafted list and by > querying the properties of the machine classes instead to assemble > the list. Do we know what uses this command, and how these users are inconvenienced by the flaw you're fixing? I'm asking because the command is pretty much out of sync with reality by (mis-)design. > Fixes: 0a7cf217d8 ("fix regression of qmp_query_command_line_options") Hah: "We need to find a better fix for 2.4." > Signed-off-by: Thomas Huth <thuth@redhat.com>
On 11/11/2022 15.53, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> The "query-command-line-options" command uses a hand-crafted list >> of options that should be returned for the "machine" parameter. >> This is pretty much out of sync with reality, for example settings >> like "kvm_shadow_mem" or "accel" are not parameters for the machine >> anymore. Also, there is no distinction between the targets here, so >> e.g. the s390x-specific values like "loadparm" in this list also >> show up with the other targets like x86_64. >> >> Let's fix this now by geting rid of the hand-crafted list and by >> querying the properties of the machine classes instead to assemble >> the list. > > Do we know what uses this command, and how these users are > inconvenienced by the flaw you're fixing? > > I'm asking because the command is pretty much out of sync with reality > by (mis-)design. libvirt apparently queries this data (see the various tests/qemucapabilitiesdata/*.replies files in their repository), but since it's so much out-of-sync with reality, it's not of a big use there yet. See for example here: https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg00581.html If we finally fix this problem with "query-command-line-options" in QEMU, it should be much easier to deprecate -no-hpet in QEMU, too. Thomas
Thomas Huth <thuth@redhat.com> writes: > On 11/11/2022 15.53, Markus Armbruster wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> The "query-command-line-options" command uses a hand-crafted list >>> of options that should be returned for the "machine" parameter. >>> This is pretty much out of sync with reality, for example settings >>> like "kvm_shadow_mem" or "accel" are not parameters for the machine >>> anymore. Also, there is no distinction between the targets here, so >>> e.g. the s390x-specific values like "loadparm" in this list also >>> show up with the other targets like x86_64. >>> >>> Let's fix this now by geting rid of the hand-crafted list and by >>> querying the properties of the machine classes instead to assemble >>> the list. >> Do we know what uses this command, and how these users are >> inconvenienced by the flaw you're fixing? >> I'm asking because the command is pretty much out of sync with reality >> by (mis-)design. > > libvirt apparently queries this data (see the various tests/qemucapabilitiesdata/*.replies files in their repository), but since > it's so much out-of-sync with reality, it's not of a big use there yet. > > See for example here: > > https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg00581.html > > If we finally fix this problem with "query-command-line-options" in QEMU, it should be much easier to deprecate -no-hpet in QEMU, too. For a value of "fix". While we can fix certain concrete issues with q-c-l-o, which may be wortwhile, the overarching issue is (in my opinion) unfixable: it can only tell us about QemuOpts. QemuOpts is only part of the truth. Last time I checked, it worked for one out of five CLI options. Moreover, our needs have long outgrown QemuOpts' design limitations, which has led to a bunch of bolted-on hacks, none of them represented in q-c-l-o.
On 15/11/2022 08.53, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 11/11/2022 15.53, Markus Armbruster wrote: >>> Thomas Huth <thuth@redhat.com> writes: >>> >>>> The "query-command-line-options" command uses a hand-crafted list >>>> of options that should be returned for the "machine" parameter. >>>> This is pretty much out of sync with reality, for example settings >>>> like "kvm_shadow_mem" or "accel" are not parameters for the machine >>>> anymore. Also, there is no distinction between the targets here, so >>>> e.g. the s390x-specific values like "loadparm" in this list also >>>> show up with the other targets like x86_64. >>>> >>>> Let's fix this now by geting rid of the hand-crafted list and by >>>> querying the properties of the machine classes instead to assemble >>>> the list. >>> Do we know what uses this command, and how these users are >>> inconvenienced by the flaw you're fixing? >>> I'm asking because the command is pretty much out of sync with reality >>> by (mis-)design. >> >> libvirt apparently queries this data (see the various tests/qemucapabilitiesdata/*.replies files in their repository), but since >> it's so much out-of-sync with reality, it's not of a big use there yet. >> >> See for example here: >> >> https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg00581.html >> >> If we finally fix this problem with "query-command-line-options" in QEMU, it should be much easier to deprecate -no-hpet in QEMU, too. > > For a value of "fix". While we can fix certain concrete issues with > q-c-l-o, which may be wortwhile, the overarching issue is (in my > opinion) unfixable: it can only tell us about QemuOpts. > > QemuOpts is only part of the truth. Last time I checked, it worked for > one out of five CLI options. Well, that's another problem. For this patch here, can we please focus on getting rid of that stupid hard-coded and outdated list in our source code? Or do you have something better almost ready that will replace this stuff in the very near future? Thomas
Thomas Huth <thuth@redhat.com> writes: > On 15/11/2022 08.53, Markus Armbruster wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> On 11/11/2022 15.53, Markus Armbruster wrote: >>>> Thomas Huth <thuth@redhat.com> writes: >>>> >>>>> The "query-command-line-options" command uses a hand-crafted list >>>>> of options that should be returned for the "machine" parameter. >>>>> This is pretty much out of sync with reality, for example settings >>>>> like "kvm_shadow_mem" or "accel" are not parameters for the machine >>>>> anymore. Also, there is no distinction between the targets here, so >>>>> e.g. the s390x-specific values like "loadparm" in this list also >>>>> show up with the other targets like x86_64. >>>>> >>>>> Let's fix this now by geting rid of the hand-crafted list and by >>>>> querying the properties of the machine classes instead to assemble >>>>> the list. >>>> >>>> Do we know what uses this command, and how these users are >>>> inconvenienced by the flaw you're fixing? >>>> I'm asking because the command is pretty much out of sync with reality >>>> by (mis-)design. >>> >>> libvirt apparently queries this data (see the various tests/qemucapabilitiesdata/*.replies files in their repository), but since >>> it's so much out-of-sync with reality, it's not of a big use there yet. >>> >>> See for example here: >>> >>> https://lists.gnu.org/archive/html/qemu-devel/2021-12/msg00581.html >>> >>> If we finally fix this problem with "query-command-line-options" in QEMU, it should be much easier to deprecate -no-hpet in QEMU, too. >> >> For a value of "fix". While we can fix certain concrete issues with >> q-c-l-o, which may be wortwhile, the overarching issue is (in my >> opinion) unfixable: it can only tell us about QemuOpts. >> >> QemuOpts is only part of the truth. Last time I checked, it worked for >> one out of five CLI options. > > Well, that's another problem. For this patch here, can we please focus on > getting rid of that stupid hard-coded and outdated list in our source code? > Or do you have something better almost ready that will replace this stuff in > the very near future? I'm not objecting to fixing "concrete issues with q-c-l-o, which may be worthwhile", such as this patch, as long as something actually makes use of the fixes, now or in the immediate future.
diff --git a/util/qemu-config.c b/util/qemu-config.c index 433488aa56..cf47e8a3d0 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -8,6 +8,7 @@ #include "qemu/error-report.h" #include "qemu/option.h" #include "qemu/config-file.h" +#include "hw/boards.h" static QemuOptsList *vm_config_groups[48]; static QemuOptsList *drive_config_groups[5]; @@ -149,97 +150,82 @@ static CommandLineParameterInfoList *get_drive_infolist(void) return head; } -/* restore machine options that are now machine's properties */ -static QemuOptsList machine_opts = { - .merge_lists = true, - .head = QTAILQ_HEAD_INITIALIZER(machine_opts.head), - .desc = { - { - .name = "type", - .type = QEMU_OPT_STRING, - .help = "emulated machine" - },{ - .name = "accel", - .type = QEMU_OPT_STRING, - .help = "accelerator list", - },{ - .name = "kernel_irqchip", - .type = QEMU_OPT_BOOL, - .help = "use KVM in-kernel irqchip", - },{ - .name = "kvm_shadow_mem", - .type = QEMU_OPT_SIZE, - .help = "KVM shadow MMU size", - },{ - .name = "kernel", - .type = QEMU_OPT_STRING, - .help = "Linux kernel image file", - },{ - .name = "initrd", - .type = QEMU_OPT_STRING, - .help = "Linux initial ramdisk file", - },{ - .name = "append", - .type = QEMU_OPT_STRING, - .help = "Linux kernel command line", - },{ - .name = "dtb", - .type = QEMU_OPT_STRING, - .help = "Linux kernel device tree file", - },{ - .name = "dumpdtb", - .type = QEMU_OPT_STRING, - .help = "Dump current dtb to a file and quit", - },{ - .name = "phandle_start", - .type = QEMU_OPT_NUMBER, - .help = "The first phandle ID we may generate dynamically", - },{ - .name = "dt_compatible", - .type = QEMU_OPT_STRING, - .help = "Overrides the \"compatible\" property of the dt root node", - },{ - .name = "dump-guest-core", - .type = QEMU_OPT_BOOL, - .help = "Include guest memory in a core dump", - },{ - .name = "mem-merge", - .type = QEMU_OPT_BOOL, - .help = "enable/disable memory merge support", - },{ - .name = "usb", - .type = QEMU_OPT_BOOL, - .help = "Set on/off to enable/disable usb", - },{ - .name = "firmware", - .type = QEMU_OPT_STRING, - .help = "firmware image", - },{ - .name = "iommu", - .type = QEMU_OPT_BOOL, - .help = "Set on/off to enable/disable Intel IOMMU (VT-d)", - },{ - .name = "suppress-vmdesc", - .type = QEMU_OPT_BOOL, - .help = "Set on to disable self-describing migration", - },{ - .name = "aes-key-wrap", - .type = QEMU_OPT_BOOL, - .help = "enable/disable AES key wrapping using the CPACF wrapping key", - },{ - .name = "dea-key-wrap", - .type = QEMU_OPT_BOOL, - .help = "enable/disable DEA key wrapping using the CPACF wrapping key", - },{ - .name = "loadparm", - .type = QEMU_OPT_STRING, - .help = "Up to 8 chars in set of [A-Za-z0-9. ](lower case chars" - " converted to upper case) to pass to machine" - " loader, boot manager, and guest kernel", - }, - { /* End of list */ } +static CommandLineParameterInfo *objprop_to_cmdline_prop(ObjectProperty *prop) +{ + CommandLineParameterInfo *info; + + info = g_malloc0(sizeof(*info)); + info->name = g_strdup(prop->name); + + if (g_str_equal(prop->type, "bool") || g_str_equal(prop->type, "OnOffAuto")) { + info->type = COMMAND_LINE_PARAMETER_TYPE_BOOLEAN; + } else if (g_str_equal(prop->type, "int")) { + info->type = COMMAND_LINE_PARAMETER_TYPE_NUMBER; + } else if (g_str_equal(prop->type, "size")) { + info->type = COMMAND_LINE_PARAMETER_TYPE_SIZE; + } else { + info->type = COMMAND_LINE_PARAMETER_TYPE_STRING; } -}; + + if (prop->description) { + info->has_help = true; + info->help = g_strdup(prop->description); + } + + return info; +} + +static CommandLineParameterInfoList *query_all_machine_properties(void) +{ + CommandLineParameterInfoList *params = NULL, *clpiter; + CommandLineParameterInfo *info; + GSList *machines, *curr_mach; + ObjectPropertyIterator op_iter; + ObjectProperty *prop; + bool is_new; + + machines = object_class_get_list(TYPE_MACHINE, false); + assert(machines); + + /* Loop over all machine classes */ + for (curr_mach = machines; curr_mach; curr_mach = curr_mach->next) { + object_class_property_iter_init(&op_iter, curr_mach->data); + /* ... and over the properties of each machine: */ + while ((prop = object_property_iter_next(&op_iter))) { + if (!prop->set) { + continue; + } + /* + * Check whether the property has already been put into the list + * (via another machine class) + */ + is_new = true; + for (clpiter = params; clpiter != NULL; clpiter = clpiter->next) { + if (g_str_equal(clpiter->value->name, prop->name)) { + is_new = false; + break; + } + } + /* If it hasn't been added before, add it now to the list */ + if (is_new) { + info = objprop_to_cmdline_prop(prop); + QAPI_LIST_PREPEND(params, info); + } + } + } + + g_slist_free(machines); + + /* Add entry for the "type" parameter */ + info = g_malloc0(sizeof(*info)); + info->name = g_strdup("type"); + info->type = COMMAND_LINE_PARAMETER_TYPE_STRING; + info->has_help = true; + info->help = g_strdup("machine type"); + QAPI_LIST_PREPEND(params, info); + + return params; +} CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option, const char *option, @@ -266,7 +252,7 @@ CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option, if (!has_option || !strcmp(option, "machine")) { info = g_malloc0(sizeof(*info)); info->option = g_strdup("machine"); - info->parameters = query_option_descs(machine_opts.desc); + info->parameters = query_all_machine_properties(); QAPI_LIST_PREPEND(conf_list, info); }
The "query-command-line-options" command uses a hand-crafted list of options that should be returned for the "machine" parameter. This is pretty much out of sync with reality, for example settings like "kvm_shadow_mem" or "accel" are not parameters for the machine anymore. Also, there is no distinction between the targets here, so e.g. the s390x-specific values like "loadparm" in this list also show up with the other targets like x86_64. Let's fix this now by geting rid of the hand-crafted list and by querying the properties of the machine classes instead to assemble the list. Fixes: 0a7cf217d8 ("fix regression of qmp_query_command_line_options") Signed-off-by: Thomas Huth <thuth@redhat.com> --- v2: Query properties from all machine classes, not only from the current one util/qemu-config.c | 168 +++++++++++++++++++++------------------------ 1 file changed, 77 insertions(+), 91 deletions(-)