On Mon, 2014-03-03 at 11:11 +0100, Paolo Bonzini wrote:
> Il 02/03/2014 14:07, Marcel Apfelbaum ha scritto:
> > @@ -1182,10 +1183,17 @@ ram_addr_t last_ram_offset(void)
> > static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
> > {
> > int ret;
> > + bool dump_guest_core = true;
> > +
> > + if (object_property_is_set(OBJECT(current_machine),
> > + "dump-guest-core", &error_abort)) {
> > + dump_guest_core = object_property_get_bool(OBJECT(current_machine),
> > + "dump-guest-core",
> > + &error_abort);
> > + }
>
> You do not need object_property_is_set. You can just initialize the
> field to true in the instance_init function. Same for mem_merge.
Sure, I can. I was just trying to leave the current functionality "as is",
is some other code will assume "false" as default.
But I can go for required/allowed getters as you suggested, thanks.
>
> > @@ -563,11 +565,14 @@ static qemu_irq *ppce500_init_mpic(PPCE500Params
> > *params, MemoryRegion *ccsr,
> > mpic = g_new(qemu_irq, 256);
> >
> > if (kvm_enabled()) {
> > - QemuOpts *machine_opts = qemu_get_machine_opts();
> > - bool irqchip_allowed = qemu_opt_get_bool(machine_opts,
> > - "kernel_irqchip", true);
> > - bool irqchip_required = qemu_opt_get_bool(machine_opts,
> > - "kernel_irqchip", false);
> > + bool irqchip_allowed = true;
> > + bool irqchip_required = false;
> > + if (object_property_is_set(OBJECT(current_machine),
> > + "kernel_irqchip", &error_abort)) {
> > + irqchip_allowed =
> > object_property_get_bool(OBJECT(current_machine),
> > + "kernel_irqchip",
> > &error_abort);
> > + irqchip_required = irqchip_allowed;
> > + }
>
> Alternatively, you could have three properties: kernel_irqchip,
> write-only (no getter); kernel-irqchip-allowed; kernel-irqchip-required.
> Setting irqchip writes to both kernel-irqchip-allowed and
> kernel-irqchip-required. kernel-irqchip-allowed and
> kernel-irqchip-required are initialized to true and false respectively
> in the instance_init.
The functionality will not be affected if I use your idea,
both implementations are not "clean", but at least yours does not require
an extra QOM method.
Thanks,
Marcel
>
>
> > diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> > index bdb057e..8651167 100644
> > --- a/hw/ppc/virtex_ml507.c
> > +++ b/hw/ppc/virtex_ml507.c
> > @@ -145,7 +145,8 @@ static int xilinx_load_device_tree(hwaddr addr,
> > int r;
> > const char *dtb_filename;
> >
> > - dtb_filename = qemu_opt_get(qemu_get_machine_opts(), "dtb");
> > + dtb_filename = object_property_get_str(OBJECT(current_machine),
> > + "dtb", &error_abort);
> > if (dtb_filename) {
> > fdt = load_device_tree(dtb_filename, &fdt_size);
> > if (!fdt) {
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 2ca9143..e483c3c 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -22,7 +22,7 @@
> >
> > #include "qemu-common.h"
> > #include "qemu/atomic.h"
> > -#include "qemu/option.h"
> > +#include "hw/boards.h"
> > #include "qemu/config-file.h"
> > #include "sysemu/sysemu.h"
> > #include "hw/hw.h"
> > @@ -1292,8 +1292,15 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s,
> > EventNotifier *n, int virq)
> > static int kvm_irqchip_create(KVMState *s)
> > {
> > int ret;
> > + bool irqchip_allowed = true;
> >
> > - if (!qemu_opt_get_bool(qemu_get_machine_opts(), "kernel_irqchip",
> > true) ||
> > + if (object_property_is_set(OBJECT(current_machine),
> > + "kernel_irqchip", &error_abort)) {
> > + irqchip_allowed = object_property_get_bool(OBJECT(current_machine),
> > + "kernel_irqchip",
> > &error_abort);
> > + }
> > +
> > + if (!irqchip_allowed ||
> > !kvm_check_extension(s, KVM_CAP_IRQCHIP)) {
> > return 0;
> > }
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index e555040..ca7e6bc 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -33,6 +33,7 @@
> > #include "exec/ioport.h"
> > #include <asm/hyperv.h>
> > #include "hw/pci/pci.h"
> > +#include "hw/boards.h"
> >
> > //#define DEBUG_KVM
> >
> > @@ -853,8 +854,13 @@ int kvm_arch_init(KVMState *s)
> > }
> > qemu_register_reset(kvm_unpoison_all, NULL);
> >
> > - shadow_mem = qemu_opt_get_size(qemu_get_machine_opts(),
> > - "kvm_shadow_mem", -1);
> > + shadow_mem = -1;
> > + if (object_property_is_set(OBJECT(current_machine),
> > + "kvm_shadow_mem", &error_abort)) {
> > + shadow_mem = object_property_get_int(OBJECT(current_machine),
> > + "kvm_shadow_mem",
> > &error_abort);
> > + }
>
> This can also be simply initialized in the instance_init function.
Sure, thanks
>
> Paolo
>
> > +
> > if (shadow_mem != -1) {
> > shadow_mem /= 4096;
> > ret = kvm_vm_ioctl(s, KVM_SET_NR_MMU_PAGES, shadow_mem);
> > diff --git a/vl.c b/vl.c
> > index dc206e1..007342c 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2624,7 +2624,8 @@ static int configure_accelerator(void)
> > bool accel_initialised = false;
> > bool init_failed = false;
> >
> > - p = qemu_opt_get(qemu_get_machine_opts(), "accel");
> > + p = object_property_get_str(OBJECT(current_machine),
> > + "accel", &error_abort);
> > if (p == NULL) {
> > /* Use the default "accelerator", tcg */
> > p = "tcg";
> > @@ -4077,6 +4078,12 @@ int main(int argc, char **argv, char **envp)
> > exit(0);
> > }
> >
> > + machine_opts = qemu_get_machine_opts();
> > + if (qemu_opt_foreach(machine_opts, object_set_property,
> > current_machine, 1) < 0) {
> > + object_unref(OBJECT(current_machine));
> > + exit(1);
> > + }
> > +
> > configure_accelerator();
> >
> > if (qtest_chrdev) {
> > @@ -4089,12 +4096,14 @@ int main(int argc, char **argv, char **envp)
> > }
> > }
> >
> > - machine_opts = qemu_get_machine_opts();
> > - kernel_filename = qemu_opt_get(machine_opts, "kernel");
> > - initrd_filename = qemu_opt_get(machine_opts, "initrd");
> > - kernel_cmdline = qemu_opt_get(machine_opts, "append");
> > - bios_name = qemu_opt_get(machine_opts, "firmware");
> > -
> > + kernel_filename = object_property_get_str(OBJECT(current_machine),
> > + "kernel", &error_abort);
> > + initrd_filename = object_property_get_str(OBJECT(current_machine),
> > + "initrd", &error_abort);
> > + kernel_cmdline = object_property_get_str(OBJECT(current_machine),
> > + "append", &error_abort);
> > + bios_name = object_property_get_str(OBJECT(current_machine),
> > + "firmware", &error_abort);
> > boot_order = machine->default_boot_order;
> > opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
> > if (opts) {
> > @@ -4135,7 +4144,8 @@ int main(int argc, char **argv, char **envp)
> > exit(1);
> > }
> >
> > - if (!linux_boot && qemu_opt_get(machine_opts, "dtb")) {
> > + if (!linux_boot && object_property_get_str(OBJECT(current_machine),
> > + "dtb", &error_abort)) {
> > fprintf(stderr, "-dtb only allowed with -kernel option\n");
> > exit(1);
> > }
> >
>
