| Message ID | 1443709790-25180-4-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
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 >
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~
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
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~
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;
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(-)