Blurb from previous question [1]: "hw/boards.h" declare current_machine, and vl.c defines it: current_machine = MACHINE(object_new_with_class(OBJECT_CLASS(machine_class))); object_property_add_child(object_get_root(), "machine", OBJECT(current_machine), &error_abort); The bigger user of 'current_machine' is the accel/KVM code. Recently in a0628599f..cc7d44c2e0 "Replace global smp variables with machine smp properties" we started to use MACHINE(qdev_get_machine()). Following a0628599f..cc7d44c2e0, a5e0b33119 use 'current_machine' again. qdev_get_machine() resolves the machine in the QOM composition tree. Paolo answered [2]: > I would always use MACHINE(qdev_get_machine()), espeecially outside > vl.c. Ideally, current_machine would be static within vl.c or even > unused outside the object_property_add_child() that you quote above. Let's remove the global current_machine. I am still confused by this comment: /* qdev_get_machine() can return something that's not TYPE_MACHINE * if this is one of the user-only emulators; in that case there's * no need to check the ignore_memory_transaction_failures board flag. */ [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg669475.html [2] https://www.mail-archive.com/qemu-devel@nongnu.org/msg669493.html Philippe Mathieu-Daudé (15): target/arm/kvm: Use CPUState::kvm_state in kvm_arm_pmu_supported() hw/ppc/spapr_rtas: Use local MachineState variable hw/ppc/spapr_rtas: Access MachineState via SpaprMachineState argument hw/ppc/spapr_rtas: Restrict variables scope to single switch case device-hotplug: Replace current_machine by qdev_get_machine() migration/savevm: Replace current_machine by qdev_get_machine() hw/core/machine-qmp-cmds: Replace current_machine by qdev_get_machine() target/arm/monitor: Replace current_machine by qdev_get_machine() device_tree: Replace current_machine by qdev_get_machine() memory: Replace current_machine by qdev_get_machine() exec: Replace current_machine by qdev_get_machine() accel: Introduce the current_accel() method accel: Replace current_machine->accelerator by current_accel() method accel/accel: Replace current_machine by qdev_get_machine() vl: Make current_machine a local variable include/hw/boards.h | 2 -- include/sysemu/accel.h | 2 ++ accel/accel.c | 7 +++++++ accel/kvm/kvm-all.c | 4 ++-- accel/tcg/tcg-all.c | 2 +- device-hotplug.c | 2 +- device_tree.c | 4 +++- exec.c | 10 ++++++---- hw/core/machine-qmp-cmds.c | 4 ++-- hw/ppc/spapr_rtas.c | 6 +++--- memory.c | 6 ++++-- migration/savevm.c | 10 +++++----- target/arm/kvm.c | 4 +--- target/arm/kvm64.c | 4 ++-- target/arm/monitor.c | 3 ++- target/i386/kvm.c | 2 +- target/ppc/kvm.c | 2 +- vl.c | 6 +++--- 18 files changed, 46 insertions(+), 34 deletions(-) -- 2.21.1
KVMState is already accessible via CPUState::kvm_state, use it. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- target/arm/kvm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/target/arm/kvm.c b/target/arm/kvm.c index b87b59a02a..8d82889150 100644 --- a/target/arm/kvm.c +++ b/target/arm/kvm.c @@ -181,9 +181,7 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu) bool kvm_arm_pmu_supported(CPUState *cpu) { - KVMState *s = KVM_STATE(current_machine->accelerator); - - return kvm_check_extension(s, KVM_CAP_ARM_PMU_V3); + return kvm_check_extension(cpu->kvm_state, KVM_CAP_ARM_PMU_V3); } int kvm_arm_get_max_vm_ipa_size(MachineState *ms) -- 2.21.1
Since we have the MachineState already available locally, ues it instead of the global current_machine. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/ppc/spapr_rtas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index 8d8d8cdfcb..e88bb1930e 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -281,7 +281,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, "DesProcs=%d," "MaxPlatProcs=%d", max_cpus, - current_machine->ram_size / MiB, + ms->ram_size / MiB, ms->smp.cpus, max_cpus); if (pcc->n_host_threads > 0) { -- 2.21.1
We received a SpaprMachineState argument. Since SpaprMachineState inherits of MachineState, use it instead of calling qdev_get_machine. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/ppc/spapr_rtas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index e88bb1930e..6f06e9d7fe 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -267,7 +267,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, uint32_t nret, target_ulong rets) { PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); - MachineState *ms = MACHINE(qdev_get_machine()); + MachineState *ms = MACHINE(spapr); unsigned int max_cpus = ms->smp.max_cpus; target_ulong parameter = rtas_ld(args, 0); target_ulong buffer = rtas_ld(args, 1); -- 2.21.1
We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS case, restrict their scope to avoid unnecessary initialization. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/ppc/spapr_rtas.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index 6f06e9d7fe..7237e5ebf2 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, uint32_t nret, target_ulong rets) { PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); - MachineState *ms = MACHINE(spapr); - unsigned int max_cpus = ms->smp.max_cpus; target_ulong parameter = rtas_ld(args, 0); target_ulong buffer = rtas_ld(args, 1); target_ulong length = rtas_ld(args, 2); @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, switch (parameter) { case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: { + MachineState *ms = MACHINE(spapr); + unsigned int max_cpus = ms->smp.max_cpus; char *param_val = g_strdup_printf("MaxEntCap=%d," "DesMem=%" PRIu64 "," "DesProcs=%d," -- 2.21.1
As we want to remove the global current_machine, replace MACHINE_GET_CLASS(current_machine) by MACHINE_GET_CLASS(qdev_get_machine()). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- device-hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/device-hotplug.c b/device-hotplug.c index f01d53774b..44d687f254 100644 --- a/device-hotplug.c +++ b/device-hotplug.c @@ -45,7 +45,7 @@ static DriveInfo *add_init_drive(const char *optstr) if (!opts) return NULL; - mc = MACHINE_GET_CLASS(current_machine); + mc = MACHINE_GET_CLASS(qdev_get_machine()); dinfo = drive_new(opts, mc->block_default_type, &err); if (err) { error_report_err(err); -- 2.21.1
As we want to remove the global current_machine, replace MACHINE_GET_CLASS(current_machine) by MACHINE_GET_CLASS(qdev_get_machine()). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- migration/savevm.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/migration/savevm.c b/migration/savevm.c index 59efc1981d..0e8b6a4715 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -292,7 +292,8 @@ static uint32_t get_validatable_capabilities_count(void) static int configuration_pre_save(void *opaque) { SaveState *state = opaque; - const char *current_name = MACHINE_GET_CLASS(current_machine)->name; + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); + const char *current_name = mc->name; MigrationState *s = migrate_get_current(); int i, j; @@ -362,7 +363,8 @@ static bool configuration_validate_capabilities(SaveState *state) static int configuration_post_load(void *opaque, int version_id) { SaveState *state = opaque; - const char *current_name = MACHINE_GET_CLASS(current_machine)->name; + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); + const char *current_name = mc->name; if (strncmp(state->name, current_name, state->len) != 0) { error_report("Machine type received is '%.*s' and local is '%s'", @@ -615,9 +617,7 @@ static void dump_vmstate_vmsd(FILE *out_file, static void dump_machine_type(FILE *out_file) { - MachineClass *mc; - - mc = MACHINE_GET_CLASS(current_machine); + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); fprintf(out_file, " \"vmschkmachine\": {\n"); fprintf(out_file, " \"Name\": \"%s\"\n", mc->name); -- 2.21.1
As we want to remove the global current_machine, replace MACHINE_GET_CLASS(current_machine) by MACHINE_GET_CLASS(qdev_get_machine()). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/core/machine-qmp-cmds.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c index eed5aeb2f7..5a04d00e4f 100644 --- a/hw/core/machine-qmp-cmds.c +++ b/hw/core/machine-qmp-cmds.c @@ -280,9 +280,9 @@ void qmp_cpu_add(int64_t id, Error **errp) { MachineClass *mc; - mc = MACHINE_GET_CLASS(current_machine); + mc = MACHINE_GET_CLASS(qdev_get_machine()); if (mc->hot_add_cpu) { - mc->hot_add_cpu(current_machine, id, errp); + mc->hot_add_cpu(MACHINE(qdev_get_machine()), id, errp); } else { error_setg(errp, "Not supported"); } -- 2.21.1
As we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- target/arm/monitor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/target/arm/monitor.c b/target/arm/monitor.c index fa054f8a36..bcbf69802d 100644 --- a/target/arm/monitor.c +++ b/target/arm/monitor.c @@ -136,7 +136,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, } if (kvm_enabled()) { - const char *cpu_type = current_machine->cpu_type; + MachineState *ms = MACHINE(qdev_get_machine()); + const char *cpu_type = ms->cpu_type; int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); bool supported = false; -- 2.21.1
As we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- device_tree.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/device_tree.c b/device_tree.c index f8b46b3c73..665ea2f586 100644 --- a/device_tree.c +++ b/device_tree.c @@ -466,7 +466,9 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt) * which phandle id to start allocating phandles. */ if (!phandle) { - phandle = machine_phandle_start(current_machine); + MachineState *ms = MACHINE(qdev_get_machine()); + + phandle = machine_phandle_start(ms); } if (!phandle) { -- 2.21.1
As we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- memory.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/memory.c b/memory.c index d7b9bb6951..57e38b1f50 100644 --- a/memory.c +++ b/memory.c @@ -3004,6 +3004,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, int n = view->nr; int i; AddressSpace *as; + MachineState *ms; qemu_printf("FlatView #%d\n", fvi->counter); ++fvi->counter; @@ -3026,6 +3027,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, return; } + ms = MACHINE(qdev_get_machine()); while (n--) { mr = range->mr; if (range->offset_in_region) { @@ -3057,7 +3059,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, if (fvi->ac) { for (i = 0; i < fv_address_spaces->len; ++i) { as = g_array_index(fv_address_spaces, AddressSpace*, i); - if (fvi->ac->has_memory(current_machine, as, + if (fvi->ac->has_memory(ms, as, int128_get64(range->addr.start), MR_SIZE(range->addr.size) + 1)) { qemu_printf(" %s", fvi->ac->name); -- 2.21.1
As we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- exec.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/exec.c b/exec.c index d4b769d0d4..98f5b049ca 100644 --- a/exec.c +++ b/exec.c @@ -1984,11 +1984,11 @@ static unsigned long last_ram_page(void) static void qemu_ram_setup_dump(void *addr, ram_addr_t size) { - int ret; + MachineState *ms = MACHINE(qdev_get_machine()); /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */ - if (!machine_dump_guest_core(current_machine)) { - ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP); + if (!machine_dump_guest_core(ms)) { + int ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP); if (ret) { perror("qemu_madvise"); fprintf(stderr, "madvise doesn't support MADV_DONTDUMP, " @@ -2108,7 +2108,9 @@ size_t qemu_ram_pagesize_largest(void) static int memory_try_enable_merging(void *addr, size_t len) { - if (!machine_mem_merge(current_machine)) { + MachineState *ms = MACHINE(qdev_get_machine()); + + if (!machine_mem_merge(ms)) { /* disabled by the user */ return 0; } -- 2.21.1
We want to remove the global current_machine. The accel/ code access few times current_machine->accelerator. Introduce the current_accel() method first, it will then be easier to replace 'current_machine' by MACHINE(qdev_get_machine()). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/sysemu/accel.h | 2 ++ accel/accel.c | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h index d4c1429711..47e5788530 100644 --- a/include/sysemu/accel.h +++ b/include/sysemu/accel.h @@ -70,4 +70,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms); /* Called just before os_setup_post (ie just before drop OS privs) */ void accel_setup_post(MachineState *ms); +AccelState *current_accel(void); + #endif diff --git a/accel/accel.c b/accel/accel.c index 1c5c3a6abb..cb555e3b06 100644 --- a/accel/accel.c +++ b/accel/accel.c @@ -63,6 +63,11 @@ int accel_init_machine(AccelState *accel, MachineState *ms) return ret; } +AccelState *current_accel(void) +{ + return current_machine->accelerator; +} + void accel_setup_post(MachineState *ms) { AccelState *accel = ms->accelerator; -- 2.21.1
As we want to remove the global current_machine, replace 'current_machine->accelerator' by current_accel(). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- accel/kvm/kvm-all.c | 4 ++-- accel/tcg/tcg-all.c | 2 +- memory.c | 2 +- target/arm/kvm64.c | 4 ++-- target/i386/kvm.c | 2 +- target/ppc/kvm.c | 2 +- vl.c | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index b2f1a5bcb5..be1980f250 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -164,7 +164,7 @@ static NotifierList kvm_irqchip_change_notifiers = int kvm_get_max_memslots(void) { - KVMState *s = KVM_STATE(current_machine->accelerator); + KVMState *s = KVM_STATE(current_accel()); return s->nr_slots; } @@ -1847,7 +1847,7 @@ static int kvm_max_vcpu_id(KVMState *s) bool kvm_vcpu_id_is_valid(int vcpu_id) { - KVMState *s = KVM_STATE(current_machine->accelerator); + KVMState *s = KVM_STATE(current_accel()); return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s); } diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c index 1dc384c8d2..1802ce02f6 100644 --- a/accel/tcg/tcg-all.c +++ b/accel/tcg/tcg-all.c @@ -124,7 +124,7 @@ static void tcg_accel_instance_init(Object *obj) static int tcg_init(MachineState *ms) { - TCGState *s = TCG_STATE(current_machine->accelerator); + TCGState *s = TCG_STATE(current_accel()); tcg_exec_init(s->tb_size * 1024 * 1024); cpu_interrupt_handler = tcg_handle_interrupt; diff --git a/memory.c b/memory.c index 57e38b1f50..60e8993499 100644 --- a/memory.c +++ b/memory.c @@ -3106,7 +3106,7 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner) }; GArray *fv_address_spaces; GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal); - AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator); + AccelClass *ac = ACCEL_GET_CLASS(current_accel()); if (ac->has_memory) { fvi.ac = ac; diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index 876184b8fe..f677877a1e 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -613,14 +613,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) bool kvm_arm_aarch32_supported(CPUState *cpu) { - KVMState *s = KVM_STATE(current_machine->accelerator); + KVMState *s = KVM_STATE(current_accel()); return kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT); } bool kvm_arm_sve_supported(CPUState *cpu) { - KVMState *s = KVM_STATE(current_machine->accelerator); + KVMState *s = KVM_STATE(current_accel()); return kvm_check_extension(s, KVM_CAP_ARM_SVE); } diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 0b511906e3..2ed15814dc 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -147,7 +147,7 @@ bool kvm_allows_irq0_override(void) static bool kvm_x2apic_api_set_flags(uint64_t flags) { - KVMState *s = KVM_STATE(current_machine->accelerator); + KVMState *s = KVM_STATE(current_accel()); return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags); } diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c index d1c334f0e3..2d011308e0 100644 --- a/target/ppc/kvm.c +++ b/target/ppc/kvm.c @@ -258,7 +258,7 @@ static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp) struct ppc_radix_page_info *kvm_get_radix_page_info(void) { - KVMState *s = KVM_STATE(current_machine->accelerator); + KVMState *s = KVM_STATE(current_accel()); struct ppc_radix_page_info *radix_page_info; struct kvm_ppc_rmmu_info rmmu_info; int i; diff --git a/vl.c b/vl.c index 86474a55c9..3ff3548183 100644 --- a/vl.c +++ b/vl.c @@ -2804,7 +2804,7 @@ static void configure_accelerators(const char *progname) } if (init_failed) { - AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator); + AccelClass *ac = ACCEL_GET_CLASS(current_accel()); error_report("falling back to %s", ac->name); } -- 2.21.1
As we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- accel/accel.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/accel/accel.c b/accel/accel.c index cb555e3b06..777d6ba119 100644 --- a/accel/accel.c +++ b/accel/accel.c @@ -65,7 +65,9 @@ int accel_init_machine(AccelState *accel, MachineState *ms) AccelState *current_accel(void) { - return current_machine->accelerator; + MachineState *ms = MACHINE(qdev_get_machine()); + + return ms->accelerator; } void accel_setup_post(MachineState *ms) -- 2.21.1
Since we now only use current_machine in vl.c, stop exporting it as a global variable in "hw/board.h", and make it static to vl.c. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/hw/boards.h | 2 -- vl.c | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/include/hw/boards.h b/include/hw/boards.h index 61f8bb8e5a..b0c0d4376d 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -59,8 +59,6 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner, #define MACHINE_CLASS(klass) \ OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE) -extern MachineState *current_machine; - void machine_run_board_init(MachineState *machine); bool machine_usb(MachineState *machine); int machine_phandle_start(MachineState *machine); diff --git a/vl.c b/vl.c index 3ff3548183..7a69af4bef 100644 --- a/vl.c +++ b/vl.c @@ -214,6 +214,8 @@ static int default_sdcard = 1; static int default_vga = 1; static int default_net = 1; +static MachineState *current_machine; + static struct { const char *driver; int *flag; @@ -1164,8 +1166,6 @@ static int usb_parse(const char *cmdline) /***********************************************************/ /* machine registration */ -MachineState *current_machine; - static MachineClass *find_machine(const char *name, GSList *machines) { GSList *el; -- 2.21.1
* Philippe Mathieu-Daudé (philmd@redhat.com) wrote: > As we want to remove the global current_machine, > replace MACHINE_GET_CLASS(current_machine) by > MACHINE_GET_CLASS(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > migration/savevm.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index 59efc1981d..0e8b6a4715 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -292,7 +292,8 @@ static uint32_t get_validatable_capabilities_count(void) > static int configuration_pre_save(void *opaque) > { > SaveState *state = opaque; > - const char *current_name = MACHINE_GET_CLASS(current_machine)->name; > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > + const char *current_name = mc->name; For migration: Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com> (Personally I'd keep it on one line, but that's just taste) > MigrationState *s = migrate_get_current(); > int i, j; > > @@ -362,7 +363,8 @@ static bool configuration_validate_capabilities(SaveState *state) > static int configuration_post_load(void *opaque, int version_id) > { > SaveState *state = opaque; > - const char *current_name = MACHINE_GET_CLASS(current_machine)->name; > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > + const char *current_name = mc->name; > > if (strncmp(state->name, current_name, state->len) != 0) { > error_report("Machine type received is '%.*s' and local is '%s'", > @@ -615,9 +617,7 @@ static void dump_vmstate_vmsd(FILE *out_file, > > static void dump_machine_type(FILE *out_file) > { > - MachineClass *mc; > - > - mc = MACHINE_GET_CLASS(current_machine); > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > > fprintf(out_file, " \"vmschkmachine\": {\n"); > fprintf(out_file, " \"Name\": \"%s\"\n", mc->name); > -- > 2.21.1 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Thu, 9 Jan 2020 16:21:21 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > We received a SpaprMachineState argument. Since SpaprMachineState > inherits of MachineState, use it instead of calling qdev_get_machine. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr_rtas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index e88bb1930e..6f06e9d7fe 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -267,7 +267,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > uint32_t nret, target_ulong rets) > { > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > - MachineState *ms = MACHINE(qdev_get_machine()); > + MachineState *ms = MACHINE(spapr); > unsigned int max_cpus = ms->smp.max_cpus; > target_ulong parameter = rtas_ld(args, 0); > target_ulong buffer = rtas_ld(args, 1);
On Thu, 9 Jan 2020 16:21:20 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Since we have the MachineState already available locally, > ues it instead of the global current_machine. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > hw/ppc/spapr_rtas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 8d8d8cdfcb..e88bb1930e 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -281,7 +281,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > "DesProcs=%d," > "MaxPlatProcs=%d", > max_cpus, > - current_machine->ram_size / MiB, > + ms->ram_size / MiB, > ms->smp.cpus, > max_cpus); > if (pcc->n_host_threads > 0) {
On Thu, 9 Jan 2020 16:21:22 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS > case, restrict their scope to avoid unnecessary initialization. > I guess a decent compiler can be smart enough detect that the initialization isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch... Anyway, reducing scope isn't bad. The only hitch I could see is that some people do prefer to have all variables declared upfront, but there's a nested param_val variable already so I guess it's okay. > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/ppc/spapr_rtas.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 6f06e9d7fe..7237e5ebf2 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > uint32_t nret, target_ulong rets) > { > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > - MachineState *ms = MACHINE(spapr); > - unsigned int max_cpus = ms->smp.max_cpus; > target_ulong parameter = rtas_ld(args, 0); > target_ulong buffer = rtas_ld(args, 1); > target_ulong length = rtas_ld(args, 2); > @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > > switch (parameter) { > case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: { > + MachineState *ms = MACHINE(spapr); > + unsigned int max_cpus = ms->smp.max_cpus; The max_cpus variable used to be a global. Now that it got moved below ms->smp, I'm not sure it's worth keeping it IMHO. What about dropping it completely and do: char *param_val = g_strdup_printf("MaxEntCap=%d," "DesMem=%" PRIu64 "," "DesProcs=%d," "MaxPlatProcs=%d", ms->smp.max_cpus, current_machine->ram_size / MiB, ms->smp.cpus, ms->smp.max_cpus); And maybe insert an empty line between the declaration of param_val and the code for a better readability ? > char *param_val = g_strdup_printf("MaxEntCap=%d," > "DesMem=%" PRIu64 "," > "DesProcs=%d,"
On 1/9/20 6:43 PM, Greg Kurz wrote: > On Thu, 9 Jan 2020 16:21:22 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS >> case, restrict their scope to avoid unnecessary initialization. >> > > I guess a decent compiler can be smart enough detect that the initialization > isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch... > Anyway, reducing scope isn't bad. The only hitch I could see is that some > people do prefer to have all variables declared upfront, but there's a nested > param_val variable already so I guess it's okay. I don't want to outsmart compilers :) The MACHINE() macro is not a simple cast, it does object introspection with OBJECT_CHECK(), thus is not free. Since object_dynamic_cast_assert() argument is not const, I'm not sure the compiler can remove the call. Richard, Eric, do you know? >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/ppc/spapr_rtas.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >> index 6f06e9d7fe..7237e5ebf2 100644 >> --- a/hw/ppc/spapr_rtas.c >> +++ b/hw/ppc/spapr_rtas.c >> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, >> uint32_t nret, target_ulong rets) >> { >> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); >> - MachineState *ms = MACHINE(spapr); >> - unsigned int max_cpus = ms->smp.max_cpus; >> target_ulong parameter = rtas_ld(args, 0); >> target_ulong buffer = rtas_ld(args, 1); >> target_ulong length = rtas_ld(args, 2); >> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, >> >> switch (parameter) { >> case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: { >> + MachineState *ms = MACHINE(spapr); >> + unsigned int max_cpus = ms->smp.max_cpus; > > The max_cpus variable used to be a global. Now that it got moved > below ms->smp, I'm not sure it's worth keeping it IMHO. What about > dropping it completely and do: > > char *param_val = g_strdup_printf("MaxEntCap=%d," > "DesMem=%" PRIu64 "," > "DesProcs=%d," > "MaxPlatProcs=%d", > ms->smp.max_cpus, > current_machine->ram_size / MiB, > ms->smp.cpus, > ms->smp.max_cpus); OK, good idea. > And maybe insert an empty line between the declaration of param_val > and the code for a better readability ? > >> char *param_val = g_strdup_printf("MaxEntCap=%d," >> "DesMem=%" PRIu64 "," >> "DesProcs=%d," >
On Fri, 10 Jan 2020 10:34:07 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 1/9/20 6:43 PM, Greg Kurz wrote: > > On Thu, 9 Jan 2020 16:21:22 +0100 > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > >> We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS > >> case, restrict their scope to avoid unnecessary initialization. > >> > > > > I guess a decent compiler can be smart enough detect that the initialization > > isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch... > > Anyway, reducing scope isn't bad. The only hitch I could see is that some > > people do prefer to have all variables declared upfront, but there's a nested > > param_val variable already so I guess it's okay. > > I don't want to outsmart compilers :) > > The MACHINE() macro is not a simple cast, it does object introspection > with OBJECT_CHECK(), thus is not free. Since Sure, I understand the motivation in avoiding an unneeded call to calling object_dynamic_cast_assert(). > object_dynamic_cast_assert() argument is not const, I'm not sure the > compiler can remove the call. > Not remove the call, but delay it to the branch that uses it, ie. parameter == RTAS_SYSPARM_SPLPAR_CHARACTERISTICS. > Richard, Eric, do you know? > > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> hw/ppc/spapr_rtas.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > >> index 6f06e9d7fe..7237e5ebf2 100644 > >> --- a/hw/ppc/spapr_rtas.c > >> +++ b/hw/ppc/spapr_rtas.c > >> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > >> uint32_t nret, target_ulong rets) > >> { > >> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > >> - MachineState *ms = MACHINE(spapr); > >> - unsigned int max_cpus = ms->smp.max_cpus; > >> target_ulong parameter = rtas_ld(args, 0); > >> target_ulong buffer = rtas_ld(args, 1); > >> target_ulong length = rtas_ld(args, 2); > >> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > >> > >> switch (parameter) { > >> case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: { > >> + MachineState *ms = MACHINE(spapr); > >> + unsigned int max_cpus = ms->smp.max_cpus; > > > > The max_cpus variable used to be a global. Now that it got moved > > below ms->smp, I'm not sure it's worth keeping it IMHO. What about > > dropping it completely and do: > > > > char *param_val = g_strdup_printf("MaxEntCap=%d," > > "DesMem=%" PRIu64 "," > > "DesProcs=%d," > > "MaxPlatProcs=%d", > > ms->smp.max_cpus, > > current_machine->ram_size / MiB, > > ms->smp.cpus, > > ms->smp.max_cpus); > > OK, good idea. > > > And maybe insert an empty line between the declaration of param_val > > and the code for a better readability ? > > > >> char *param_val = g_strdup_printf("MaxEntCap=%d," > >> "DesMem=%" PRIu64 "," > >> "DesProcs=%d," > > >
On Thu, Jan 9, 2020 at 11:22 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > KVMState is already accessible via CPUState::kvm_state, use it. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/arm/kvm.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/target/arm/kvm.c b/target/arm/kvm.c > index b87b59a02a..8d82889150 100644 > --- a/target/arm/kvm.c > +++ b/target/arm/kvm.c > @@ -181,9 +181,7 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu) > > bool kvm_arm_pmu_supported(CPUState *cpu) > { > - KVMState *s = KVM_STATE(current_machine->accelerator); > - > - return kvm_check_extension(s, KVM_CAP_ARM_PMU_V3); > + return kvm_check_extension(cpu->kvm_state, KVM_CAP_ARM_PMU_V3); > } > > int kvm_arm_get_max_vm_ipa_size(MachineState *ms) > -- > 2.21.1 > >
On 1/10/20 3:50 AM, Greg Kurz wrote: >>> I guess a decent compiler can be smart enough detect that the initialization >>> isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch... >>> Anyway, reducing scope isn't bad. The only hitch I could see is that some >>> people do prefer to have all variables declared upfront, but there's a nested >>> param_val variable already so I guess it's okay. >> >> I don't want to outsmart compilers :) Or conversely play the game of which compilers will warn about an atypical construct. >> >> The MACHINE() macro is not a simple cast, it does object introspection >> with OBJECT_CHECK(), thus is not free. Since > > Sure, I understand the motivation in avoiding an unneeded call > to calling object_dynamic_cast_assert(). > >> object_dynamic_cast_assert() argument is not const, I'm not sure the >> compiler can remove the call. >> > > Not remove the call, but delay it to the branch that uses it, > ie. parameter == RTAS_SYSPARM_SPLPAR_CHARACTERISTICS. > >> Richard, Eric, do you know? >> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> --- >>>> hw/ppc/spapr_rtas.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >>>> index 6f06e9d7fe..7237e5ebf2 100644 >>>> --- a/hw/ppc/spapr_rtas.c >>>> +++ b/hw/ppc/spapr_rtas.c >>>> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, >>>> uint32_t nret, target_ulong rets) >>>> { >>>> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); >>>> - MachineState *ms = MACHINE(spapr); >>>> - unsigned int max_cpus = ms->smp.max_cpus; >>>> target_ulong parameter = rtas_ld(args, 0); >>>> target_ulong buffer = rtas_ld(args, 1); >>>> target_ulong length = rtas_ld(args, 2); >>>> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, >>>> >>>> switch (parameter) { >>>> case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: { >>>> + MachineState *ms = MACHINE(spapr); >>>> + unsigned int max_cpus = ms->smp.max_cpus; Declaring an initializer inside a switch statement can trigger warnings under some compilation scenarios (particularly if the variable is referenced outside of the scope where it was introduced). But here, you are using 'case label: { ...' to create a scope, which presumably ends before the next case label, and is thus not going to trigger compiler warnings. An alternative is indeed leaving the declaration up front but deferring the (possibly expensive) initializer to the case label that needs it: MachineState *ms; switch (parameter) { case ...: ms = MACHINE(spapr); and done that way, you might not even need the extra {} behind the case label (I didn't read the file to see if there is already some other reason for having introduced that sub-scope). As for whether compilers are smart enough to defer non-trivial initialization to the one case label that uses the variable, I wouldn't count on it. If the non-trivial initialization includes a function call (which the MACHINE() macro does), it's much harder to prove whether that function call has side effects that may be needed prior to the switch statement. So limiting the scope of the initialization (whether by dropping the declaration, or just deferring the call) DOES make sense. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Jan 9, 2020 at 11:27 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > As we want to remove the global current_machine, > replace MACHINE_GET_CLASS(current_machine) by > MACHINE_GET_CLASS(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > device-hotplug.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/device-hotplug.c b/device-hotplug.c > index f01d53774b..44d687f254 100644 > --- a/device-hotplug.c > +++ b/device-hotplug.c > @@ -45,7 +45,7 @@ static DriveInfo *add_init_drive(const char *optstr) > if (!opts) > return NULL; > > - mc = MACHINE_GET_CLASS(current_machine); > + mc = MACHINE_GET_CLASS(qdev_get_machine()); > dinfo = drive_new(opts, mc->block_default_type, &err); > if (err) { > error_report_err(err); > -- > 2.21.1 > >
On Thu, Jan 9, 2020 at 11:30 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > As we want to remove the global current_machine, > replace MACHINE_GET_CLASS(current_machine) by > MACHINE_GET_CLASS(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > migration/savevm.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/migration/savevm.c b/migration/savevm.c > index 59efc1981d..0e8b6a4715 100644 > --- a/migration/savevm.c > +++ b/migration/savevm.c > @@ -292,7 +292,8 @@ static uint32_t get_validatable_capabilities_count(void) > static int configuration_pre_save(void *opaque) > { > SaveState *state = opaque; > - const char *current_name = MACHINE_GET_CLASS(current_machine)->name; > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > + const char *current_name = mc->name; > MigrationState *s = migrate_get_current(); > int i, j; > > @@ -362,7 +363,8 @@ static bool configuration_validate_capabilities(SaveState *state) > static int configuration_post_load(void *opaque, int version_id) > { > SaveState *state = opaque; > - const char *current_name = MACHINE_GET_CLASS(current_machine)->name; > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > + const char *current_name = mc->name; > > if (strncmp(state->name, current_name, state->len) != 0) { > error_report("Machine type received is '%.*s' and local is '%s'", > @@ -615,9 +617,7 @@ static void dump_vmstate_vmsd(FILE *out_file, > > static void dump_machine_type(FILE *out_file) > { > - MachineClass *mc; > - > - mc = MACHINE_GET_CLASS(current_machine); > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > > fprintf(out_file, " \"vmschkmachine\": {\n"); > fprintf(out_file, " \"Name\": \"%s\"\n", mc->name); > -- > 2.21.1 > >
On Thu, Jan 9, 2020 at 11:30 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > As we want to remove the global current_machine, > replace MACHINE_GET_CLASS(current_machine) by > MACHINE_GET_CLASS(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/core/machine-qmp-cmds.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c > index eed5aeb2f7..5a04d00e4f 100644 > --- a/hw/core/machine-qmp-cmds.c > +++ b/hw/core/machine-qmp-cmds.c > @@ -280,9 +280,9 @@ void qmp_cpu_add(int64_t id, Error **errp) > { > MachineClass *mc; > > - mc = MACHINE_GET_CLASS(current_machine); > + mc = MACHINE_GET_CLASS(qdev_get_machine()); > if (mc->hot_add_cpu) { > - mc->hot_add_cpu(current_machine, id, errp); > + mc->hot_add_cpu(MACHINE(qdev_get_machine()), id, errp); > } else { > error_setg(errp, "Not supported"); > } > -- > 2.21.1 > >
On Thu, Jan 9, 2020 at 11:23 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > As we want to remove the global current_machine, > replace 'current_machine' by MACHINE(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/arm/monitor.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/target/arm/monitor.c b/target/arm/monitor.c > index fa054f8a36..bcbf69802d 100644 > --- a/target/arm/monitor.c > +++ b/target/arm/monitor.c > @@ -136,7 +136,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type, > } > > if (kvm_enabled()) { > - const char *cpu_type = current_machine->cpu_type; > + MachineState *ms = MACHINE(qdev_get_machine()); > + const char *cpu_type = ms->cpu_type; > int len = strlen(cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX); > bool supported = false; > > -- > 2.21.1 > >
On Thu, Jan 9, 2020 at 11:34 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > As we want to remove the global current_machine, > replace 'current_machine' by MACHINE(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > device_tree.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/device_tree.c b/device_tree.c > index f8b46b3c73..665ea2f586 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -466,7 +466,9 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt) > * which phandle id to start allocating phandles. > */ > if (!phandle) { > - phandle = machine_phandle_start(current_machine); > + MachineState *ms = MACHINE(qdev_get_machine()); > + > + phandle = machine_phandle_start(ms); > } > > if (!phandle) { > -- > 2.21.1 > >
On Thu, Jan 9, 2020 at 11:29 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > As we want to remove the global current_machine, > replace 'current_machine' by MACHINE(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > memory.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/memory.c b/memory.c > index d7b9bb6951..57e38b1f50 100644 > --- a/memory.c > +++ b/memory.c > @@ -3004,6 +3004,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, > int n = view->nr; > int i; > AddressSpace *as; > + MachineState *ms; > > qemu_printf("FlatView #%d\n", fvi->counter); > ++fvi->counter; > @@ -3026,6 +3027,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, > return; > } > > + ms = MACHINE(qdev_get_machine()); Why not set this at the top? Alistair > while (n--) { > mr = range->mr; > if (range->offset_in_region) { > @@ -3057,7 +3059,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, > if (fvi->ac) { > for (i = 0; i < fv_address_spaces->len; ++i) { > as = g_array_index(fv_address_spaces, AddressSpace*, i); > - if (fvi->ac->has_memory(current_machine, as, > + if (fvi->ac->has_memory(ms, as, > int128_get64(range->addr.start), > MR_SIZE(range->addr.size) + 1)) { > qemu_printf(" %s", fvi->ac->name); > -- > 2.21.1 > >
On 1/12/20 10:48 AM, Alistair Francis wrote: > On Thu, Jan 9, 2020 at 11:29 PM Philippe Mathieu-Daudé > <philmd@redhat.com> wrote: >> >> As we want to remove the global current_machine, >> replace 'current_machine' by MACHINE(qdev_get_machine()). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> memory.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/memory.c b/memory.c >> index d7b9bb6951..57e38b1f50 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -3004,6 +3004,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, >> int n = view->nr; >> int i; >> AddressSpace *as; >> + MachineState *ms; >> >> qemu_printf("FlatView #%d\n", fvi->counter); >> ++fvi->counter; >> @@ -3026,6 +3027,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, >> return; >> } >> >> + ms = MACHINE(qdev_get_machine()); > > Why not set this at the top? Calling qdev_get_machine() is not free as it does some introspection checks. Since we can return earlier if there are no rendered FlatView, I placed the machinestate initialization just before it we need to access it. > Alistair > >> while (n--) { >> mr = range->mr; >> if (range->offset_in_region) { >> @@ -3057,7 +3059,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, >> if (fvi->ac) { >> for (i = 0; i < fv_address_spaces->len; ++i) { >> as = g_array_index(fv_address_spaces, AddressSpace*, i); >> - if (fvi->ac->has_memory(current_machine, as, >> + if (fvi->ac->has_memory(ms, as, >> int128_get64(range->addr.start), >> MR_SIZE(range->addr.size) + 1)) { >> qemu_printf(" %s", fvi->ac->name); >> -- >> 2.21.1 >> >> >
[-- Attachment #1: Type: text/plain, Size: 1318 bytes --] On Thu, Jan 09, 2020 at 04:21:20PM +0100, Philippe Mathieu-Daudé wrote: > Since we have the MachineState already available locally, > ues it instead of the global current_machine. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_rtas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 8d8d8cdfcb..e88bb1930e 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -281,7 +281,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > "DesProcs=%d," > "MaxPlatProcs=%d", > max_cpus, > - current_machine->ram_size / MiB, > + ms->ram_size / MiB, > ms->smp.cpus, > max_cpus); > if (pcc->n_host_threads > 0) { -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1256 bytes --] On Thu, Jan 09, 2020 at 04:21:21PM +0100, Philippe Mathieu-Daudé wrote: > We received a SpaprMachineState argument. Since SpaprMachineState > inherits of MachineState, use it instead of calling qdev_get_machine. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Acked-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_rtas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index e88bb1930e..6f06e9d7fe 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -267,7 +267,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, > uint32_t nret, target_ulong rets) > { > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > - MachineState *ms = MACHINE(qdev_get_machine()); > + MachineState *ms = MACHINE(spapr); > unsigned int max_cpus = ms->smp.max_cpus; > target_ulong parameter = rtas_ld(args, 0); > target_ulong buffer = rtas_ld(args, 1); -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1842 bytes --] On Fri, Jan 10, 2020 at 10:50:55AM +0100, Greg Kurz wrote: > On Fri, 10 Jan 2020 10:34:07 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > On 1/9/20 6:43 PM, Greg Kurz wrote: > > > On Thu, 9 Jan 2020 16:21:22 +0100 > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > > > >> We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS > > >> case, restrict their scope to avoid unnecessary initialization. > > >> > > > > > > I guess a decent compiler can be smart enough detect that the initialization > > > isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch... > > > Anyway, reducing scope isn't bad. The only hitch I could see is that some > > > people do prefer to have all variables declared upfront, but there's a nested > > > param_val variable already so I guess it's okay. > > > > I don't want to outsmart compilers :) > > > > The MACHINE() macro is not a simple cast, it does object introspection > > with OBJECT_CHECK(), thus is not free. Since > > Sure, I understand the motivation in avoiding an unneeded call > to calling object_dynamic_cast_assert(). > > > object_dynamic_cast_assert() argument is not const, I'm not sure the > > compiler can remove the call. > > > > Not remove the call, but delay it to the branch that uses it, > ie. parameter == RTAS_SYSPARM_SPLPAR_CHARACTERISTICS. I think any performance consideration here is a red herring. This particular RTAS call is a handful-of-times-per-boot thing, and only AFAIK used by AIX guests. I'm in favour of the change on the grounds of code locality and readability. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --]
On Fri, Jan 10, 2020 at 1:37 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > As we want to remove the global current_machine, > replace 'current_machine' by MACHINE(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > exec.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/exec.c b/exec.c > index d4b769d0d4..98f5b049ca 100644 > --- a/exec.c > +++ b/exec.c > @@ -1984,11 +1984,11 @@ static unsigned long last_ram_page(void) > > static void qemu_ram_setup_dump(void *addr, ram_addr_t size) > { > - int ret; > + MachineState *ms = MACHINE(qdev_get_machine()); > > /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */ > - if (!machine_dump_guest_core(current_machine)) { > - ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP); > + if (!machine_dump_guest_core(ms)) { > + int ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP); > if (ret) { > perror("qemu_madvise"); > fprintf(stderr, "madvise doesn't support MADV_DONTDUMP, " > @@ -2108,7 +2108,9 @@ size_t qemu_ram_pagesize_largest(void) > > static int memory_try_enable_merging(void *addr, size_t len) > { > - if (!machine_mem_merge(current_machine)) { > + MachineState *ms = MACHINE(qdev_get_machine()); > + > + if (!machine_mem_merge(ms)) { > /* disabled by the user */ > return 0; > } > -- > 2.21.1 > >
On Fri, Jan 10, 2020 at 1:38 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > As we want to remove the global current_machine, > replace 'current_machine->accelerator' by current_accel(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > accel/kvm/kvm-all.c | 4 ++-- > accel/tcg/tcg-all.c | 2 +- > memory.c | 2 +- > target/arm/kvm64.c | 4 ++-- > target/i386/kvm.c | 2 +- > target/ppc/kvm.c | 2 +- > vl.c | 2 +- > 7 files changed, 9 insertions(+), 9 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index b2f1a5bcb5..be1980f250 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -164,7 +164,7 @@ static NotifierList kvm_irqchip_change_notifiers = > > int kvm_get_max_memslots(void) > { > - KVMState *s = KVM_STATE(current_machine->accelerator); > + KVMState *s = KVM_STATE(current_accel()); > > return s->nr_slots; > } > @@ -1847,7 +1847,7 @@ static int kvm_max_vcpu_id(KVMState *s) > > bool kvm_vcpu_id_is_valid(int vcpu_id) > { > - KVMState *s = KVM_STATE(current_machine->accelerator); > + KVMState *s = KVM_STATE(current_accel()); > return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s); > } > > diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c > index 1dc384c8d2..1802ce02f6 100644 > --- a/accel/tcg/tcg-all.c > +++ b/accel/tcg/tcg-all.c > @@ -124,7 +124,7 @@ static void tcg_accel_instance_init(Object *obj) > > static int tcg_init(MachineState *ms) > { > - TCGState *s = TCG_STATE(current_machine->accelerator); > + TCGState *s = TCG_STATE(current_accel()); > > tcg_exec_init(s->tb_size * 1024 * 1024); > cpu_interrupt_handler = tcg_handle_interrupt; > diff --git a/memory.c b/memory.c > index 57e38b1f50..60e8993499 100644 > --- a/memory.c > +++ b/memory.c > @@ -3106,7 +3106,7 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner) > }; > GArray *fv_address_spaces; > GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal); > - AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator); > + AccelClass *ac = ACCEL_GET_CLASS(current_accel()); > > if (ac->has_memory) { > fvi.ac = ac; > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 876184b8fe..f677877a1e 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -613,14 +613,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf) > > bool kvm_arm_aarch32_supported(CPUState *cpu) > { > - KVMState *s = KVM_STATE(current_machine->accelerator); > + KVMState *s = KVM_STATE(current_accel()); > > return kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT); > } > > bool kvm_arm_sve_supported(CPUState *cpu) > { > - KVMState *s = KVM_STATE(current_machine->accelerator); > + KVMState *s = KVM_STATE(current_accel()); > > return kvm_check_extension(s, KVM_CAP_ARM_SVE); > } > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 0b511906e3..2ed15814dc 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -147,7 +147,7 @@ bool kvm_allows_irq0_override(void) > > static bool kvm_x2apic_api_set_flags(uint64_t flags) > { > - KVMState *s = KVM_STATE(current_machine->accelerator); > + KVMState *s = KVM_STATE(current_accel()); > > return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags); > } > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c > index d1c334f0e3..2d011308e0 100644 > --- a/target/ppc/kvm.c > +++ b/target/ppc/kvm.c > @@ -258,7 +258,7 @@ static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp) > > struct ppc_radix_page_info *kvm_get_radix_page_info(void) > { > - KVMState *s = KVM_STATE(current_machine->accelerator); > + KVMState *s = KVM_STATE(current_accel()); > struct ppc_radix_page_info *radix_page_info; > struct kvm_ppc_rmmu_info rmmu_info; > int i; > diff --git a/vl.c b/vl.c > index 86474a55c9..3ff3548183 100644 > --- a/vl.c > +++ b/vl.c > @@ -2804,7 +2804,7 @@ static void configure_accelerators(const char *progname) > } > > if (init_failed) { > - AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator); > + AccelClass *ac = ACCEL_GET_CLASS(current_accel()); > error_report("falling back to %s", ac->name); > } > > -- > 2.21.1 > >
On Fri, Jan 10, 2020 at 1:27 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > We want to remove the global current_machine. The accel/ > code access few times current_machine->accelerator. Introduce > the current_accel() method first, it will then be easier to > replace 'current_machine' by MACHINE(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > include/sysemu/accel.h | 2 ++ > accel/accel.c | 5 +++++ > 2 files changed, 7 insertions(+) > > diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h > index d4c1429711..47e5788530 100644 > --- a/include/sysemu/accel.h > +++ b/include/sysemu/accel.h > @@ -70,4 +70,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms); > /* Called just before os_setup_post (ie just before drop OS privs) */ > void accel_setup_post(MachineState *ms); > > +AccelState *current_accel(void); > + > #endif > diff --git a/accel/accel.c b/accel/accel.c > index 1c5c3a6abb..cb555e3b06 100644 > --- a/accel/accel.c > +++ b/accel/accel.c > @@ -63,6 +63,11 @@ int accel_init_machine(AccelState *accel, MachineState *ms) > return ret; > } > > +AccelState *current_accel(void) > +{ > + return current_machine->accelerator; > +} > + > void accel_setup_post(MachineState *ms) > { > AccelState *accel = ms->accelerator; > -- > 2.21.1 > >
On Fri, Jan 10, 2020 at 1:40 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > As we want to remove the global current_machine, > replace 'current_machine' by MACHINE(qdev_get_machine()). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> I feel like this could be squashed with the one that adds this function, but either way: Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > accel/accel.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/accel/accel.c b/accel/accel.c > index cb555e3b06..777d6ba119 100644 > --- a/accel/accel.c > +++ b/accel/accel.c > @@ -65,7 +65,9 @@ int accel_init_machine(AccelState *accel, MachineState *ms) > > AccelState *current_accel(void) > { > - return current_machine->accelerator; > + MachineState *ms = MACHINE(qdev_get_machine()); > + > + return ms->accelerator; > } > > void accel_setup_post(MachineState *ms) > -- > 2.21.1 > >
On Fri, Jan 10, 2020 at 1:34 AM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > Since we now only use current_machine in vl.c, stop exporting > it as a global variable in "hw/board.h", and make it static > to vl.c. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > include/hw/boards.h | 2 -- > vl.c | 4 ++-- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 61f8bb8e5a..b0c0d4376d 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -59,8 +59,6 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner, > #define MACHINE_CLASS(klass) \ > OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE) > > -extern MachineState *current_machine; > - > void machine_run_board_init(MachineState *machine); > bool machine_usb(MachineState *machine); > int machine_phandle_start(MachineState *machine); > diff --git a/vl.c b/vl.c > index 3ff3548183..7a69af4bef 100644 > --- a/vl.c > +++ b/vl.c > @@ -214,6 +214,8 @@ static int default_sdcard = 1; > static int default_vga = 1; > static int default_net = 1; > > +static MachineState *current_machine; > + > static struct { > const char *driver; > int *flag; > @@ -1164,8 +1166,6 @@ static int usb_parse(const char *cmdline) > /***********************************************************/ > /* machine registration */ > > -MachineState *current_machine; > - > static MachineClass *find_machine(const char *name, GSList *machines) > { > GSList *el; > -- > 2.21.1 > >
On Sun, Jan 12, 2020 at 11:45 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > On 1/12/20 10:48 AM, Alistair Francis wrote: > > On Thu, Jan 9, 2020 at 11:29 PM Philippe Mathieu-Daudé > > <philmd@redhat.com> wrote: > >> > >> As we want to remove the global current_machine, > >> replace 'current_machine' by MACHINE(qdev_get_machine()). > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> memory.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/memory.c b/memory.c > >> index d7b9bb6951..57e38b1f50 100644 > >> --- a/memory.c > >> +++ b/memory.c > >> @@ -3004,6 +3004,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, > >> int n = view->nr; > >> int i; > >> AddressSpace *as; > >> + MachineState *ms; > >> > >> qemu_printf("FlatView #%d\n", fvi->counter); > >> ++fvi->counter; > >> @@ -3026,6 +3027,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, > >> return; > >> } > >> > >> + ms = MACHINE(qdev_get_machine()); > > > > Why not set this at the top? > > Calling qdev_get_machine() is not free as it does some introspection > checks. Since we can return earlier if there are no rendered FlatView, I > placed the machinestate initialization just before it we need to access it. Works for me, maybe worth putting this in the commit? Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > > > Alistair > > > >> while (n--) { > >> mr = range->mr; > >> if (range->offset_in_region) { > >> @@ -3057,7 +3059,7 @@ static void mtree_print_flatview(gpointer key, gpointer value, > >> if (fvi->ac) { > >> for (i = 0; i < fv_address_spaces->len; ++i) { > >> as = g_array_index(fv_address_spaces, AddressSpace*, i); > >> - if (fvi->ac->has_memory(current_machine, as, > >> + if (fvi->ac->has_memory(ms, as, > >> int128_get64(range->addr.start), > >> MR_SIZE(range->addr.size) + 1)) { > >> qemu_printf(" %s", fvi->ac->name); > >> -- > >> 2.21.1 > >> > >> > > >
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> As we want to remove the global current_machine,
> replace MACHINE_GET_CLASS(current_machine) by
> MACHINE_GET_CLASS(qdev_get_machine()).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
by the migration bits.
Quick pointer to prior discussion: Message-ID: <87ftqh1ae5.fsf@dusky.pond.sub.org> https://lists.nongnu.org/archive/html/qemu-devel/2019-04/msg02860.html