This patch makes the remaining dozen or so uses of the global
current_machine outside vl.c use qdev_get_machine() instead,
and then make current_machine local to vl.c instead of global.

Signed-off-by: Like Xu <like.xu@linux.intel.com>

---
Changes in v2:
    - make the variable current_machine "static" (Thomas Huth)

---
 accel/kvm/kvm-all.c | 6 ++++--
 device-hotplug.c    | 3 ++-
 device_tree.c       | 3 ++-
 exec.c              | 6 ++++--
 hw/ppc/spapr_rtas.c | 3 ++-
 include/hw/boards.h | 1 -
 migration/savevm.c  | 9 ++++++---
 qmp.c               | 3 ++-
 target/i386/kvm.c   | 3 ++-
 target/ppc/kvm.c    | 3 ++-
 vl.c                | 4 ++--
 11 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 241db49..d103de2 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -140,7 +140,8 @@ static const KVMCapabilityInfo kvm_required_capabilites[] = {
 
 int kvm_get_max_memslots(void)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    MachineState *ms = MACHINE(qdev_get_machine());
+    KVMState *s = KVM_STATE(ms->accelerator);
 
     return s->nr_slots;
 }
@@ -1519,7 +1520,8 @@ static int kvm_max_vcpu_id(KVMState *s)
 
 bool kvm_vcpu_id_is_valid(int vcpu_id)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    MachineState *ms = MACHINE(qdev_get_machine());
+    KVMState *s = KVM_STATE(ms->accelerator);
     return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s);
 }
 
diff --git a/device-hotplug.c b/device-hotplug.c
index 6090d5f..3d033f5 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -37,6 +37,7 @@
 
 static DriveInfo *add_init_drive(const char *optstr)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     Error *err = NULL;
     DriveInfo *dinfo;
     QemuOpts *opts;
@@ -46,7 +47,7 @@ static DriveInfo *add_init_drive(const char *optstr)
     if (!opts)
         return NULL;
 
-    mc = MACHINE_GET_CLASS(current_machine);
+    mc = MACHINE_GET_CLASS(ms);
     dinfo = drive_new(opts, mc->block_default_type, &err);
     if (!dinfo) {
         error_report_err(err);
diff --git a/device_tree.c b/device_tree.c
index 296278e..370b74e 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -455,6 +455,7 @@ int qemu_fdt_setprop_phandle(void *fdt, const char *node_path,
 
 uint32_t qemu_fdt_alloc_phandle(void *fdt)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     static int phandle = 0x0;
 
     /*
@@ -462,7 +463,7 @@ uint32_t qemu_fdt_alloc_phandle(void *fdt)
      * which phandle id to start allocating phandles.
      */
     if (!phandle) {
-        phandle = machine_phandle_start(current_machine);
+        phandle = machine_phandle_start(ms);
     }
 
     if (!phandle) {
diff --git a/exec.c b/exec.c
index 6ab62f4..15ff2b1 100644
--- a/exec.c
+++ b/exec.c
@@ -1969,10 +1969,11 @@ static unsigned long last_ram_page(void)
 
 static void qemu_ram_setup_dump(void *addr, ram_addr_t size)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     int ret;
 
     /* Use MADV_DONTDUMP, if user doesn't want the guest memory in the core */
-    if (!machine_dump_guest_core(current_machine)) {
+    if (!machine_dump_guest_core(ms)) {
         ret = qemu_madvise(addr, size, QEMU_MADV_DONTDUMP);
         if (ret) {
             perror("qemu_madvise");
@@ -2094,7 +2095,8 @@ 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;
     }
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
index 24c45b1..51e320d 100644
--- a/hw/ppc/spapr_rtas.c
+++ b/hw/ppc/spapr_rtas.c
@@ -231,6 +231,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
                                           target_ulong args,
                                           uint32_t nret, target_ulong rets)
 {
+    MachineState *ms = MACHINE(spapr);
     target_ulong parameter = rtas_ld(args, 0);
     target_ulong buffer = rtas_ld(args, 1);
     target_ulong length = rtas_ld(args, 2);
@@ -243,7 +244,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,
                                           smp_cpus,
                                           max_cpus);
         ret = sysparm_st(buffer, length, param_val, strlen(param_val) + 1);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index e231860..1d598c8 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -58,7 +58,6 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
     OBJECT_CLASS_CHECK(MachineClass, (klass), TYPE_MACHINE)
 
 MachineClass *find_default_machine(void);
-extern MachineState *current_machine;
 
 void machine_run_board_init(MachineState *machine);
 bool machine_usb(MachineState *machine);
diff --git a/migration/savevm.c b/migration/savevm.c
index 1415001..8077ac5 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -286,8 +286,9 @@ static uint32_t get_validatable_capabilities_count(void)
 
 static int configuration_pre_save(void *opaque)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     SaveState *state = opaque;
-    const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
+    const char *current_name = MACHINE_GET_CLASS(ms)->name;
     MigrationState *s = migrate_get_current();
     int i, j;
 
@@ -355,8 +356,9 @@ static bool configuration_validate_capabilities(SaveState *state)
 
 static int configuration_post_load(void *opaque, int version_id)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     SaveState *state = opaque;
-    const char *current_name = MACHINE_GET_CLASS(current_machine)->name;
+    const char *current_name = MACHINE_GET_CLASS(ms)->name;
 
     if (strncmp(state->name, current_name, state->len) != 0) {
         error_report("Machine type received is '%.*s' and local is '%s'",
@@ -566,9 +568,10 @@ static void dump_vmstate_vmsd(FILE *out_file,
 
 static void dump_machine_type(FILE *out_file)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     MachineClass *mc;
 
-    mc = MACHINE_GET_CLASS(current_machine);
+    mc = MACHINE_GET_CLASS(ms);
 
     fprintf(out_file, "  \"vmschkmachine\": {\n");
     fprintf(out_file, "    \"Name\": \"%s\"\n", mc->name);
diff --git a/qmp.c b/qmp.c
index b92d62c..f2a5473 100644
--- a/qmp.c
+++ b/qmp.c
@@ -119,9 +119,10 @@ void qmp_system_powerdown(Error **erp)
 
 void qmp_cpu_add(int64_t id, Error **errp)
 {
+    MachineState *ms = MACHINE(qdev_get_machine());
     MachineClass *mc;
 
-    mc = MACHINE_GET_CLASS(current_machine);
+    mc = MACHINE_GET_CLASS(ms);
     if (mc->hot_add_cpu) {
         mc->hot_add_cpu(id, errp);
     } else {
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 3b29ce5..b25d766 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -134,7 +134,8 @@ bool kvm_allows_irq0_override(void)
 
 static bool kvm_x2apic_api_set_flags(uint64_t flags)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    MachineState *ms = MACHINE(qdev_get_machine());
+    KVMState *s = KVM_STATE(ms->accelerator);
 
     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 2427c8e..c3bea37 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -276,7 +276,8 @@ 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);
+    MachineState *ms = MACHINE(qdev_get_machine());
+    KVMState *s = KVM_STATE(ms->accelerator);
     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 d61d560..4c34907 100644
--- a/vl.c
+++ b/vl.c
@@ -1265,6 +1265,8 @@ static QemuOptsList qemu_smp_opts = {
     },
 };
 
+static MachineState *current_machine;
+
 static void smp_parse(QemuOpts *opts)
 {
     if (opts) {
@@ -1462,8 +1464,6 @@ static int usb_parse(const char *cmdline)
 /***********************************************************/
 /* machine registration */
 
-MachineState *current_machine;
-
 static MachineClass *find_machine(const char *name)
 {
     GSList *el, *machines = object_class_get_list(TYPE_MACHINE, false);
-- 
1.8.3.1

Like Xu <like.xu@linux.intel.com> writes:

> This patch makes the remaining dozen or so uses of the global
> current_machine outside vl.c use qdev_get_machine() instead,
> and then make current_machine local to vl.c instead of global.
>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>

You effectively replace

    current_machine

by

    MACHINE(qdev_get_machine())

qdev_get_machine() uses container_get(), which has a side effect: any
path component that doesn't exist already gets created as "container"
object.  In case of qdev_get_machine(), that's just "/machine".

Creating "/machine" as "container" is of course wrong.  You therefore
must not use qdev_get_machine() before main() creates "/machine".  It
does like this:

    object_property_add_child(object_get_root(), "machine",
                              OBJECT(current_machine), &error_abort);

I recently had several cases of code rearrangements explode because the
reordered code called qdev_get_machine() too early.  Makes me rather
skeptical about this patch.  To be frank, I consider qdev_get_machine()
a trap for the unwary.  container_get(), too.

If we decide using it to make current_machine static a good idea anyway,
we need to check the new uses carefully to make sure they can't run
before main() creates "/machine".

On 2019/4/2 19:27, Markus Armbruster wrote:
> Like Xu <like.xu@linux.intel.com> writes:
> 
>> This patch makes the remaining dozen or so uses of the global
>> current_machine outside vl.c use qdev_get_machine() instead,
>> and then make current_machine local to vl.c instead of global.
>>
>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> 
> You effectively replace
> 
>      current_machine
> 
> by
> 
>      MACHINE(qdev_get_machine())
> 
> qdev_get_machine() uses container_get(), which has a side effect: any
> path component that doesn't exist already gets created as "container"
> object.  In case of qdev_get_machine(), that's just "/machine".
> 
> Creating "/machine" as "container" is of course wrong.  You therefore
> must not use qdev_get_machine() before main() creates "/machine".  It
> does like this:
> 
>      object_property_add_child(object_get_root(), "machine",
>                                OBJECT(current_machine), &error_abort);
> 
> I recently had several cases of code rearrangements explode because the
> reordered code called qdev_get_machine() too early.  Makes me rather
> skeptical about this patch.  To be frank, I consider qdev_get_machine()
> a trap for the unwary.  container_get(), too.
> 
> If we decide using it to make current_machine static a good idea anyway,
> we need to check the new uses carefully to make sure they can't run
> before main() creates "/machine".
> 
> 

Thanks for your comments and this issue may not exist in this patch.

I am curious when and where we would call qdev_get_machine() before 
main() initializes current_machine and adds it to QOM store which is
called very early.

On Tue, 2 Apr 2019 21:09:39 +0800
Like Xu <like.xu@linux.intel.com> wrote:

> On 2019/4/2 19:27, Markus Armbruster wrote:
> > Like Xu <like.xu@linux.intel.com> writes:
> >   
> >> This patch makes the remaining dozen or so uses of the global
> >> current_machine outside vl.c use qdev_get_machine() instead,
> >> and then make current_machine local to vl.c instead of global.
> >>
> >> Signed-off-by: Like Xu <like.xu@linux.intel.com>  
> > 
> > You effectively replace
> > 
> >      current_machine
> > 
> > by
> > 
> >      MACHINE(qdev_get_machine())
> > 
> > qdev_get_machine() uses container_get(), which has a side effect: any
> > path component that doesn't exist already gets created as "container"
> > object.  In case of qdev_get_machine(), that's just "/machine".
> > 
> > Creating "/machine" as "container" is of course wrong.  You therefore
> > must not use qdev_get_machine() before main() creates "/machine".  It
> > does like this:
> > 
> >      object_property_add_child(object_get_root(), "machine",
> >                                OBJECT(current_machine), &error_abort);
> > 
> > I recently had several cases of code rearrangements explode because the
> > reordered code called qdev_get_machine() too early.  Makes me rather
> > skeptical about this patch.  To be frank, I consider qdev_get_machine()
> > a trap for the unwary.  container_get(), too.
> > 
> > If we decide using it to make current_machine static a good idea anyway,
> > we need to check the new uses carefully to make sure they can't run
> > before main() creates "/machine".

maybe we can assert in qdev_get_machine() if machine hasn't been created yet?
with this at least it will be hard to misuse function or catch invalid users.
(but it still might miss some use cases/CLI options which are not tested)

> >   
> 
> Thanks for your comments and this issue may not exist in this patch.
> 
> I am curious when and where we would call qdev_get_machine() before 
> main() initializes current_machine and adds it to QOM store which is
> called very early.

Igor Mammedov <imammedo@redhat.com> writes:

> On Tue, 2 Apr 2019 21:09:39 +0800
> Like Xu <like.xu@linux.intel.com> wrote:
>
>> On 2019/4/2 19:27, Markus Armbruster wrote:
>> > Like Xu <like.xu@linux.intel.com> writes:
>> >   
>> >> This patch makes the remaining dozen or so uses of the global
>> >> current_machine outside vl.c use qdev_get_machine() instead,
>> >> and then make current_machine local to vl.c instead of global.
>> >>
>> >> Signed-off-by: Like Xu <like.xu@linux.intel.com>  
>> > 
>> > You effectively replace
>> > 
>> >      current_machine
>> > 
>> > by
>> > 
>> >      MACHINE(qdev_get_machine())
>> > 
>> > qdev_get_machine() uses container_get(), which has a side effect: any
>> > path component that doesn't exist already gets created as "container"
>> > object.  In case of qdev_get_machine(), that's just "/machine".
>> > 
>> > Creating "/machine" as "container" is of course wrong.  You therefore
>> > must not use qdev_get_machine() before main() creates "/machine".  It
>> > does like this:
>> > 
>> >      object_property_add_child(object_get_root(), "machine",
>> >                                OBJECT(current_machine), &error_abort);
>> > 
>> > I recently had several cases of code rearrangements explode because the
>> > reordered code called qdev_get_machine() too early.  Makes me rather
>> > skeptical about this patch.  To be frank, I consider qdev_get_machine()
>> > a trap for the unwary.  container_get(), too.
>> > 
>> > If we decide using it to make current_machine static a good idea anyway,
>> > we need to check the new uses carefully to make sure they can't run
>> > before main() creates "/machine".
>
> maybe we can assert in qdev_get_machine() if machine hasn't been created yet?
> with this at least it will be hard to misuse function or catch invalid users.
> (but it still might miss some use cases/CLI options which are not tested)

Good idea.  When my code created "/machine" as a container, debugging
the resulting crash took me a bit of time.  The assertion you propose
would've saved me some.

On 2019/4/3 0:13, Markus Armbruster wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
>> On Tue, 2 Apr 2019 21:09:39 +0800
>> Like Xu <like.xu@linux.intel.com> wrote:
>>
>>> On 2019/4/2 19:27, Markus Armbruster wrote:
>>>> Like Xu <like.xu@linux.intel.com> writes:
>>>>    
>>>>> This patch makes the remaining dozen or so uses of the global
>>>>> current_machine outside vl.c use qdev_get_machine() instead,
>>>>> and then make current_machine local to vl.c instead of global.
>>>>>
>>>>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>>>>
>>>> You effectively replace
>>>>
>>>>       current_machine
>>>>
>>>> by
>>>>
>>>>       MACHINE(qdev_get_machine())
>>>>
>>>> qdev_get_machine() uses container_get(), which has a side effect: any
>>>> path component that doesn't exist already gets created as "container"
>>>> object.  In case of qdev_get_machine(), that's just "/machine".
>>>>
>>>> Creating "/machine" as "container" is of course wrong.  You therefore
>>>> must not use qdev_get_machine() before main() creates "/machine".  It
>>>> does like this:
>>>>
>>>>       object_property_add_child(object_get_root(), "machine",
>>>>                                 OBJECT(current_machine), &error_abort);
>>>>
>>>> I recently had several cases of code rearrangements explode because the
>>>> reordered code called qdev_get_machine() too early.  Makes me rather
>>>> skeptical about this patch.  To be frank, I consider qdev_get_machine()
>>>> a trap for the unwary.  container_get(), too.
>>>>
>>>> If we decide using it to make current_machine static a good idea anyway,
>>>> we need to check the new uses carefully to make sure they can't run
>>>> before main() creates "/machine".
>>
>> maybe we can assert in qdev_get_machine() if machine hasn't been created yet?
>> with this at least it will be hard to misuse function or catch invalid users.
>> (but it still might miss some use cases/CLI options which are not tested)
> 
> Good idea.  When my code created "/machine" as a container, debugging
> the resulting crash took me a bit of time.  The assertion you propose
> would've saved me some.
> 
> 

Good idea and please help review if this assertion in qdev_get_machine() 
could help for your code rearrangement:

       if (dev == NULL) {
           dev = container_get(object_get_root(), "/machine");
       }

+     assert(object_dynamic_cast(dev, TYPE_MACHINE) != NULL);
       return dev;

On Tue, 2 Apr 2019 at 23:13, Markus Armbruster <armbru@redhat.com> wrote:
>
> Igor Mammedov <imammedo@redhat.com> writes:
> > maybe we can assert in qdev_get_machine() if machine hasn't been created yet?
> > with this at least it will be hard to misuse function or catch invalid users.
> > (but it still might miss some use cases/CLI options which are not tested)
>
> Good idea.  When my code created "/machine" as a container, debugging
> the resulting crash took me a bit of time.  The assertion you propose
> would've saved me some.

One wrinkle to watch out for is code paths that are used in the
linux-user emulator, where there is no machine at all... For instance
cpu_common_realizefn() handles this case by explicitly checking
whether the thing it gets back from qdev_get_machine() is a
TYPE_MACHINE or not.

thanks
-- PMM

On Tue, Apr 02, 2019 at 11:23:42PM +0700, Peter Maydell wrote:
> On Tue, 2 Apr 2019 at 23:13, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Igor Mammedov <imammedo@redhat.com> writes:
> > > maybe we can assert in qdev_get_machine() if machine hasn't been created yet?
> > > with this at least it will be hard to misuse function or catch invalid users.
> > > (but it still might miss some use cases/CLI options which are not tested)
> >
> > Good idea.  When my code created "/machine" as a container, debugging
> > the resulting crash took me a bit of time.  The assertion you propose
> > would've saved me some.
> 
> One wrinkle to watch out for is code paths that are used in the
> linux-user emulator, where there is no machine at all... For instance
> cpu_common_realizefn() handles this case by explicitly checking
> whether the thing it gets back from qdev_get_machine() is a
> TYPE_MACHINE or not.

Is there a real use case for calling qdev_get_machine() in user
mode?

I'd prefer to make qdev_get_machine() unavailable in user mode,
so we could detect these cases at compile time (and treat them as
bugs).

-- 
Eduardo

On Wed, 3 Apr 2019 at 02:16, Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> On Tue, Apr 02, 2019 at 11:23:42PM +0700, Peter Maydell wrote:
> > One wrinkle to watch out for is code paths that are used in the
> > linux-user emulator, where there is no machine at all... For instance
> > cpu_common_realizefn() handles this case by explicitly checking
> > whether the thing it gets back from qdev_get_machine() is a
> > TYPE_MACHINE or not.
>
> Is there a real use case for calling qdev_get_machine() in user
> mode?

In this case it's "I'm in a function which is in obj-common (so shared
between user-only and system-emulation) and I need to do something
if we're in system emulation mode". It could probably be restructured
somehow.

thanks
-- PMM

On Tue, 2 Apr 2019 23:23:42 +0700
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Tue, 2 Apr 2019 at 23:13, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Igor Mammedov <imammedo@redhat.com> writes:  
> > > maybe we can assert in qdev_get_machine() if machine hasn't been created yet?
> > > with this at least it will be hard to misuse function or catch invalid users.
> > > (but it still might miss some use cases/CLI options which are not tested)  
> >
> > Good idea.  When my code created "/machine" as a container, debugging
> > the resulting crash took me a bit of time.  The assertion you propose
> > would've saved me some.  
> 
> One wrinkle to watch out for is code paths that are used in the
> linux-user emulator, where there is no machine at all... For instance
> cpu_common_realizefn() handles this case by explicitly checking
> whether the thing it gets back from qdev_get_machine() is a
> TYPE_MACHINE or not.
this one can be solved by adding 'ignore_memory_transaction_failures'
property to the CPU class where it applies (I'm not sure why it's
in generic cpu code instead of ARM only) and then setting compat
property in affected boards code.

but looking at users qdev_get_machine() it's not the only place where it
would explode in *-user build, so it would need to be taken care of
as well.

> 
> thanks
> -- PMM
> 

On Thu, 4 Apr 2019 at 17:05, Igor Mammedov <imammedo@redhat.com> wrote:
> On Tue, 2 Apr 2019 23:23:42 +0700
> Peter Maydell <peter.maydell@linaro.org> wrote:
> > One wrinkle to watch out for is code paths that are used in the
> > linux-user emulator, where there is no machine at all... For instance
> > cpu_common_realizefn() handles this case by explicitly checking
> > whether the thing it gets back from qdev_get_machine() is a
> > TYPE_MACHINE or not.
> this one can be solved by adding 'ignore_memory_transaction_failures'
> property to the CPU class where it applies (I'm not sure why it's
> in generic cpu code instead of ARM only) and then setting compat
> property in affected boards code.

It is not arm-specific, it just happens that all the current users
are arm boards. It is a generic thing that can apply to
any CPU for any target architecture, but only if the board
requests it. The current code seemed a reasonable way to
implement this that didn't involve having to write too much
invasive plumbing code to get the behaviour to apply in the
cases where it needed to be applied, but if there's a better
way to do this I'm happy to review/test patches.

thanks
-- PMM