Cleanups around the &#39;current_machine&#39; global variable

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/ppc/spapr_rtas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/ppc/spapr_rtas.c +1 -1

index 8d8d8cdfcb..e88bb1930e 100644
@@ -281,7 +281,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
                                           "DesProcs=%d,"
                                           "MaxPlatProcs=%d",
                                           max_cpus,
-                                          current_machine->ram_size / MiB,
+                                          ms->ram_size / MiB,
                                           ms->smp.cpus,
                                           max_cpus);
         if (pcc->n_host_threads > 0) {

Signature · 1 line

2.21.1

Philippe Mathieu-Daudé

author

2020-01-21 11:06 UTC #2 lore raw

PATCH v2 2/10 hw/ppc/spapr_rtas: Access MachineState via SpaprMachineState argument

We received a SpaprMachineState argument. Since SpaprMachineState inherits of MachineState, use it instead of calling qdev_get_machine.

Reviewed-by Greg Kurz <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 hw/ppc/spapr_rtas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/ppc/spapr_rtas.c +1 -1

index e88bb1930e..6f06e9d7fe 100644
@@ -267,7 +267,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
                                           uint32_t nret, target_ulong rets)
 {
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
-    MachineState *ms = MACHINE(qdev_get_machine());
+    MachineState *ms = MACHINE(spapr);
     unsigned int max_cpus = ms->smp.max_cpus;
     target_ulong parameter = rtas_ld(args, 0);
     target_ulong buffer = rtas_ld(args, 1);

Signature · 1 line

2.21.1

Philippe Mathieu-Daudé

author

2020-01-21 11:05 UTC #3 lore raw

PATCH v2 3/10 hw/ppc/spapr_rtas: Remove local variable

We only access this variable in the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS case. Use it in place and remove the local declaration.

Suggested-by Greg Kurz <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
v2: Do not reduce MachineState variable scope (David Gibson)

---
 hw/ppc/spapr_rtas.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

hw/ppc/spapr_rtas.c +2 -3

index 6f06e9d7fe..85135e0e1a 100644
@@ -268,7 +268,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
 {
     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);

@@ -280,10 +279,10 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
                                           "DesMem=%" PRIu64 ","
                                           "DesProcs=%d,"
                                           "MaxPlatProcs=%d",
-                                          max_cpus,
+                                          ms->smp.max_cpus,
                                           ms->ram_size / MiB,
                                           ms->smp.cpus,
-                                          max_cpus);
+                                          ms->smp.max_cpus);
         if (pcc->n_host_threads > 0) {
             char *hostthr_val, *old = param_val;

Signature · 1 line

2.21.1

David Gibson

Acked-by on v2 3/10

2020-01-22 03:31 UTC #4 lore raw

Re: [PATCH v2 03/10] hw/ppc/spapr_rtas: Remove local variable

Quoted 5 lines · Philippe Mathieu-Daudé

We only access this variable in the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS case. Use it in place and remove the local declaration.
Suggested-by Greg Kurz <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>

Quoted 31 lines

---
v2: Do not reduce MachineState variable scope (David Gibson)

---
 hw/ppc/spapr_rtas.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

hw/ppc/spapr_rtas.c +2 -3

index 6f06e9d7fe..85135e0e1a 100644
@@ -268,7 +268,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
 {
     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);

@@ -280,10 +279,10 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
                                           "DesMem=%" PRIu64 ","
                                           "DesProcs=%d,"
                                           "MaxPlatProcs=%d",
-                                          max_cpus,
+                                          ms->smp.max_cpus,
                                           ms->ram_size / MiB,
                                           ms->smp.cpus,
-                                          max_cpus);
+                                          ms->smp.max_cpus);
         if (pcc->n_host_threads > 0) {
             char *hostthr_val, *old = param_val;

Signature · 4 lines

David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Philippe Mathieu-Daudé

author

2020-01-21 11:07 UTC #5 lore raw

PATCH v2 4/10 target/arm/kvm: Use CPUState::kvm_state in kvm_arm_pmu_supported()

KVMState is already accessible via CPUState::kvm_state, use it.

Reviewed-by Alistair Francis <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
 target/arm/kvm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

target/arm/kvm.c +1 -3

index b87b59a02a..8d82889150 100644
@@ -181,9 +181,7 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
 
 bool kvm_arm_pmu_supported(CPUState *cpu)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
-
-    return kvm_check_extension(s, KVM_CAP_ARM_PMU_V3);
+    return kvm_check_extension(cpu->kvm_state, KVM_CAP_ARM_PMU_V3);
 }
 
 int kvm_arm_get_max_vm_ipa_size(MachineState *ms)

Signature · 1 line

2.21.1

Philippe Mathieu-Daudé

author

2020-01-21 11:10 UTC #6 lore raw

PATCH v2 5/10 target/s390x: Remove duplicated ifdef macro

Commit ae71ed8610 replaced the use of global max_cpus variable with a machine property, but introduced a unnecessary ifdef, as this block is already in the 'not CONFIG_USER_ONLY' branch part:

   86 #if defined(CONFIG_USER_ONLY)
   87
  ...
  106 #else /* !CONFIG_USER_ONLY */
  107
  ...
  292 static void do_ext_interrupt(CPUS390XState *env)
  293 {
  ...
  313 #ifndef CONFIG_USER_ONLY
  314         MachineState *ms = MACHINE(qdev_get_machine());
  315         unsigned int max_cpus = ms->smp.max_cpus;
  316 #endif

To ease code review, remove the duplicated preprocessor macro, and move the declarations at the beginning of the statement.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
v2: New patch
Cc: Like Xu <[email protected]>

---
 target/s390x/excp_helper.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

target/s390x/excp_helper.c +3 -4

index e70c20d363..1e9d6f20c1 100644
@@ -305,15 +305,14 @@ static void do_ext_interrupt(CPUS390XState *env)
 
     if ((env->pending_int & INTERRUPT_EMERGENCY_SIGNAL) &&
         (env->cregs[0] & CR0_EMERGENCY_SIGNAL_SC)) {
+        MachineState *ms = MACHINE(qdev_get_machine());
+        unsigned int max_cpus = ms->smp.max_cpus;
+
         lowcore->ext_int_code = cpu_to_be16(EXT_EMERGENCY);
         cpu_addr = find_first_bit(env->emergency_signals, S390_MAX_CPUS);
         g_assert(cpu_addr < S390_MAX_CPUS);
         lowcore->cpu_addr = cpu_to_be16(cpu_addr);
         clear_bit(cpu_addr, env->emergency_signals);
-#ifndef CONFIG_USER_ONLY
-        MachineState *ms = MACHINE(qdev_get_machine());
-        unsigned int max_cpus = ms->smp.max_cpus;
-#endif
         if (bitmap_empty(env->emergency_signals, max_cpus)) {
             env->pending_int &= ~INTERRUPT_EMERGENCY_SIGNAL;
         }

Signature · 1 line

2.21.1

Cornelia Huck

Applied

2020-01-21 14:05 UTC #7 lore raw

Re: [PATCH v2 05/10] target/s390x: Remove duplicated ifdef macro

Quoted 21 lines · On Tue, 21 Jan 2020 12:03:44 +0100 Philippe Mathieu-Daudé <[email protected]>…

Commit ae71ed8610 replaced the use of global max_cpus variable with a machine property, but introduced a unnecessary ifdef, as this block is already in the 'not CONFIG_USER_ONLY' branch part:
   86 #if defined(CONFIG_USER_ONLY)
   87
  ...
  106 #else /* !CONFIG_USER_ONLY */
  107
  ...
  292 static void do_ext_interrupt(CPUS390XState *env)
  293 {
  ...
  313 #ifndef CONFIG_USER_ONLY
  314         MachineState *ms = MACHINE(qdev_get_machine());
  315         unsigned int max_cpus = ms->smp.max_cpus;
  316 #endif
To ease code review, remove the duplicated preprocessor macro, and move the declarations at the beginning of the statement.

Quoted 7 lines

Signed-off-by Philippe Mathieu-Daudé <[email protected]>
---
v2: New patch
Cc: Like Xu <[email protected]>
---
 target/s390x/excp_helper.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Thanks, applied.

Cornelia Huck

Reviewed-by on v2 5/10

2020-01-21 16:48 UTC #8 lore raw

Re: [PATCH v2 05/10] target/s390x: Remove duplicated ifdef macro

Quoted 21 lines · On Tue, 21 Jan 2020 12:03:44 +0100 Philippe Mathieu-Daudé <[email protected]>…

Commit ae71ed8610 replaced the use of global max_cpus variable with a machine property, but introduced a unnecessary ifdef, as this block is already in the 'not CONFIG_USER_ONLY' branch part:
   86 #if defined(CONFIG_USER_ONLY)
   87
  ...
  106 #else /* !CONFIG_USER_ONLY */
  107
  ...
  292 static void do_ext_interrupt(CPUS390XState *env)
  293 {
  ...
  313 #ifndef CONFIG_USER_ONLY
  314         MachineState *ms = MACHINE(qdev_get_machine());
  315         unsigned int max_cpus = ms->smp.max_cpus;
  316 #endif
To ease code review, remove the duplicated preprocessor macro, and move the declarations at the beginning of the statement.

Quoted 7 lines

Signed-off-by Philippe Mathieu-Daudé <[email protected]>
---
v2: New patch
Cc: Like Xu <[email protected]>
---
 target/s390x/excp_helper.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

I case this goes via the misc tree:

Philippe Mathieu-Daudé

author

2020-01-21 11:08 UTC #9 lore raw

PATCH v2 6/10 qom/object: Display more helpful message when a parent is missing

QEMU object model is scarse in documentation. Some calls are recursive, and it might be hard to figure out even trivial issues.

We can avoid developers to waste time in a debugging session by displaying a simple error message.

This commit is also similar to e02bdf1cecd2 ("Display more helpful message when an object type is missing").

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
v2: New patch

---
 qom/object.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

qom/object.c +5 -1

index 0d971ca897..bc13a55094 100644
@@ -173,7 +173,11 @@ static TypeImpl *type_get_parent(TypeImpl *type)
 {
     if (!type->parent_type && type->parent) {
         type->parent_type = type_get_by_name(type->parent);
-        g_assert(type->parent_type != NULL);
+        if (!type->parent_type) {
+            fprintf(stderr, "Object '%s' missing its parent '%s'\n",
+                    type->name, type->parent);
+            abort();
+        }
     }
 
     return type->parent_type;

Signature · 1 line

2.21.1

Cornelia Huck

Reviewed-by on v2 6/10

2020-01-21 14:32 UTC #10 lore raw

Re: [PATCH v2 06/10] qom/object: Display more helpful message when a parent is missing

Quoted 21 lines · On Tue, 21 Jan 2020 12:03:45 +0100 Philippe Mathieu-Daudé <[email protected]>…

QEMU object model is scarse in documentation. Some calls are recursive, and it might be hard to figure out even trivial issues.
We can avoid developers to waste time in a debugging session by displaying a simple error message.
This commit is also similar to e02bdf1cecd2 ("Display more helpful message when an object type is missing").
Signed-off-by Philippe Mathieu-Daudé <[email protected]>
---
v2: New patch
---
 qom/object.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
qom/object.c +0 -0
index 0d971ca897..bc13a55094 100644

qom/object.c static TypeImpl *type_get_parent(TypeImpl *type) · line 177

Quoted 6 lines

qom/object.c +2 -1

@@ -173,7 +173,11 @@ static TypeImpl *type_get_parent(TypeImpl *type)
 {
     if (!type->parent_type && type->parent) {
         type->parent_type = type_get_by_name(type->parent);
-        g_assert(type->parent_type != NULL);
+        if (!type->parent_type) {
+            fprintf(stderr, "Object '%s' missing its parent '%s'\n",

Maybe s/missing/is missing/ ?

Anyway, a real improvement :)

qom/object.c static TypeImpl *type_get_parent(TypeImpl *type) · line 183

Quoted 6 lines

qom/object.c +3 -0

+                    type->name, type->parent);
+            abort();
+        }
     }
 
     return type->parent_type;

Philippe Mathieu-Daudé

author

2020-01-21 11:09 UTC #11 lore raw

PATCH v2 7/10 qdev: Abort if the root machine container is missing

The QEMU device API (qdev) relies on having the '/machine' container always available.

If it is missing, QEMU will later crash dereferencing a NULL pointer, we will get a SEGV, open a debugger, look at the backtrace, and figure out we messed with QOM.
Or we can use g_assert() which abort, displaying the filename and line number, so we can quickly open our favorite IDE. Prefer the later, to save time to developers.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
v2: New patch

---
 hw/core/qdev.c | 1 +
 1 file changed, 1 insertion(+)

hw/core/qdev.c +1 -0

index 58e87d336d..d30cf6320b 100644
@@ -1143,6 +1143,7 @@ Object *qdev_get_machine(void)
 
     if (dev == NULL) {
         dev = container_get(object_get_root(), "/machine");
+        g_assert(dev != NULL);
     }
 
     return dev;

Signature · 1 line

2.21.1

Markus Armbruster

2020-01-21 13:54 UTC #12 lore raw

Re: [PATCH v2 07/10] qdev: Abort if the root machine container is missing

Quoted 22 lines · Philippe Mathieu-Daudé

The QEMU device API (qdev) relies on having the '/machine' container always available.
If it is missing, QEMU will later crash dereferencing a NULL pointer, we will get a SEGV, open a debugger, look at the backtrace, and figure out we messed with QOM.
Or we can use g_assert() which abort, displaying the filename and line number, so we can quickly open our favorite IDE. Prefer the later, to save time to developers.
Signed-off-by Philippe Mathieu-Daudé <[email protected]>
---
v2: New patch
---
 hw/core/qdev.c | 1 +
 1 file changed, 1 insertion(+)
hw/core/qdev.c +0 -0
index 58e87d336d..d30cf6320b 100644

hw/core/qdev.c Object *qdev_get_machine(void) · line 1149

Quoted 7 lines

hw/core/qdev.c +1 -0

@@ -1143,6 +1143,7 @@ Object *qdev_get_machine(void)
 
     if (dev == NULL) {
         dev = container_get(object_get_root(), "/machine");
+        g_assert(dev != NULL);
     }
 
     return dev;

container_get()'s contract promises it won't return null. I think the assertion belongs there instead.

Philippe Mathieu-Daudé

author

2020-01-21 11:09 UTC #13 lore raw

PATCH v2 8/10 accel: Introduce the current_accel() wrapper

The accel/ code only access the MachineState::accel field. As we simply want to access the accelerator, not the machine, add a current_accel() wrapper.

Reviewed-by Alistair Francis <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
v2: Reworded description using 'wrapper'

---
 include/sysemu/accel.h | 2 ++
 accel/accel.c          | 5 +++++
 2 files changed, 7 insertions(+)

include/sysemu/accel.h +2 -0

index d4c1429711..47e5788530 100644
@@ -70,4 +70,6 @@ int accel_init_machine(AccelState *accel, MachineState *ms);
 /* Called just before os_setup_post (ie just before drop OS privs) */
 void accel_setup_post(MachineState *ms);
 
+AccelState *current_accel(void);
+
 #endif

accel/accel.c +5 -0

index 1c5c3a6abb..cb555e3b06 100644
@@ -63,6 +63,11 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
     return ret;
 }
 
+AccelState *current_accel(void)
+{
+    return current_machine->accelerator;
+}
+
 void accel_setup_post(MachineState *ms)
 {
     AccelState *accel = ms->accelerator;

Signature · 1 line

2.21.1

Cornelia Huck

Reviewed-by on v2 8/10

2020-01-21 14:30 UTC #14 lore raw

Re: [PATCH v2 08/10] accel: Introduce the current_accel() wrapper

Quoted 1 line · On Tue, 21 Jan 2020 12:03:47 +0100 Philippe Mathieu-Daudé <[email protected]>…

The accel/ code only access the MachineState::accel field.

s/access/accesses/

Quoted 6 lines

As we simply want to access the accelerator, not the machine, add a current_accel() wrapper.
Suggested-by Paolo Bonzini <[email protected]>
Reviewed-by Alistair Francis <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>

Quoted 6 lines

---
v2: Reworded description using 'wrapper'

---
 include/sysemu/accel.h | 2 ++
 accel/accel.c          | 5 +++++
 2 files changed, 7 insertions(+)

Philippe Mathieu-Daudé

author

2020-01-21 11:05 UTC #15 lore raw

PATCH v2 9/10 accel: Replace current_machine->accelerator by current_accel() wrapper

We actually want to access the accelerator, not the machine, so use the current_accel() wrapper instead.

Reviewed-by Alistair Francis <[email protected]>

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
v2:
- Reworded description
- Remove unused include in arm/kvm64

---
 accel/kvm/kvm-all.c | 4 ++--
 accel/tcg/tcg-all.c | 2 +-
 memory.c            | 2 +-
 target/arm/kvm64.c  | 5 ++---
 target/i386/kvm.c   | 2 +-
 target/ppc/kvm.c    | 2 +-
 vl.c                | 2 +-
 7 files changed, 9 insertions(+), 10 deletions(-)

accel/kvm/kvm-all.c +2 -2

index 1ada2f4ecb..c111312dfd 100644
@@ -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;
 }

@@ -1848,7 +1848,7 @@ static int kvm_max_vcpu_id(KVMState *s)
 
 bool kvm_vcpu_id_is_valid(int vcpu_id)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
     return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s);
 }

accel/tcg/tcg-all.c +1 -1

index 1dc384c8d2..1802ce02f6 100644
@@ -124,7 +124,7 @@ static void tcg_accel_instance_init(Object *obj)
 
 static int tcg_init(MachineState *ms)
 {
-    TCGState *s = TCG_STATE(current_machine->accelerator);
+    TCGState *s = TCG_STATE(current_accel());
 
     tcg_exec_init(s->tb_size * 1024 * 1024);
     cpu_interrupt_handler = tcg_handle_interrupt;

memory.c +1 -1

index d7b9bb6951..854798791e 100644
@@ -3104,7 +3104,7 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner)
         };
         GArray *fv_address_spaces;
         GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
-        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
+        AccelClass *ac = ACCEL_GET_CLASS(current_accel());
 
         if (ac->has_memory) {
             fvi.ac = ac;

target/arm/kvm64.c +2 -3

index 876184b8fe..e3c580e749 100644
@@ -26,7 +26,6 @@
 #include "sysemu/kvm.h"
 #include "sysemu/kvm_int.h"
 #include "kvm_arm.h"
-#include "hw/boards.h"
 #include "internals.h"
 
 static bool have_guest_debug;

@@ -613,14 +612,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 
 bool kvm_arm_aarch32_supported(CPUState *cpu)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
 
     return kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
 }
 
 bool kvm_arm_sve_supported(CPUState *cpu)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
 
     return kvm_check_extension(s, KVM_CAP_ARM_SVE);
 }

target/i386/kvm.c +1 -1

index 7ee3202634..eddb930065 100644
@@ -147,7 +147,7 @@ bool kvm_allows_irq0_override(void)
 
 static bool kvm_x2apic_api_set_flags(uint64_t flags)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
 
     return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
 }

target/ppc/kvm.c +1 -1

index b5799e62b4..45ede6b6d9 100644
@@ -258,7 +258,7 @@ static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp)
 
 struct ppc_radix_page_info *kvm_get_radix_page_info(void)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
     struct ppc_radix_page_info *radix_page_info;
     struct kvm_ppc_rmmu_info rmmu_info;
     int i;

vl.c +1 -1

index 71d3e7eefb..a8ea36f4f8 100644
@@ -2812,7 +2812,7 @@ static void configure_accelerators(const char *progname)
     }
 
     if (init_failed) {
-        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
+        AccelClass *ac = ACCEL_GET_CLASS(current_accel());
         error_report("falling back to %s", ac->name);
     }

Signature · 1 line

2.21.1

Cornelia Huck

2020-01-21 14:04 UTC #16 lore raw

Re: [PATCH v2 09/10] accel: Replace current_machine->accelerator by current_accel() wrapper

Quoted 25 lines · On Tue, 21 Jan 2020 12:03:48 +0100 Philippe Mathieu-Daudé <[email protected]>…

We actually want to access the accelerator, not the machine, so use the current_accel() wrapper instead.
Suggested-by Paolo Bonzini <[email protected]>
Reviewed-by Alistair Francis <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>
---
v2:
- Reworded description
- Remove unused include in arm/kvm64
---
 accel/kvm/kvm-all.c | 4 ++--
 accel/tcg/tcg-all.c | 2 +-
 memory.c            | 2 +-
 target/arm/kvm64.c  | 5 ++---
 target/i386/kvm.c   | 2 +-
 target/ppc/kvm.c    | 2 +-
 vl.c                | 2 +-
 7 files changed, 9 insertions(+), 10 deletions(-)
target/ppc/kvm.c +0 -0
index b5799e62b4..45ede6b6d9 100644

target/ppc/kvm.c static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp) · line 264

Quoted 8 lines

target/ppc/kvm.c +1 -1

@@ -258,7 +258,7 @@ static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp)
 
 struct ppc_radix_page_info *kvm_get_radix_page_info(void)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
     struct ppc_radix_page_info *radix_page_info;
     struct kvm_ppc_rmmu_info rmmu_info;
     int i;

What about the usage in kvmppc_svm_off()?

David Gibson

Acked-by on v2 9/10

2020-01-22 03:29 UTC #17 lore raw

Re: [PATCH v2 09/10] accel: Replace current_machine->accelerator by current_accel() wrapper

Quoted 6 lines · Philippe Mathieu-Daudé

We actually want to access the accelerator, not the machine, so use the current_accel() wrapper instead.
Suggested-by Paolo Bonzini <[email protected]>
Reviewed-by Alistair Francis <[email protected]>
Signed-off-by Philippe Mathieu-Daudé <[email protected]>

ppc parts

Quoted 130 lines

---
v2:
- Reworded description
- Remove unused include in arm/kvm64

---
 accel/kvm/kvm-all.c | 4 ++--
 accel/tcg/tcg-all.c | 2 +-
 memory.c            | 2 +-
 target/arm/kvm64.c  | 5 ++---
 target/i386/kvm.c   | 2 +-
 target/ppc/kvm.c    | 2 +-
 vl.c                | 2 +-
 7 files changed, 9 insertions(+), 10 deletions(-)

accel/kvm/kvm-all.c +2 -2

index 1ada2f4ecb..c111312dfd 100644
@@ -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;
 }

@@ -1848,7 +1848,7 @@ static int kvm_max_vcpu_id(KVMState *s)
 
 bool kvm_vcpu_id_is_valid(int vcpu_id)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
     return vcpu_id >= 0 && vcpu_id < kvm_max_vcpu_id(s);
 }

accel/tcg/tcg-all.c +1 -1

index 1dc384c8d2..1802ce02f6 100644
@@ -124,7 +124,7 @@ static void tcg_accel_instance_init(Object *obj)
 
 static int tcg_init(MachineState *ms)
 {
-    TCGState *s = TCG_STATE(current_machine->accelerator);
+    TCGState *s = TCG_STATE(current_accel());
 
     tcg_exec_init(s->tb_size * 1024 * 1024);
     cpu_interrupt_handler = tcg_handle_interrupt;

memory.c +1 -1

index d7b9bb6951..854798791e 100644
@@ -3104,7 +3104,7 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner)
         };
         GArray *fv_address_spaces;
         GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
-        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
+        AccelClass *ac = ACCEL_GET_CLASS(current_accel());
 
         if (ac->has_memory) {
             fvi.ac = ac;

target/arm/kvm64.c +2 -3

index 876184b8fe..e3c580e749 100644
@@ -26,7 +26,6 @@
 #include "sysemu/kvm.h"
 #include "sysemu/kvm_int.h"
 #include "kvm_arm.h"
-#include "hw/boards.h"
 #include "internals.h"
 
 static bool have_guest_debug;

@@ -613,14 +612,14 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 
 bool kvm_arm_aarch32_supported(CPUState *cpu)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
 
     return kvm_check_extension(s, KVM_CAP_ARM_EL1_32BIT);
 }
 
 bool kvm_arm_sve_supported(CPUState *cpu)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
 
     return kvm_check_extension(s, KVM_CAP_ARM_SVE);
 }

target/i386/kvm.c +1 -1

index 7ee3202634..eddb930065 100644
@@ -147,7 +147,7 @@ bool kvm_allows_irq0_override(void)
 
 static bool kvm_x2apic_api_set_flags(uint64_t flags)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
 
     return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags);
 }

target/ppc/kvm.c +1 -1

index b5799e62b4..45ede6b6d9 100644
@@ -258,7 +258,7 @@ static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp)
 
 struct ppc_radix_page_info *kvm_get_radix_page_info(void)
 {
-    KVMState *s = KVM_STATE(current_machine->accelerator);
+    KVMState *s = KVM_STATE(current_accel());
     struct ppc_radix_page_info *radix_page_info;
     struct kvm_ppc_rmmu_info rmmu_info;
     int i;

vl.c +1 -1

index 71d3e7eefb..a8ea36f4f8 100644
@@ -2812,7 +2812,7 @@ static void configure_accelerators(const char *progname)
     }
 
     if (init_failed) {
-        AccelClass *ac = ACCEL_GET_CLASS(current_machine->accelerator);
+        AccelClass *ac = ACCEL_GET_CLASS(current_accel());
         error_report("falling back to %s", ac->name);
     }

Signature · 4 lines

David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Philippe Mathieu-Daudé

author

2020-01-21 11:11 UTC #18 lore raw

PATCH v2 10/10 accel/tcg: Sanitize include path

Commit af0440ae852 moved the qemu_tcg_configure() function, but introduced extraneous 'include/' in the includes path. As it is not necessary, remove it.

Signed-off-by Philippe Mathieu-Daudé <[email protected]>

---
v2: New patch

---
 accel/tcg/tcg-all.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

accel/tcg/tcg-all.c +3 -3

index 1802ce02f6..acfdcfdf59 100644
@@ -31,9 +31,9 @@
 #include "sysemu/cpus.h"
 #include "qemu/main-loop.h"
 #include "tcg/tcg.h"
-#include "include/qapi/error.h"
-#include "include/qemu/error-report.h"
-#include "include/hw/boards.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "hw/boards.h"
 #include "qapi/qapi-builtin-visit.h"
 
 typedef struct TCGState {

Signature · 1 line

2.21.1

Cornelia Huck

Reviewed-by on v2 10/10

2020-01-21 14:34 UTC #19 lore raw

Re: [PATCH v2 10/10] accel/tcg: Sanitize include path

Quoted 18 lines · On Tue, 21 Jan 2020 12:03:49 +0100 Philippe Mathieu-Daudé <[email protected]>…

Commit af0440ae852 moved the qemu_tcg_configure() function, but introduced extraneous 'include/' in the includes path. As it is not necessary, remove it.
Signed-off-by Philippe Mathieu-Daudé <[email protected]>
---
v2: New patch
---
 accel/tcg/tcg-all.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
accel/tcg/tcg-all.c +0 -0
index 1802ce02f6..acfdcfdf59 100644
@@ -31,9 +31,9 @@
 #include "sysemu/cpus.h"
 #include "qemu/main-loop.h"

accel/tcg/tcg-all.c line 39

Quoted 10 lines

accel/tcg/tcg-all.c +3 -3

 #include "tcg/tcg.h"
-#include "include/qapi/error.h"
-#include "include/qemu/error-report.h"
-#include "include/hw/boards.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "hw/boards.h"
 #include "qapi/qapi-builtin-visit.h"
 
 typedef struct TCGState {

Markus Armbruster

2020-01-21 13:33 UTC #20 lore raw

Quoted 8 lines · Philippe Mathieu-Daudé

v1 was "Replace current_machine by qdev_get_machine()":
https://www.mail-archive.com/[email protected]/msg669611.html
But Markus objected, see:
https://www.mail-archive.com/[email protected]/msg670122.html
and older discussion:
https://www.mail-archive.com/[email protected]/msg611338.html

Quoted 6 lines

This series salvage patches from v1, and add other trivial cleanups.
Can the ARM/PPC/S390 patches could go via their own tree, and the rest via Paolo's 'misc' tree?
Supersedes: <[email protected]>

Separating off uncontroversial patches is always a good idea. These all look okay to me on first glance, with one minor remark on PATCH 7. I think you got or are likely to get competent review for all of them. If you need me to have a closer look, just ask.

Thanks!

Philippe Mathieu-Daudé

author

2020-01-21 13:36 UTC #21 lore raw

Quoted 15 lines · Markus Armbruster

Quoted 13 lines · Philippe Mathieu-Daudé
v1 was "Replace current_machine by qdev_get_machine()":
https://www.mail-archive.com/[email protected]/msg669611.html
But Markus objected, see:
https://www.mail-archive.com/[email protected]/msg670122.html
and older discussion:
https://www.mail-archive.com/[email protected]/msg611338.html
This series salvage patches from v1, and add other trivial cleanups.
Can the ARM/PPC/S390 patches could go via their own tree, and the rest via Paolo's 'misc' tree?

Quoted 6 lines

Quoted 1 line
Supersedes: <[email protected]>
Separating off uncontroversial patches is always a good idea. These all look okay to me on first glance, with one minor remark on PATCH 7. I think you got or are likely to get competent review for all of them. If you need me to have a closer look, just ask.

Good :) Thanks!

Paolo Bonzini

maintainer

2020-01-21 16:28 UTC #22 lore raw