From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55049) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XywbO-0008Lq-8O for qemu-devel@nongnu.org; Thu, 11 Dec 2014 00:47:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XywbI-0007UA-Ki for qemu-devel@nongnu.org; Thu, 11 Dec 2014 00:47:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36998) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XywbI-0007U3-Ch for qemu-devel@nongnu.org; Thu, 11 Dec 2014 00:47:44 -0500 Message-ID: <1418274309.15503.47.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Thu, 11 Dec 2014 07:05:09 +0200 In-Reply-To: References: <1418217570-15517-1-git-send-email-marcel.a@redhat.com> <1418217570-15517-2-git-send-email-marcel.a@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] machine: remove qemu_machine_opts global list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Bellows Cc: Peter Maydell , Alexander Graf , QEMU Developers , mst@redhat.com On Wed, 2014-12-10 at 16:59 -0600, Greg Bellows wrote: > > > On 10 December 2014 at 07:19, Marcel Apfelbaum > wrote: > QEMU has support for options per machine, keeping > a global list of options is no longer necessary. > > Signed-off-by: Marcel Apfelbaum > --- > hw/core/machine.c | 45 +++++++++++++++++++++++++++++ > hw/i386/pc.c | 7 +++++ > hw/ppc/spapr.c | 3 ++ > vl.c | 84 > ++++--------------------------------------------------- > 4 files changed, 61 insertions(+), 78 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 19d3e3a..a0ae5f9 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -291,48 +291,93 @@ static void machine_initfn(Object *obj) > > object_property_add_str(obj, "accel", > machine_get_accel, > machine_set_accel, NULL); > + object_property_set_description(obj, "accel", > + "Accelerator list", > + NULL); > Hi Greg, Thank you for your review! > > If this is a common combination of calls would it make sense to create > a wrapper routine that just does both of these steps together for > readability and convenience? Maybe something like: > > > object_property_add_str_with_desc(obj, "accel", "Accelerator > list" > machine_get_accel, > machine_set_accel, > NULL); I really thought about it, but one wrapper would not be enough, we need also one for object_property_add_bool, object_property_add and so on. The code needed would be twice as this one and for the moment this series is the only reason for that, maybe is not enough. If you or anyone else things we should still go for it, I'll be glad to. > > object_property_add_bool(obj, "kernel-irqchip", > machine_get_kernel_irqchip, > machine_set_kernel_irqchip, > NULL); > + object_property_set_description(obj, "kernel-irqchip", > + "Use KVM in-kernel > irqchip", > + NULL); > object_property_add(obj, "kvm-shadow-mem", "int", > machine_get_kvm_shadow_mem, > machine_set_kvm_shadow_mem, > NULL, NULL, NULL); > + object_property_set_description(obj, "kvm-shadow-mem", > + "KVM shadow MMU size", > + NULL); > object_property_add_str(obj, "kernel", > machine_get_kernel, > machine_set_kernel, NULL); > + object_property_set_description(obj, "kernel", > + "Linux kernel image > file", > + NULL); > object_property_add_str(obj, "initrd", > machine_get_initrd, > machine_set_initrd, NULL); > + object_property_set_description(obj, "initrd", > + "Linux initial ramdisk > file", > + NULL); > object_property_add_str(obj, "append", > machine_get_append, > machine_set_append, NULL); > + object_property_set_description(obj, "append", > + "Linux kernel command > line", > + NULL); > object_property_add_str(obj, "dtb", > machine_get_dtb, machine_set_dtb, > NULL); > + object_property_set_description(obj, "dtb", > + "Linux kernel device tree > file", > + NULL); > object_property_add_str(obj, "dumpdtb", > machine_get_dumpdtb, > machine_set_dumpdtb, NULL); > + object_property_set_description(obj, "dumpdtb", > + "Dump current dtb to a > file and quit", > + NULL); > object_property_add(obj, "phandle-start", "int", > machine_get_phandle_start, > machine_set_phandle_start, > NULL, NULL, NULL); > + object_property_set_description(obj, "phandle-start", > + "The first phandle ID we > may generate dynamically", > + NULL); > object_property_add_str(obj, "dt-compatible", > machine_get_dt_compatible, > machine_set_dt_compatible, > NULL); > + object_property_set_description(obj, "dt-compatible", > + "Overrides the > \"compatible\" property of the dt root node", > + NULL); > object_property_add_bool(obj, "dump-guest-core", > machine_get_dump_guest_core, > machine_set_dump_guest_core, > NULL); > + object_property_set_description(obj, "dump-guest-core", > + "Include guest memory in > a core dump", > > > FWIW, there is an extra space in the desc string that can probably be > corrected. Thanks! Copied exactly from the original :) I'll correct it if I send another version. > > + NULL); > object_property_add_bool(obj, "mem-merge", > machine_get_mem_merge, > machine_set_mem_merge, NULL); > + object_property_set_description(obj, "mem-merge", > + "Enable/disable memory > merge support", > + NULL); > object_property_add_bool(obj, "usb", > machine_get_usb, > machine_set_usb, NULL); > + object_property_set_description(obj, "usb", > + "Set on/off to > enable/disable usb", > + NULL); > object_property_add_str(obj, "firmware", > machine_get_firmware, > machine_set_firmware, NULL); > + object_property_set_description(obj, "firmware", > + "Firmware image", > + NULL); > object_property_add_bool(obj, "iommu", > machine_get_iommu, > machine_set_iommu, NULL); > + object_property_set_description(obj, "iommu", > + "Set on/off to > enable/disable Intel IOMMU (VT-d)", > + NULL); > > /* Register notifier when init is done for sysbus sanity > checks */ > ms->sysbus_notifier.notify = machine_init_notify; > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index f31d55e..01ddd70 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1804,17 +1804,24 @@ static void pc_machine_initfn(Object > *obj) > object_property_add(obj, PC_MACHINE_MEMHP_REGION_SIZE, > "int", > > pc_machine_get_hotplug_memory_region_size, > NULL, NULL, NULL, NULL); > + > pcms->max_ram_below_4g = 1ULL << 32; /* 4G */ > object_property_add(obj, PC_MACHINE_MAX_RAM_BELOW_4G, > "size", > pc_machine_get_max_ram_below_4g, > pc_machine_set_max_ram_below_4g, > NULL, NULL, NULL); > + object_property_set_description(obj, > PC_MACHINE_MAX_RAM_BELOW_4G, > + "Maximum ram below the 4G > boundary (32bit boundary)", > + NULL); > > pcms->vmport = ON_OFF_AUTO_AUTO; > object_property_add(obj, PC_MACHINE_VMPORT, "OnOffAuto", > pc_machine_get_vmport, > pc_machine_set_vmport, > NULL, NULL, NULL); > + object_property_set_description(obj, PC_MACHINE_VMPORT, > + "Enable vmport (pc & > q35)", > + NULL); > > pcms->enforce_aligned_dimm = true; > object_property_add_bool(obj, > PC_MACHINE_ENFORCE_ALIGNED_DIMM, > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 30de25d..08401e0 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1655,6 +1655,9 @@ static void spapr_machine_initfn(Object > *obj) > { > object_property_add_str(obj, "kvm-type", > spapr_get_kvm_type, > spapr_set_kvm_type, NULL); > + object_property_set_description(obj, "kvm-type", > + "Specifies the KVM > virtualization mode (HV, PR)", > + NULL); > } > > static void ppc_cpu_do_nmi_on_cpu(void *arg) > diff --git a/vl.c b/vl.c > index eb89d62..80d30dd 100644 > --- a/vl.c > +++ b/vl.c > @@ -311,84 +311,12 @@ static QemuOptsList qemu_machine_opts = > { > .merge_lists = true, > .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", > - }, { > - .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 = "kvm-type", > - .type = QEMU_OPT_STRING, > - .help = "Specifies the KVM virtualization mode > (HV, PR)", > - },{ > - .name = PC_MACHINE_MAX_RAM_BELOW_4G, > - .type = QEMU_OPT_SIZE, > - .help = "maximum ram below the 4G boundary (32bit > boundary)", > - }, { > - .name = PC_MACHINE_VMPORT, > - .type = QEMU_OPT_STRING, > - .help = "Enable vmport (pc & q35)", > - },{ > - .name = "iommu", > - .type = QEMU_OPT_BOOL, > - .help = "Set on/off to enable/disable Intel IOMMU > (VT-d)", > - }, > - { /* End of list */ } > + /* > + * no elements => accept any > + * sanity checking will happen later > + * when setting machine properties > + */ > + { } > }, > }; > > -- > 1.9.3 > > > > Otherwise... > > > Reviewed-by: Greg Bellows Appreciated. Thanks, Marcel