[PATCH 00/15] Replace current_machine by qdev_get_machine()

[PATCH 00/15] Replace current_machine by qdev_get_machine()

[Philippe Mathieu-Daudé]

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

[PATCH 01/15] target/arm/kvm: Use CPUState::kvm_state in kvm_arm_pmu_supported()

[Philippe Mathieu-Daudé]

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

Re: [PATCH 01/15] target/arm/kvm: Use CPUState::kvm_state in kvm_arm_pmu_supported()

[Alistair Francis]

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
>
>

[PATCH 02/15] hw/ppc/spapr_rtas: Use local MachineState variable

[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é <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

Re: [PATCH 02/15] hw/ppc/spapr_rtas: Use local MachineState variable

[Greg Kurz]

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) {

Re: [PATCH 02/15] hw/ppc/spapr_rtas: Use local MachineState variable

[David Gibson]

[-- 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 --]

[PATCH 03/15] hw/ppc/spapr_rtas: Access MachineState via SpaprMachineState argument

[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é <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

Re: [PATCH 03/15] hw/ppc/spapr_rtas: Access MachineState via SpaprMachineState argument

[Greg Kurz]

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);

Re: [PATCH 03/15] hw/ppc/spapr_rtas: Access MachineState via SpaprMachineState argument

[David Gibson]

[-- 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 --]

[PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case

[Philippe Mathieu-Daudé]

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

Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case

[Greg Kurz]

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,"

Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case

[Philippe Mathieu-Daudé]

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,"
> 

Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case

[Greg Kurz]

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,"
> > 
> 

Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case

[Eric Blake]

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

Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case

[David Gibson]

[-- 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 --]

[PATCH 05/15] device-hotplug: Replace current_machine by qdev_get_machine()

[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()).

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

Re: [PATCH 05/15] device-hotplug: Replace current_machine by qdev_get_machine()

[Alistair Francis]

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
>
>

[PATCH 06/15] migration/savevm: Replace current_machine by qdev_get_machine()

[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()).

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

Re: [PATCH 06/15] migration/savevm: Replace current_machine by qdev_get_machine()

[Dr. David Alan Gilbert]

* 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

Re: [PATCH 06/15] migration/savevm: Replace current_machine by qdev_get_machine()

[Alistair Francis]

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
>
>

Re: [PATCH 06/15] migration/savevm: Replace current_machine by qdev_get_machine()

[Juan Quintela]

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.

[PATCH 07/15] hw/core/machine-qmp-cmds: Replace current_machine by qdev_get_machine()

[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()).

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

Re: [PATCH 07/15] hw/core/machine-qmp-cmds: Replace current_machine by qdev_get_machine()

[Alistair Francis]

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
>
>

[PATCH 08/15] target/arm/monitor: Replace current_machine by qdev_get_machine()

[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é <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

Re: [PATCH 08/15] target/arm/monitor: Replace current_machine by qdev_get_machine()

[Alistair Francis]

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
>
>

[PATCH 09/15] device_tree: Replace current_machine by qdev_get_machine()

[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é <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

Re: [PATCH 09/15] device_tree: Replace current_machine by qdev_get_machine()

[Alistair Francis]

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
>
>

[PATCH 10/15] memory: Replace current_machine by qdev_get_machine()

[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é <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

Re: [PATCH 10/15] memory: Replace current_machine by qdev_get_machine()

[Alistair Francis]

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
>
>

Re: [PATCH 10/15] memory: Replace current_machine by qdev_get_machine()

[Philippe Mathieu-Daudé]

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
>>
>>
> 

Re: [PATCH 10/15] memory: Replace current_machine by qdev_get_machine()

[Alistair Francis]

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
> >>
> >>
> >
>

[PATCH 11/15] exec: Replace current_machine by qdev_get_machine()

[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é <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

Re: [PATCH 11/15] exec: Replace current_machine by qdev_get_machine()

[Alistair Francis]

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
>
>

[PATCH 12/15] accel: Introduce the current_accel() method

[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()).

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

Re: [PATCH 12/15] accel: Introduce the current_accel() method

[Alistair Francis]

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
>
>

[PATCH 13/15] accel: Replace current_machine->accelerator by current_accel() method

[Philippe Mathieu-Daudé]

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

Re: [PATCH 13/15] accel: Replace current_machine->accelerator by current_accel() method

[Alistair Francis]

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
>
>

[PATCH 14/15] accel/accel: Replace current_machine by qdev_get_machine()

[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é <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

Re: [PATCH 14/15] accel/accel: Replace current_machine by qdev_get_machine()

[Alistair Francis]

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
>
>

[PATCH 15/15] vl: Make current_machine a local variable

[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.

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

Re: [PATCH 15/15] vl: Make current_machine a local variable

[Alistair Francis]

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
>
>

Re: [PATCH 00/15] Replace current_machine by qdev_get_machine()

[Markus Armbruster]

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