[Qemu-devel] [PATCH 0/3] exec: Collect CPU's AddressSpace info into its own struct

[Qemu-devel] [PATCH 0/3] exec: Collect CPU's AddressSpace info into its own struct

[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 and for some early comments.

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 which I hope Paolo will review to tell
me if I've misunderstood things :-)
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.

CPUState::as remains, as a convenience pointer to the first
AddressSpace we know about for the benefit of all the targets
which only have one AS per CPU.

The plan here for where to go next is to support calling
tcg_cpu_address_space_init() more than once (in which case
it will add entries to the array), and then update the places
which currently hardcode cpu->cpu_ases[0] to pick the entry
in the array based on a target-cpu hook to convert MemoryAttributes
into an address space index.

I also need to think a bit harder about how KVM will fit into
this, and exactly where we call tcg_cpu_address_space_init().
(Current thought is that should be from the cpu object's
realize function, based on QOM properties for allowing the board
to register the AddressSpaces with the CPU.)

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

 cpu-exec.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 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>
---
 exec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/exec.c b/exec.c
index f7883d2..2616394 100644
--- a/exec.c
+++ b/exec.c
@@ -534,7 +534,6 @@ void cpu_exec_init(CPUArchState *env)
 #ifndef CONFIG_USER_ONLY
     cpu->as = &address_space_memory;
     cpu->thread_id = qemu_get_thread_id();
-    cpu_reload_memory_map(cpu);
 #endif
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
 #if defined(CONFIG_USER_ONLY)
-- 
1.9.1

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

[Edgar E. Iglesias]

On Fri, Jul 03, 2015 at 04:11:41PM +0100, Peter Maydell wrote:
> 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.
>

Looks good to me;
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>

Cheers,
Edgar


 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  exec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index f7883d2..2616394 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -534,7 +534,6 @@ void cpu_exec_init(CPUArchState *env)
>  #ifndef CONFIG_USER_ONLY
>      cpu->as = &address_space_memory;
>      cpu->thread_id = qemu_get_thread_id();
> -    cpu_reload_memory_map(cpu);
>  #endif
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>  #if defined(CONFIG_USER_ONLY)
> -- 
> 1.9.1
> 

[Qemu-devel] [PATCH 2/3] cpu-exec.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>
---
This patch is as much to get confirmation from Paolo about whether I
understand this as anything else :-) (Also, fixes the duplicate
"as it"...)
---
 cpu-exec.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 2ffeb6e..d84c2cd 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -150,13 +150,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 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.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.c b/cpu-exec.c
index d84c2cd..cdfb0de 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -145,10 +145,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
@@ -167,17 +165,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 cpuas->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 2616394..ecefd18 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)
@@ -416,7 +431,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);
@@ -503,13 +518,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
 
@@ -2097,7 +2115,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;
@@ -2156,19 +2175,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 2573e8c..53445a1 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -93,7 +93,7 @@ void QEMU_NORETURN cpu_loop_exit(CPUState *cpu);
 
 #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 */
 void tlb_flush_page(CPUState *cpu, target_ulong addr);
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 6fdcbcd..df39eba 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 39f0f19..ca18e49 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -228,6 +228,10 @@ struct kvm_run;
  * This allows a single read-compare-cbranch-write sequence to test
  * for both decrementer underflow and exceptions.
  * @can_do_io: Nonzero if memory-mapped IO is safe.
+ * @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.
@@ -269,9 +273,8 @@ struct CPUState {
     int64_t icount_extra;
     sigjmp_buf jmp_env;
 
+    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 0/3] exec: Collect CPU's AddressSpace info into its own struct

[Paolo Bonzini]

On 03/07/2015 17:11, Peter Maydell wrote:
> Patch 2 is a comment change which I hope Paolo will review to tell
> me if I've misunderstood things :-)

Yes, the comment summarizes our discussions from yesterday pretty well.

Paolo

Re: [Qemu-devel] [PATCH 0/3] exec: Collect CPU's AddressSpace info into its own struct

[Peter Maydell]

On 3 July 2015 at 16:11, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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 and for some early comments.
>
> 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 which I hope Paolo will review to tell
> me if I've misunderstood things :-)
> 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.
>
> CPUState::as remains, as a convenience pointer to the first
> AddressSpace we know about for the benefit of all the targets
> which only have one AS per CPU.

Paolo, would you take (a rebased version of) this series as it
stands, on the basis that it fixes that FIXME about the MemoryListener?
Or do you want more of an actual use case for it first?

(I've done the rebase, just wondering if it's worth sending out
again yet.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] exec: Collect CPU's AddressSpace info into its own struct

[Paolo Bonzini]

On 01/10/2015 15:05, Peter Maydell wrote:
>> >
>> > CPUState::as remains, as a convenience pointer to the first
>> > AddressSpace we know about for the benefit of all the targets
>> > which only have one AS per CPU.
> Paolo, would you take (a rebased version of) this series as it
> stands, on the basis that it fixes that FIXME about the MemoryListener?
> Or do you want more of an actual use case for it first?

Yes, sure.

Paolo