Replace current_machine by qdev_get_machine()
cover Replace current_machine by qdev_get_machine()
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]:
Quoted 3 lines
I would always use MACHINE(qdev_get_machine()), espeecially outside
vl.c. Ideally,current_machinewould be static withinvl.cor even unused outside theobject_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/[email protected]/msg669475.html
[2] https://www.mail-archive.com/[email protected]/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(-)
Signature · 1 line
2.21.1
PATCH 1/15 target/arm/kvm: Use CPUState::kvm_state in kvm_arm_pmu_supported()
KVMState is already accessible via CPUState::kvm_state, use it.
--- target/arm/kvm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
target/arm/kvm.c +1 -3
@@ -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)
Signature · 1 line
2.21.1
Re: [PATCH 01/15] target/arm/kvm: Use CPUState::kvm_state in kvm_arm_pmu_supported()
Quoted 4 lines · Philippe Mathieu-Daudé
KVMState is already accessible via
CPUState::kvm_state, use it.Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 23 lines
--- target/arm/kvm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)target/arm/kvm.c +1
-4@@ -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
PATCH 2/15 hw/ppc/spapr_rtas: Use local MachineState variable
In reply to Philippe Mathieu-DaudéSince we have the MachineState already available locally, ues it instead of the global current_machine.
--- hw/ppc/spapr_rtas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
hw/ppc/spapr_rtas.c +1 -1
@@ -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 ) {
Signature · 1 line
2.21.1
Re: [PATCH 02/15] hw/ppc/spapr_rtas: Use local MachineState variable
Quoted 5 lines · On Thu, 9 Jan 2020 16:21:20 +0100 Philippe Mathieu-Daudé <[email protected]>…
Since we have the MachineState already available locally, ues it instead of the global
current_machine.Signed-off-by Philippe Mathieu-Daudé <[email protected]>---
Quoted 16 lines
hw/ppc/spapr_rtas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/ppc/spapr_rtas.c +1
-1@@ -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 ) {
Re: [PATCH 02/15] hw/ppc/spapr_rtas: Use local MachineState variable
In reply to Philippe Mathieu-DaudéQuoted 4 lines · Philippe Mathieu-Daudé
Since we have the MachineState already available locally, ues it instead of the global
current_machine.Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Quoted 17 lines
--- hw/ppc/spapr_rtas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/ppc/spapr_rtas.c +1
-1@@ -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 ) {
Signature · 4 lines
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
PATCH 3/15 hw/ppc/spapr_rtas: Access MachineState via SpaprMachineState argument
In reply to Philippe Mathieu-DaudéWe received a SpaprMachineState argument. Since SpaprMachineState inherits of MachineState, use it instead of calling qdev_get_machine.
--- hw/ppc/spapr_rtas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
hw/ppc/spapr_rtas.c +1 -1
@@ -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 );
Signature · 1 line
2.21.1
Re: [PATCH 03/15] hw/ppc/spapr_rtas: Access MachineState via SpaprMachineState argument
Quoted 5 lines · On Thu, 9 Jan 2020 16:21:21 +0100 Philippe Mathieu-Daudé <[email protected]>…
We received a SpaprMachineState argument. Since SpaprMachineState inherits of MachineState, use it instead of calling
qdev_get_machine.Signed-off-by Philippe Mathieu-Daudé <[email protected]>---
Quoted 16 lines
hw/ppc/spapr_rtas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/ppc/spapr_rtas.c +1
-1@@ -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 );
Re: [PATCH 03/15] hw/ppc/spapr_rtas: Access MachineState via SpaprMachineState argument
In reply to Philippe Mathieu-DaudéQuoted 4 lines · Philippe Mathieu-Daudé
We received a SpaprMachineState argument. Since SpaprMachineState inherits of MachineState, use it instead of calling
qdev_get_machine.Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Quoted 17 lines
--- hw/ppc/spapr_rtas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)hw/ppc/spapr_rtas.c +1
-1@@ -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 );
Signature · 4 lines
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
PATCH 4/15 hw/ppc/spapr_rtas: Restrict variables scope to single switch case
In reply to Philippe Mathieu-DaudéWe only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS case, restrict their scope to avoid unnecessary initialization.
--- hw/ppc/spapr_rtas.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
hw/ppc/spapr_rtas.c +2 -2
@@ -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,"
Signature · 1 line
2.21.1
Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case
Quoted 3 lines · On Thu, 9 Jan 2020 16:21:22 +0100 Philippe Mathieu-Daudé <[email protected]>…
We only access these variables in
RTAS_SYSPARM_SPLPAR_CHARACTERISTICScase, 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.
Quoted 19 lines
Signed-off-by Philippe Mathieu-Daudé <[email protected]>--- hw/ppc/spapr_rtas.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)hw/ppc/spapr_rtas.c +0
-2@@ -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 );
hw/ppc/spapr_rtas.c static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, · line 278
Quoted 5 lines
hw/ppc/spapr_rtas.c +2
-0@@ -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 ?
Quoted 3 lines
hw/ppc/spapr_rtas.c +0
-0char *param_val =g_strdup_printf ("MaxEntCap=%d," "DesMem=%" PRIu64"," "DesProcs=%d,"
Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case
Quoted 7 lines · Greg Kurz
Quoted 3 lines · On Thu, 9 Jan 2020 16:21:22 +0100 Philippe Mathieu-Daudé <[email protected]>…
We only access these variables in
RTAS_SYSPARM_SPLPAR_CHARACTERISTICScase, restrict their scope to avoid unnecessary initialization.
Quoted 5 lines
I guess a decent compiler can be smart enough detect that the initialization isn't needed outside of the
RTAS_SYSPARM_SPLPAR_CHARACTERISTICSbranch... 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 nestedparam_valvariable 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?
Quoted 29 lines
Quoted 19 lines
Signed-off-by Philippe Mathieu-Daudé <[email protected]>--- hw/ppc/spapr_rtas.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)hw/ppc/spapr_rtas.c +0
-2@@ -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 );hw/ppc/spapr_rtas.c static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, · line 278
Quoted 5 lines
hw/ppc/spapr_rtas.c +2
-0@@ -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_cpusvariable used to be a global. Now that it got moved belowms->smp, I'm not sure it's worth keeping it IMHO. What about dropping it completely and do:
Quoted 8 lines
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.
Quoted 7 lines
And maybe insert an empty line between the declaration of
param_valand the code for a better readability ?Quoted 3 lines
hw/ppc/spapr_rtas.c +0
-0char *param_val =g_strdup_printf ("MaxEntCap=%d," "DesMem=%" PRIu64"," "DesProcs=%d,"
Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case
Quoted 14 lines · On Fri, 10 Jan 2020 10:34:07 +0100 Philippe Mathieu-Daudé <[email protected]>…
Quoted 7 lines · Greg Kurz
Quoted 3 lines · On Thu, 9 Jan 2020 16:21:22 +0100 Philippe Mathieu-Daudé <[email protected]>…
We only access these variables in
RTAS_SYSPARM_SPLPAR_CHARACTERISTICScase, restrict their scope to avoid unnecessary initialization.Quoted 5 lines
I guess a decent compiler can be smart enough detect that the initialization isn't needed outside of the
RTAS_SYSPARM_SPLPAR_CHARACTERISTICSbranch... 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 nestedparam_valvariable already so I guess it's okay.
Quoted 4 lines
I don't want to outsmart compilers :)
The
MACHINE()macro is not a simple cast, it does object introspection withOBJECT_CHECK(), thus is not free. Since
Sure, I understand the motivation in avoiding an unneeded call to calling object_dynamic_cast_assert().
Quoted 3 lines
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.
Quoted 50 lines
Richard, Eric, do you know?
Quoted 29 lines
Quoted 19 lines
Signed-off-by Philippe Mathieu-Daudé <[email protected]>--- hw/ppc/spapr_rtas.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)hw/ppc/spapr_rtas.c +0
-2@@ -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 );hw/ppc/spapr_rtas.c static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, · line 278
Quoted 5 lines
hw/ppc/spapr_rtas.c +2
-0@@ -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_cpusvariable used to be a global. Now that it got moved belowms->smp, I'm not sure it's worth keeping it IMHO. What about dropping it completely and do:Quoted 8 lines
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.
Quoted 7 lines
And maybe insert an empty line between the declaration of
param_valand the code for a better readability ?Quoted 3 lines
hw/ppc/spapr_rtas.c +0
-0char *param_val =g_strdup_printf ("MaxEntCap=%d," "DesMem=%" PRIu64"," "DesProcs=%d,"
Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case
Quoted 7 lines · Greg Kurz
Quoted 7 lines
Quoted 5 lines
I guess a decent compiler can be smart enough detect that the initialization isn't needed outside of the
RTAS_SYSPARM_SPLPAR_CHARACTERISTICSbranch... 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 nestedparam_valvariable 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.
Quoted 36 lines
Quoted 3 lines
The
MACHINE()macro is not a simple cast, it does object introspection withOBJECT_CHECK(), thus is not free. SinceSure, I understand the motivation in avoiding an unneeded call to calling
object_dynamic_cast_assert().Quoted 3 lines
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.Quoted 22 lines
Richard, Eric, do you know?
Quoted 20 lines
Quoted 20 lines
Signed-off-by Philippe Mathieu-Daudé <[email protected]>--- hw/ppc/spapr_rtas.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)hw/ppc/spapr_rtas.c +0
-2@@ -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,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
hw/ppc/spapr_rtas.c static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu, · line 278
Quoted 4 lines
hw/ppc/spapr_rtas.c +2
-0switch (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.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case
In reply to Greg KurzQuoted 25 lines · Greg Kurz
Quoted 14 lines · On Fri, 10 Jan 2020 10:34:07 +0100 Philippe Mathieu-Daudé <[email protected]>…
Quoted 7 lines · Greg Kurz
Quoted 3 lines · On Thu, 9 Jan 2020 16:21:22 +0100 Philippe Mathieu-Daudé <[email protected]>…
We only access these variables in
RTAS_SYSPARM_SPLPAR_CHARACTERISTICScase, restrict their scope to avoid unnecessary initialization.Quoted 5 lines
I guess a decent compiler can be smart enough detect that the initialization isn't needed outside of the
RTAS_SYSPARM_SPLPAR_CHARACTERISTICSbranch... 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 nestedparam_valvariable already so I guess it's okay.Quoted 4 lines
I don't want to outsmart compilers :)
The
MACHINE()macro is not a simple cast, it does object introspection withOBJECT_CHECK(), thus is not free. SinceSure, I understand the motivation in avoiding an unneeded call to calling
object_dynamic_cast_assert().
Quoted 6 lines
Quoted 3 lines
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.
Signature · 4 lines
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
PATCH 5/15 device-hotplug: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéAs we want to remove the global current_machine,
replace MACHINE_GET_CLASS(current_machine) by
MACHINE_GET_CLASS(qdev_get_machine()).
--- device-hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
device-hotplug.c +1 -1
@@ -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);
Signature · 1 line
2.21.1
Re: [PATCH 05/15] device-hotplug: Replace current_machine by qdev_get_machine()
Quoted 6 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine,
replaceMACHINE_GET_CLASS(current_machine)by
MACHINE_GET_CLASS(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 21 lines
--- device-hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)device-hotplug.c +1
-2@@ -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
PATCH 6/15 migration/savevm: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéAs we want to remove the global current_machine,
replace MACHINE_GET_CLASS(current_machine) by
MACHINE_GET_CLASS(qdev_get_machine()).
--- migration/savevm.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
migration/savevm.c +5 -5
@@ -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);
Signature · 1 line
2.21.1
Re: [PATCH 06/15] migration/savevm: Replace current_machine by qdev_get_machine()
Quoted 14 lines · * Philippe Mathieu-Daudé ([email protected]) wrote:
As we want to remove the global
current_machine,
replaceMACHINE_GET_CLASS(current_machine)by
MACHINE_GET_CLASS(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>--- migration/savevm.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)migration/savevm.c +0
-0
migration/savevm.c static uint32_t get_validatable_capabilities_count(void) · line 296
Quoted 6 lines
migration/savevm.c +2
-1@@ -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:
(Personally I'd keep it on one line, but that's just taste)
Quoted 27 lines
migration/savevm.c +3
-4MigrationState *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);Signature · 1 line
2.21.1
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [PATCH 06/15] migration/savevm: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéQuoted 6 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine,
replaceMACHINE_GET_CLASS(current_machine)by
MACHINE_GET_CLASS(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 43 lines
--- migration/savevm.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)migration/savevm.c +5
-6@@ -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
Re: [PATCH 06/15] migration/savevm: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéQuoted 5 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine,
replaceMACHINE_GET_CLASS(current_machine)by
MACHINE_GET_CLASS(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>
by the migration bits.
PATCH 7/15 hw/core/machine-qmp-cmds: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéAs we want to remove the global current_machine,
replace MACHINE_GET_CLASS(current_machine) by
MACHINE_GET_CLASS(qdev_get_machine()).
--- hw/core/machine-qmp-cmds.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
hw/core/machine-qmp-cmds.c +2 -2
@@ -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" ); }
Signature · 1 line
2.21.1
Re: [PATCH 07/15] hw/core/machine-qmp-cmds: Replace current_machine by qdev_get_machine()
Quoted 6 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine,
replaceMACHINE_GET_CLASS(current_machine)by
MACHINE_GET_CLASS(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 24 lines
--- hw/core/machine-qmp-cmds.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)hw/core/machine-qmp-cmds.c +2
-3@@ -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
PATCH 8/15 target/arm/monitor: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéAs we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).
--- target/arm/monitor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
target/arm/monitor.c +2 -1
@@ -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;
Signature · 1 line
2.21.1
Re: [PATCH 08/15] target/arm/monitor: Replace current_machine by qdev_get_machine()
Quoted 5 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 22 lines
--- target/arm/monitor.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)target/arm/monitor.c +2
-2@@ -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
PATCH 9/15 device_tree: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéAs we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).
--- device_tree.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
device_tree.c +3 -1
@@ -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) {
Signature · 1 line
2.21.1
Re: [PATCH 09/15] device_tree: Replace current_machine by qdev_get_machine()
Quoted 5 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 23 lines
--- device_tree.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)device_tree.c +3
-2@@ -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
PATCH 10/15 memory: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéAs we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).
--- memory.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
memory.c +3 -1
@@ -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);
Signature · 1 line
2.21.1
Re: [PATCH 10/15] memory: Replace current_machine by qdev_get_machine()
Quoted 22 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>--- memory.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)memory.c +1
-0@@ -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;
memory.c static void mtree_print_flatview(gpointer key, gpointer value, · line 3030
Quoted 4 lines
memory.c +1
-0@@ -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
Quoted 16 lines
memory.c +1
-2while (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
Re: [PATCH 10/15] memory: Replace current_machine by qdev_get_machine()
Quoted 27 lines · Alistair Francis
Quoted 25 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>--- memory.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)memory.c +1
-0@@ -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 ; }
Quoted 3 lines
memory.c static void mtree_print_flatview(gpointer key, gpointer value, · line 3030
Quoted 1 line
memory.c +1
-0+ 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.
Quoted 19 lines
Alistair
Quoted 16 lines
memory.c +1
-2while (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
Alistair Francis
Reviewed-byRe: [PATCH 10/15] memory: Replace current_machine by qdev_get_machine()
Quoted 29 lines · Philippe Mathieu-Daudé
Quoted 27 lines · Alistair Francis
Quoted 25 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>--- memory.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)memory.c +1
-0@@ -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 ; }
Quoted 7 lines
Quoted 3 lines
memory.c static void mtree_print_flatview(gpointer key, gpointer value, · line 3030
Quoted 1 line
memory.c +1
-0+ 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?
Alistair
Quoted 21 lines
Quoted 20 lines
Alistair
Quoted 16 lines
memory.c +1
-2while (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
PATCH 11/15 exec: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéAs we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).
--- exec.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
exec.c +6 -4
@@ -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 ; }
Signature · 1 line
2.21.1
Re: [PATCH 11/15] exec: Replace current_machine by qdev_get_machine()
Quoted 5 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 38 lines
--- exec.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)exec.c +6
-5@@ -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
PATCH 12/15 accel: Introduce the current_accel() method
In reply to Philippe Mathieu-Daudé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()).
--- include/sysemu/accel.h | 2 ++ accel/accel.c | 5 +++++ 2 files changed, 7 insertions(+)
include/sysemu/accel.h +2 -0
@@ -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
accel/accel.c +5 -0
@@ -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;
Signature · 1 line
2.21.1
Re: [PATCH 12/15] accel: Introduce the current_accel() method
Quoted 7 lines · Philippe Mathieu-Daudé
We want to remove the global
current_machine. The accel/ code access few timescurrent_machine->accelerator. Introduce thecurrent_accel()method first, it will then be easier to replace 'current_machine' by MACHINE(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 36 lines
--- include/sysemu/accel.h | 2 ++ accel/accel.c | 5 +++++ 2 files changed, 7 insertions(+)include/sysemu/accel.h +2
-0@@ -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 accel/accel.c +5
-1@@ -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
PATCH 13/15 accel: Replace current_machine->accelerator by current_accel() method
In reply to Philippe Mathieu-DaudéAs we want to remove the global current_machine, replace 'current_machine->accelerator' by current_accel().
--- 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(-)
accel/kvm/kvm-all.c +2 -2
@@ -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); }
accel/tcg/tcg-all.c +1 -1
@@ -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;
memory.c +1 -1
@@ -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;
target/arm/kvm64.c +2 -2
@@ -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 ); }
target/i386/kvm.c +1 -1
@@ -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); }
target/ppc/kvm.c +1 -1
@@ -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;
vl.c +1 -1
@@ -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); }
Signature · 1 line
2.21.1
Re: [PATCH 13/15] accel: Replace current_machine->accelerator by current_accel() method
Quoted 5 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine, replace 'current_machine->accelerator' bycurrent_accel().Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 122 lines
--- 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(-)accel/kvm/kvm-all.c +2
-2@@ -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); }accel/tcg/tcg-all.c +1
-1@@ -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;memory.c +1
-1@@ -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;target/arm/kvm64.c +2
-2@@ -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 ); }target/i386/kvm.c +1
-1@@ -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); }target/ppc/kvm.c +1
-1@@ -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;vl.c +1
-2@@ -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
PATCH 14/15 accel/accel: Replace current_machine by qdev_get_machine()
In reply to Philippe Mathieu-DaudéAs we want to remove the global current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).
--- accel/accel.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
accel/accel.c +3 -1
@@ -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)
Signature · 1 line
2.21.1
Re: [PATCH 14/15] accel/accel: Replace current_machine by qdev_get_machine()
Quoted 5 lines · Philippe Mathieu-Daudé
As we want to remove the global
current_machine, replace 'current_machine' by MACHINE(qdev_get_machine()).Signed-off-by Philippe Mathieu-Daudé <[email protected]>
I feel like this could be squashed with the one that adds this function, but either way:
Alistair
Quoted 23 lines
--- accel/accel.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)accel/accel.c +3
-2@@ -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
PATCH 15/15 vl: Make current_machine a local variable
In reply to Philippe Mathieu-Daudé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.
--- include/hw/boards.h | 2 -- vl.c | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-)
include/hw/boards.h +0 -2
@@ -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);
vl.c +2 -2
@@ -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;
Signature · 1 line
2.21.1
Re: [PATCH 15/15] vl: Make current_machine a local variable
Quoted 6 lines · Philippe Mathieu-Daudé
Since we now only use
current_machineinvl.c, stop exporting it as a global variable in "hw/board.h", and make it static tovl.c.Signed-off-by Philippe Mathieu-Daudé <[email protected]>
Alistair
Quoted 44 lines
--- include/hw/boards.h | 2 -- vl.c | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-)include/hw/boards.h +0
-2@@ -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);vl.c +2
-3@@ -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
Quick pointer to prior discussion:
Message-ID: <[email protected]>
https://lists.nongnu.org/archive/html/qemu-devel/2019-04/msg02860.html