From: Marcel Apfelbaum <marcel.a@redhat.com>
To: Greg Bellows <greg.bellows@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>,
Alexander Graf <agraf@suse.de>,
QEMU Developers <qemu-devel@nongnu.org>,
mst@redhat.com
Subject: Re: [Qemu-devel] [PATCH 1/3] machine: remove qemu_machine_opts global list
Date: Thu, 11 Dec 2014 07:05:09 +0200 [thread overview]
Message-ID: <1418274309.15503.47.camel@localhost.localdomain> (raw)
In-Reply-To: <CAOgzsHW65o13yOzN7ZrJrh2vRrK7m1eOvOcF3=OMu8R3pML8aQ@mail.gmail.com>
On Wed, 2014-12-10 at 16:59 -0600, Greg Bellows wrote:
>
>
> On 10 December 2014 at 07:19, Marcel Apfelbaum <marcel.a@redhat.com>
> wrote:
> QEMU has support for options per machine, keeping
> a global list of options is no longer necessary.
>
> Signed-off-by: Marcel Apfelbaum <marcel.a@redhat.com>
> ---
> 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 <greg.bellows@linaro.org>
Appreciated.
Thanks,
Marcel
next prev parent reply other threads:[~2014-12-11 5:47 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-10 13:19 [Qemu-devel] [PATCH 0/3] machine: dynamic options per machine type Marcel Apfelbaum
2014-12-10 13:19 ` [Qemu-devel] [PATCH 1/3] machine: remove qemu_machine_opts global list Marcel Apfelbaum
2014-12-10 22:59 ` Greg Bellows
2014-12-11 5:05 ` Marcel Apfelbaum [this message]
2014-12-11 14:57 ` Greg Bellows
2014-12-15 15:13 ` Alexander Graf
2014-12-15 15:20 ` Paolo Bonzini
2014-12-10 13:19 ` [Qemu-devel] [PATCH 2/3] vl.c: simplified machine_set_property Marcel Apfelbaum
2014-12-11 17:49 ` Greg Bellows
2014-12-10 13:19 ` [Qemu-devel] [PATCH 3/3] vl.c: add HMP help to machine Marcel Apfelbaum
2014-12-15 15:16 ` Alexander Graf
2014-12-15 15:20 ` Daniel P. Berrange
2014-12-15 19:24 ` Marcel Apfelbaum
2014-12-15 15:14 ` [Qemu-devel] [PATCH 0/3] machine: dynamic options per machine type Alexander Graf
2014-12-15 17:24 ` Peter Maydell
2014-12-15 17:33 ` Alexander Graf
2014-12-15 17:55 ` Greg Bellows
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1418274309.15503.47.camel@localhost.localdomain \
--to=marcel.a@redhat.com \
--cc=agraf@suse.de \
--cc=greg.bellows@linaro.org \
--cc=mst@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).