[Qemu-devel] [PATCH v2 0/3] exec.c: avoid iterating through CPUs in tcg_commit()

[Qemu-devel] [PATCH v2 0/3] exec.c: avoid iterating through CPUs in tcg_commit()

[Peter Maydell]

This patchset is a bit of groundwork heading in the direction
of supporting multiple AddressSpaces per CPU. It does actually
do something useful (handle a FIXME remark in tcg_commit()) so
I've put it out for that reason.

Patch 1 is just removing an unnecessary call to cpu_reload_memory_map()
to make the cleanup a bit easier.
Patch 2 is a comment change to clarify what's going on in
cpu_reload_memory_map().
Patch 3 is the actual work here: we collect up the fields that
were directly in CPUState but relating to our one-and-only
AddressSpace, and put them in a new struct CPUAddressSpace
CPUState now has a pointer to an array of these (with only one
entry for now). The rearrangement lets us fix the tcg_commit()
FIXME remark by going directly to the affected CPUState from
the MemoryListener pointer rather than having to search all CPUs
for it.


Changes v1->v2: just rebased.

Peter Maydell (3):
  exec.c: Don't call cpu_reload_memory_map() from cpu_exec_init()
  cpu-exec-common.c: Clarify comment about cpu_reload_memory_map()'s RCU
    operations
  exec.c: Collect AddressSpace related fields into a CPUAddressSpace
    struct

 cpu-exec-common.c       | 33 ++++++++++++++--------------
 exec.c                  | 57 ++++++++++++++++++++++++++++++++-----------------
 include/exec/exec-all.h |  2 +-
 include/qemu/typedefs.h |  1 +
 include/qom/cpu.h       |  7 ++++--
 5 files changed, 62 insertions(+), 38 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/3] exec.c: Don't call cpu_reload_memory_map() from cpu_exec_init()

[Peter Maydell]

Currently we call cpu_reload_memory_map() from cpu_exec_init(),
but this is not necessary:
 * KVM doesn't use the data structures maintained by
   cpu_reload_memory_map() (the TLB and cpu->memory_dispatch)
 * for TCG, we will call this function via tcg_commit() either
   as soon as tcg_cpu_address_space_init() registers the listener,
   or when the first MemoryRegion is added to the AddressSpace
   if the AS is empty when we register the listener

The unnecessary call is awkward for adding support for multiple
address spaces per CPU, so drop it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
---
 exec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/exec.c b/exec.c
index 47ada31..f048c23 100644
--- a/exec.c
+++ b/exec.c
@@ -598,7 +598,6 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
 #ifndef CONFIG_USER_ONLY
     cpu->as = &address_space_memory;
     cpu->thread_id = qemu_get_thread_id();
-    cpu_reload_memory_map(cpu);
 #endif
 
 #if defined(CONFIG_USER_ONLY)
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/3] cpu-exec-common.c: Clarify comment about cpu_reload_memory_map()'s RCU operations

[Peter Maydell]

The reason for cpu_reload_memory_map()'s RCU operations is not
so much because the guest could make the critical section very
long, but that it could have a critical section within which
it made an arbitrary number of changes to the memory map and
thus accumulate an unbounded amount of memory data structures
awaiting reclamation. Clarify the comment to make this clearer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
---
 cpu-exec-common.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/cpu-exec-common.c b/cpu-exec-common.c
index 16d305b..b95b09a 100644
--- a/cpu-exec-common.c
+++ b/cpu-exec-common.c
@@ -42,13 +42,21 @@ void cpu_reload_memory_map(CPUState *cpu)
     AddressSpaceDispatch *d;
 
     if (qemu_in_vcpu_thread()) {
-        /* Do not let the guest prolong the critical section as much as it
-         * as it desires.
+        /* The guest can in theory prolong the RCU critical section as long
+         * as it feels like. The major problem with this is that because it
+         * can do multiple reconfigurations of the memory map within the
+         * critical section, we could potentially accumulate an unbounded
+         * collection of memory data structures awaiting reclamation.
          *
-         * Currently, this is prevented by the I/O thread's periodinc kicking
-         * of the VCPU thread (iothread_requesting_mutex, qemu_cpu_kick_thread)
-         * but this will go away once TCG's execution moves out of the global
-         * mutex.
+         * Because the only thing we're currently protecting with RCU is the
+         * memory data structures, it's sufficient to break the critical section
+         * in this callback, which we know will get called every time the
+         * memory map is rearranged.
+         *
+         * (If we add anything else in the system that uses RCU to protect
+         * its data structures, we will need to implement some other mechanism
+         * to force TCG CPUs to exit the critical section, at which point this
+         * part of this callback might become unnecessary.)
          *
          * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
          * only protects cpu->as->dispatch.  Since we reload it below, we can
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/3] exec.c: Collect AddressSpace related fields into a CPUAddressSpace struct

[Peter Maydell]

Gather up all the fields currently in CPUState which deal with the CPU's
AddressSpace into a separate CPUAddressSpace struct. This paves the way
for allowing the CPU to know about more than one AddressSpace.

The rearrangement also allows us to make the MemoryListener a directly
embedded object in the CPUAddressSpace (it could not be embedded in
CPUState because 'struct MemoryListener' isn't defined for the user-only
builds). This allows us to resolve the FIXME in tcg_commit() by going
directly from the MemoryListener to the CPUAddressSpace.

This patch extracts the actual update of the cached dispatch pointer
from cpu_reload_memory_map() (which is renamed accordingly to
cpu_reloading_memory_map() as it is only responsible for breaking
cpu-exec.c's RCU critical section now). This lets us keep the definition
of the CPUAddressSpace struct private to exec.c.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpu-exec-common.c       | 13 +++---------
 exec.c                  | 56 +++++++++++++++++++++++++++++++++----------------
 include/exec/exec-all.h |  2 +-
 include/qemu/typedefs.h |  1 +
 include/qom/cpu.h       |  7 +++++--
 5 files changed, 48 insertions(+), 31 deletions(-)

diff --git a/cpu-exec-common.c b/cpu-exec-common.c
index b95b09a..43edf36 100644
--- a/cpu-exec-common.c
+++ b/cpu-exec-common.c
@@ -37,10 +37,8 @@ void cpu_resume_from_signal(CPUState *cpu, void *puc)
     siglongjmp(cpu->jmp_env, 1);
 }
 
-void cpu_reload_memory_map(CPUState *cpu)
+void cpu_reloading_memory_map(void)
 {
-    AddressSpaceDispatch *d;
-
     if (qemu_in_vcpu_thread()) {
         /* The guest can in theory prolong the RCU critical section as long
          * as it feels like. The major problem with this is that because it
@@ -59,17 +57,12 @@ void cpu_reload_memory_map(CPUState *cpu)
          * part of this callback might become unnecessary.)
          *
          * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
-         * only protects cpu->as->dispatch.  Since we reload it below, we can
-         * split the critical section.
+         * only protects cpu->as->dispatch. Since we know our caller is about
+         * to reload it, it's safe to split the critical section.
          */
         rcu_read_unlock();
         rcu_read_lock();
     }
-
-    /* The CPU and TLB are protected by the iothread lock.  */
-    d = atomic_rcu_read(&cpu->as->dispatch);
-    cpu->memory_dispatch = d;
-    tlb_flush(cpu, 1);
 }
 #endif
 
diff --git a/exec.c b/exec.c
index f048c23..5ad0317 100644
--- a/exec.c
+++ b/exec.c
@@ -158,6 +158,21 @@ static void memory_map_init(void);
 static void tcg_commit(MemoryListener *listener);
 
 static MemoryRegion io_mem_watch;
+
+/**
+ * CPUAddressSpace: all the information a CPU needs about an AddressSpace
+ * @cpu: the CPU whose AddressSpace this is
+ * @as: the AddressSpace itself
+ * @memory_dispatch: its dispatch pointer (cached, RCU protected)
+ * @tcg_as_listener: listener for tracking changes to the AddressSpace
+ */
+struct CPUAddressSpace {
+    CPUState *cpu;
+    AddressSpace *as;
+    struct AddressSpaceDispatch *memory_dispatch;
+    MemoryListener tcg_as_listener;
+};
+
 #endif
 
 #if !defined(CONFIG_USER_ONLY)
@@ -428,7 +443,7 @@ address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr,
                                   hwaddr *xlat, hwaddr *plen)
 {
     MemoryRegionSection *section;
-    section = address_space_translate_internal(cpu->memory_dispatch,
+    section = address_space_translate_internal(cpu->cpu_ases[0].memory_dispatch,
                                                addr, xlat, plen, false);
 
     assert(!section->mr->iommu_ops);
@@ -534,13 +549,16 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
     /* We only support one address space per cpu at the moment.  */
     assert(cpu->as == as);
 
-    if (cpu->tcg_as_listener) {
-        memory_listener_unregister(cpu->tcg_as_listener);
-    } else {
-        cpu->tcg_as_listener = g_new0(MemoryListener, 1);
+    if (cpu->cpu_ases) {
+        /* We've already registered the listener for our only AS */
+        return;
     }
-    cpu->tcg_as_listener->commit = tcg_commit;
-    memory_listener_register(cpu->tcg_as_listener, as);
+
+    cpu->cpu_ases = g_new0(CPUAddressSpace, 1);
+    cpu->cpu_ases[0].cpu = cpu;
+    cpu->cpu_ases[0].as = as;
+    cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit;
+    memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as);
 }
 #endif
 
@@ -2182,7 +2200,8 @@ static uint16_t dummy_section(PhysPageMap *map, AddressSpace *as,
 
 MemoryRegion *iotlb_to_region(CPUState *cpu, hwaddr index)
 {
-    AddressSpaceDispatch *d = atomic_rcu_read(&cpu->memory_dispatch);
+    CPUAddressSpace *cpuas = &cpu->cpu_ases[0];
+    AddressSpaceDispatch *d = atomic_rcu_read(&cpuas->memory_dispatch);
     MemoryRegionSection *sections = d->map.sections;
 
     return sections[index & ~TARGET_PAGE_MASK].mr;
@@ -2241,19 +2260,20 @@ static void mem_commit(MemoryListener *listener)
 
 static void tcg_commit(MemoryListener *listener)
 {
-    CPUState *cpu;
+    CPUAddressSpace *cpuas;
+    AddressSpaceDispatch *d;
 
     /* since each CPU stores ram addresses in its TLB cache, we must
        reset the modified entries */
-    /* XXX: slow ! */
-    CPU_FOREACH(cpu) {
-        /* FIXME: Disentangle the cpu.h circular files deps so we can
-           directly get the right CPU from listener.  */
-        if (cpu->tcg_as_listener != listener) {
-            continue;
-        }
-        cpu_reload_memory_map(cpu);
-    }
+    cpuas = container_of(listener, CPUAddressSpace, tcg_as_listener);
+    cpu_reloading_memory_map();
+    /* The CPU and TLB are protected by the iothread lock.
+     * We reload the dispatch pointer now because cpu_reloading_memory_map()
+     * may have split the RCU critical section.
+     */
+    d = atomic_rcu_read(&cpuas->as->dispatch);
+    cpuas->memory_dispatch = d;
+    tlb_flush(cpuas->cpu, 1);
 }
 
 void address_space_init_dispatch(AddressSpace *as)
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index a3719b7..0e7480c 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -94,7 +94,7 @@ void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
 
 #if !defined(CONFIG_USER_ONLY)
 bool qemu_in_vcpu_thread(void);
-void cpu_reload_memory_map(CPUState *cpu);
+void cpu_reloading_memory_map(void);
 void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as);
 /* cputlb.c */
 /**
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 3a835ff..544b780 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -16,6 +16,7 @@ typedef struct BusClass BusClass;
 typedef struct BusState BusState;
 typedef struct CharDriverState CharDriverState;
 typedef struct CompatProperty CompatProperty;
+typedef struct CPUAddressSpace CPUAddressSpace;
 typedef struct DeviceState DeviceState;
 typedef struct DeviceListener DeviceListener;
 typedef struct DisplayChangeListener DisplayChangeListener;
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 9405554..231430b 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -234,6 +234,10 @@ struct kvm_run;
  * @can_do_io: Nonzero if memory-mapped IO is safe. Deterministic execution
  * requires that IO only be performed on the last instruction of a TB
  * so that interrupts take effect immediately.
+ * @cpu_ases: Pointer to array of CPUAddressSpaces (which define the
+ *            AddressSpaces this CPU has)
+ * @as: Pointer to the first AddressSpace, for the convenience of targets which
+ *      only have a single AddressSpace
  * @env_ptr: Pointer to subclass-specific CPUArchState field.
  * @current_tb: Currently executing TB.
  * @gdb_regs: Additional GDB registers.
@@ -280,9 +284,8 @@ struct CPUState {
     QemuMutex work_mutex;
     struct qemu_work_item *queued_work_first, *queued_work_last;
 
+    CPUAddressSpace *cpu_ases;
     AddressSpace *as;
-    struct AddressSpaceDispatch *memory_dispatch;
-    MemoryListener *tcg_as_listener;
 
     void *env_ptr; /* CPUArchState */
     struct TranslationBlock *current_tb;
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 3/3] exec.c: Collect AddressSpace related fields into a CPUAddressSpace struct

[Edgar E. Iglesias]

On Thu, Oct 01, 2015 at 03:29:50PM +0100, Peter Maydell wrote:
> Gather up all the fields currently in CPUState which deal with the CPU's
> AddressSpace into a separate CPUAddressSpace struct. This paves the way
> for allowing the CPU to know about more than one AddressSpace.
> 
> The rearrangement also allows us to make the MemoryListener a directly
> embedded object in the CPUAddressSpace (it could not be embedded in
> CPUState because 'struct MemoryListener' isn't defined for the user-only
> builds). This allows us to resolve the FIXME in tcg_commit() by going
> directly from the MemoryListener to the CPUAddressSpace.
> 
> This patch extracts the actual update of the cached dispatch pointer
> from cpu_reload_memory_map() (which is renamed accordingly to
> cpu_reloading_memory_map() as it is only responsible for breaking
> cpu-exec.c's RCU critical section now). This lets us keep the definition
> of the CPUAddressSpace struct private to exec.c.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  cpu-exec-common.c       | 13 +++---------
>  exec.c                  | 56 +++++++++++++++++++++++++++++++++----------------
>  include/exec/exec-all.h |  2 +-
>  include/qemu/typedefs.h |  1 +
>  include/qom/cpu.h       |  7 +++++--
>  5 files changed, 48 insertions(+), 31 deletions(-)
> 
> diff --git a/cpu-exec-common.c b/cpu-exec-common.c
> index b95b09a..43edf36 100644
> --- a/cpu-exec-common.c
> +++ b/cpu-exec-common.c
> @@ -37,10 +37,8 @@ void cpu_resume_from_signal(CPUState *cpu, void *puc)
>      siglongjmp(cpu->jmp_env, 1);
>  }
>  
> -void cpu_reload_memory_map(CPUState *cpu)
> +void cpu_reloading_memory_map(void)
>  {
> -    AddressSpaceDispatch *d;
> -
>      if (qemu_in_vcpu_thread()) {
>          /* The guest can in theory prolong the RCU critical section as long
>           * as it feels like. The major problem with this is that because it
> @@ -59,17 +57,12 @@ void cpu_reload_memory_map(CPUState *cpu)
>           * part of this callback might become unnecessary.)
>           *
>           * This pair matches cpu_exec's rcu_read_lock()/rcu_read_unlock(), which
> -         * only protects cpu->as->dispatch.  Since we reload it below, we can
> -         * split the critical section.
> +         * only protects cpu->as->dispatch. Since we know our caller is about
> +         * to reload it, it's safe to split the critical section.
>           */
>          rcu_read_unlock();
>          rcu_read_lock();
>      }
> -
> -    /* The CPU and TLB are protected by the iothread lock.  */
> -    d = atomic_rcu_read(&cpu->as->dispatch);
> -    cpu->memory_dispatch = d;
> -    tlb_flush(cpu, 1);
>  }
>  #endif
>  
> diff --git a/exec.c b/exec.c
> index f048c23..5ad0317 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -158,6 +158,21 @@ static void memory_map_init(void);
>  static void tcg_commit(MemoryListener *listener);
>  
>  static MemoryRegion io_mem_watch;
> +
> +/**
> + * CPUAddressSpace: all the information a CPU needs about an AddressSpace
> + * @cpu: the CPU whose AddressSpace this is
> + * @as: the AddressSpace itself
> + * @memory_dispatch: its dispatch pointer (cached, RCU protected)
> + * @tcg_as_listener: listener for tracking changes to the AddressSpace
> + */
> +struct CPUAddressSpace {
> +    CPUState *cpu;
> +    AddressSpace *as;
> +    struct AddressSpaceDispatch *memory_dispatch;
> +    MemoryListener tcg_as_listener;
> +};
> +
>  #endif
>  
>  #if !defined(CONFIG_USER_ONLY)
> @@ -428,7 +443,7 @@ address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr,
>                                    hwaddr *xlat, hwaddr *plen)
>  {
>      MemoryRegionSection *section;
> -    section = address_space_translate_internal(cpu->memory_dispatch,
> +    section = address_space_translate_internal(cpu->cpu_ases[0].memory_dispatch,
>                                                 addr, xlat, plen, false);
>  
>      assert(!section->mr->iommu_ops);
> @@ -534,13 +549,16 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
>      /* We only support one address space per cpu at the moment.  */
>      assert(cpu->as == as);
>  
> -    if (cpu->tcg_as_listener) {
> -        memory_listener_unregister(cpu->tcg_as_listener);
> -    } else {
> -        cpu->tcg_as_listener = g_new0(MemoryListener, 1);
> +    if (cpu->cpu_ases) {
> +        /* We've already registered the listener for our only AS */
> +        return;
>      }
> -    cpu->tcg_as_listener->commit = tcg_commit;
> -    memory_listener_register(cpu->tcg_as_listener, as);
> +
> +    cpu->cpu_ases = g_new0(CPUAddressSpace, 1);
> +    cpu->cpu_ases[0].cpu = cpu;
> +    cpu->cpu_ases[0].as = as;
> +    cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit;
> +    memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as);
>  }
>  #endif
>  
> @@ -2182,7 +2200,8 @@ static uint16_t dummy_section(PhysPageMap *map, AddressSpace *as,
>  
>  MemoryRegion *iotlb_to_region(CPUState *cpu, hwaddr index)
>  {
> -    AddressSpaceDispatch *d = atomic_rcu_read(&cpu->memory_dispatch);
> +    CPUAddressSpace *cpuas = &cpu->cpu_ases[0];
> +    AddressSpaceDispatch *d = atomic_rcu_read(&cpuas->memory_dispatch);
>      MemoryRegionSection *sections = d->map.sections;
>  
>      return sections[index & ~TARGET_PAGE_MASK].mr;
> @@ -2241,19 +2260,20 @@ static void mem_commit(MemoryListener *listener)
>  
>  static void tcg_commit(MemoryListener *listener)
>  {
> -    CPUState *cpu;
> +    CPUAddressSpace *cpuas;
> +    AddressSpaceDispatch *d;
>  
>      /* since each CPU stores ram addresses in its TLB cache, we must
>         reset the modified entries */
> -    /* XXX: slow ! */
> -    CPU_FOREACH(cpu) {
> -        /* FIXME: Disentangle the cpu.h circular files deps so we can
> -           directly get the right CPU from listener.  */
> -        if (cpu->tcg_as_listener != listener) {
> -            continue;
> -        }
> -        cpu_reload_memory_map(cpu);
> -    }
> +    cpuas = container_of(listener, CPUAddressSpace, tcg_as_listener);
> +    cpu_reloading_memory_map();
> +    /* The CPU and TLB are protected by the iothread lock.
> +     * We reload the dispatch pointer now because cpu_reloading_memory_map()
> +     * may have split the RCU critical section.
> +     */
> +    d = atomic_rcu_read(&cpuas->as->dispatch);
> +    cpuas->memory_dispatch = d;
> +    tlb_flush(cpuas->cpu, 1);
>  }
>  
>  void address_space_init_dispatch(AddressSpace *as)
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index a3719b7..0e7480c 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -94,7 +94,7 @@ void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
>  
>  #if !defined(CONFIG_USER_ONLY)
>  bool qemu_in_vcpu_thread(void);
> -void cpu_reload_memory_map(CPUState *cpu);
> +void cpu_reloading_memory_map(void);
>  void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as);
>  /* cputlb.c */
>  /**
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 3a835ff..544b780 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -16,6 +16,7 @@ typedef struct BusClass BusClass;
>  typedef struct BusState BusState;
>  typedef struct CharDriverState CharDriverState;
>  typedef struct CompatProperty CompatProperty;
> +typedef struct CPUAddressSpace CPUAddressSpace;
>  typedef struct DeviceState DeviceState;
>  typedef struct DeviceListener DeviceListener;
>  typedef struct DisplayChangeListener DisplayChangeListener;
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 9405554..231430b 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -234,6 +234,10 @@ struct kvm_run;
>   * @can_do_io: Nonzero if memory-mapped IO is safe. Deterministic execution
>   * requires that IO only be performed on the last instruction of a TB
>   * so that interrupts take effect immediately.
> + * @cpu_ases: Pointer to array of CPUAddressSpaces (which define the
> + *            AddressSpaces this CPU has)
> + * @as: Pointer to the first AddressSpace, for the convenience of targets which
> + *      only have a single AddressSpace
>   * @env_ptr: Pointer to subclass-specific CPUArchState field.
>   * @current_tb: Currently executing TB.
>   * @gdb_regs: Additional GDB registers.
> @@ -280,9 +284,8 @@ struct CPUState {
>      QemuMutex work_mutex;
>      struct qemu_work_item *queued_work_first, *queued_work_last;
>  
> +    CPUAddressSpace *cpu_ases;
>      AddressSpace *as;
> -    struct AddressSpaceDispatch *memory_dispatch;
> -    MemoryListener *tcg_as_listener;
>  
>      void *env_ptr; /* CPUArchState */
>      struct TranslationBlock *current_tb;
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v2 3/3] exec.c: Collect AddressSpace related fields into a CPUAddressSpace struct

[Richard Henderson]

On 10/02/2015 12:29 AM, Peter Maydell wrote:
> +    cpu->cpu_ases = g_new0(CPUAddressSpace, 1);
> +    cpu->cpu_ases[0].cpu = cpu;
> +    cpu->cpu_ases[0].as = as;
> +    cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit;
> +    memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as);
>   }

What's the plan when it's more than one?

Just thinking about why separate allocation vs embedding an array.  Though 
possibly with the CPUState member being a pointer to an array within the 
TargetCPUClass, or CPUTargetState.  Dunno.

All that said, what you've got works.


r~

Re: [Qemu-devel] [PATCH v2 3/3] exec.c: Collect AddressSpace related fields into a CPUAddressSpace struct

[Peter Maydell]

On 7 October 2015 at 10:57, Richard Henderson <rth@twiddle.net> wrote:
> On 10/02/2015 12:29 AM, Peter Maydell wrote:
>>
>> +    cpu->cpu_ases = g_new0(CPUAddressSpace, 1);
>> +    cpu->cpu_ases[0].cpu = cpu;
>> +    cpu->cpu_ases[0].as = as;
>> +    cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit;
>> +    memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as);
>>   }
>
>
> What's the plan when it's more than one?

We g_realloc() the array to make it larger if the target-specific
code calls us again to add another AS.

> Just thinking about why separate allocation vs embedding an array.  Though
> possibly with the CPUState member being a pointer to an array within the
> TargetCPUClass, or CPUTargetState.  Dunno.

An embedded array runs you into the problem that cpu.h doesn't
have access to a definition of the MemoryListener struct (at
least I think it's that one), so it doesn't know how much space
to allocate in the structure. Plus MemoryListener doesn't
exist in non-softmmu configs, and allowing the CPUState struct
to be different sizes for softmmu vs not doesn't work because
the header can be used from compiled-once-only .c files.
This awkwardness is why we ended up with CPUState having a
pointer to a MemoryListener and thus the loop in tcg_commit
in the first place.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/3] exec.c: Collect AddressSpace related fields into a CPUAddressSpace struct

[Richard Henderson]

On 10/08/2015 08:13 AM, Peter Maydell wrote:
> On 7 October 2015 at 10:57, Richard Henderson <rth@twiddle.net> wrote:
>> On 10/02/2015 12:29 AM, Peter Maydell wrote:
>>>
>>> +    cpu->cpu_ases = g_new0(CPUAddressSpace, 1);
>>> +    cpu->cpu_ases[0].cpu = cpu;
>>> +    cpu->cpu_ases[0].as = as;
>>> +    cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit;
>>> +    memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as);
>>>    }
>>
>>
>> What's the plan when it's more than one?
>
> We g_realloc() the array to make it larger if the target-specific
> code calls us again to add another AS.
>
>> Just thinking about why separate allocation vs embedding an array.  Though
>> possibly with the CPUState member being a pointer to an array within the
>> TargetCPUClass, or CPUTargetState.  Dunno.
>
> An embedded array runs you into the problem that cpu.h doesn't
> have access to a definition of the MemoryListener struct (at
> least I think it's that one), so it doesn't know how much space
> to allocate in the structure. Plus MemoryListener doesn't
> exist in non-softmmu configs, and allowing the CPUState struct
> to be different sizes for softmmu vs not doesn't work because
> the header can be used from compiled-once-only .c files.
> This awkwardness is why we ended up with CPUState having a
> pointer to a MemoryListener and thus the loop in tcg_commit
> in the first place.

Ah, right.  Thanks.  Whole series

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~