| Message ID | 1387181170-23267-9-git-send-email-edgar.iglesias@gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 16.12.2013 09:05, schrieb edgar.iglesias@gmail.com: > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > cputlb.c | 4 ++-- > exec.c | 31 +++++++++++++++++++++++-------- > include/exec/cpu-defs.h | 3 +++ > include/exec/exec-all.h | 1 + > include/exec/softmmu_template.h | 4 ++-- > include/qom/cpu.h | 2 ++ > 6 files changed, 33 insertions(+), 12 deletions(-) [...] > diff --git a/exec.c b/exec.c > index 803bbde..edb6a43 100644 > --- a/exec.c > +++ b/exec.c > @@ -136,6 +136,7 @@ typedef struct subpage_t { > > static void io_mem_init(void); > static void memory_map_init(void); > +static void tcg_commit(MemoryListener *listener); > > static MemoryRegion io_mem_watch; > #endif > @@ -434,6 +435,25 @@ CPUState *qemu_get_cpu(int index) > return NULL; > } > > +#if !defined(CONFIG_USER_ONLY) > +void cpu_address_space_init(CPUState *cpu, AddressSpace *as) > +{ > + CPUArchState *env = cpu->env_ptr; > + > + if (tcg_enabled()) { > + if (cpu->tcg_as_listener) { > + memory_listener_unregister(cpu->tcg_as_listener); > + } else { > + cpu->tcg_as_listener = g_new0(MemoryListener, 1); > + } > + cpu->tcg_as_listener->commit = tcg_commit; > + memory_listener_register(cpu->tcg_as_listener, as); > + } > + > + env->as = as; > +} > +#endif > + > void cpu_exec_init(CPUArchState *env) > { > CPUState *cpu = ENV_GET_CPU(env); > @@ -453,6 +473,7 @@ void cpu_exec_init(CPUArchState *env) > QTAILQ_INIT(&env->breakpoints); > QTAILQ_INIT(&env->watchpoints); > #ifndef CONFIG_USER_ONLY > + cpu_address_space_init(cpu, &address_space_memory); > cpu->thread_id = qemu_get_thread_id(); > #endif > QTAILQ_INSERT_TAIL(&cpus, cpu, node); > @@ -482,9 +503,10 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) > #else > static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) > { > + CPUArchState *env = cpu->env_ptr; > hwaddr phys = cpu_get_phys_page_debug(cpu, pc); > if (phys != -1) { > - tb_invalidate_phys_addr(&address_space_memory, > + tb_invalidate_phys_addr(env->as, > phys | (pc & ~TARGET_PAGE_MASK)); > } > } [...] > diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h > index 01cd8c7..406b36c 100644 > --- a/include/exec/cpu-defs.h > +++ b/include/exec/cpu-defs.h > @@ -176,6 +176,9 @@ typedef struct CPUWatchpoint { > sigjmp_buf jmp_env; \ > int exception_index; \ > \ > + /* Per CPU address-space. */ \ > + AddressSpace *as; \ Why are you adding this field here rather than in CPUState alongside the other field? This being a pointer I can't imagine problems for non-softmmu, and I had posted patches a while ago to move the surrounding fields there. Having it in CPUState would avoid some of the env_ptr accesses above, which were supposed to be an interim solution only. Regards, Andreas > + \ > /* user data */ \ > void *opaque; \ > [...] > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 7739e00..c1febae 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -186,6 +186,8 @@ struct CPUState { > uint32_t interrupt_request; > int singlestep_enabled; > > + MemoryListener *tcg_as_listener; > + > void *env_ptr; /* CPUArchState */ > struct TranslationBlock *current_tb; > struct GDBRegisterState *gdb_regs;
On Mon, Dec 16, 2013 at 01:11:47PM +0100, Andreas Färber wrote: > Am 16.12.2013 09:05, schrieb edgar.iglesias@gmail.com: > > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com> > > > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > > --- > > cputlb.c | 4 ++-- > > exec.c | 31 +++++++++++++++++++++++-------- > > include/exec/cpu-defs.h | 3 +++ > > include/exec/exec-all.h | 1 + > > include/exec/softmmu_template.h | 4 ++-- > > include/qom/cpu.h | 2 ++ > > 6 files changed, 33 insertions(+), 12 deletions(-) > [...] > > diff --git a/exec.c b/exec.c > > index 803bbde..edb6a43 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -136,6 +136,7 @@ typedef struct subpage_t { > > > > static void io_mem_init(void); > > static void memory_map_init(void); > > +static void tcg_commit(MemoryListener *listener); > > > > static MemoryRegion io_mem_watch; > > #endif > > @@ -434,6 +435,25 @@ CPUState *qemu_get_cpu(int index) > > return NULL; > > } > > > > +#if !defined(CONFIG_USER_ONLY) > > +void cpu_address_space_init(CPUState *cpu, AddressSpace *as) > > +{ > > + CPUArchState *env = cpu->env_ptr; > > + > > + if (tcg_enabled()) { > > + if (cpu->tcg_as_listener) { > > + memory_listener_unregister(cpu->tcg_as_listener); > > + } else { > > + cpu->tcg_as_listener = g_new0(MemoryListener, 1); > > + } > > + cpu->tcg_as_listener->commit = tcg_commit; > > + memory_listener_register(cpu->tcg_as_listener, as); > > + } > > + > > + env->as = as; > > +} > > +#endif > > + > > void cpu_exec_init(CPUArchState *env) > > { > > CPUState *cpu = ENV_GET_CPU(env); > > @@ -453,6 +473,7 @@ void cpu_exec_init(CPUArchState *env) > > QTAILQ_INIT(&env->breakpoints); > > QTAILQ_INIT(&env->watchpoints); > > #ifndef CONFIG_USER_ONLY > > + cpu_address_space_init(cpu, &address_space_memory); > > cpu->thread_id = qemu_get_thread_id(); > > #endif > > QTAILQ_INSERT_TAIL(&cpus, cpu, node); > > @@ -482,9 +503,10 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) > > #else > > static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) > > { > > + CPUArchState *env = cpu->env_ptr; > > hwaddr phys = cpu_get_phys_page_debug(cpu, pc); > > if (phys != -1) { > > - tb_invalidate_phys_addr(&address_space_memory, > > + tb_invalidate_phys_addr(env->as, > > phys | (pc & ~TARGET_PAGE_MASK)); > > } > > } > [...] > > diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h > > index 01cd8c7..406b36c 100644 > > --- a/include/exec/cpu-defs.h > > +++ b/include/exec/cpu-defs.h > > @@ -176,6 +176,9 @@ typedef struct CPUWatchpoint { > > sigjmp_buf jmp_env; \ > > int exception_index; \ > > \ > > + /* Per CPU address-space. */ \ > > + AddressSpace *as; \ > > Why are you adding this field here rather than in CPUState alongside the > other field? This being a pointer I can't imagine problems for > non-softmmu, and I had posted patches a while ago to move the > surrounding fields there. Having it in CPUState would avoid some of the > env_ptr accesses above, which were supposed to be an interim solution only. Hi, This was discussed when I posted the RFC. My first try had this member in CPUState but some of the hot paths for mmio accesses needed to do ENV_GET_CPU() to get hold of the CS and AS. ENV_GET_CPU does a runtime type check that would slow things down. That's the reason I moved it to env. I'm not against moving the field back if it doesnt cost too much. Maybe we should consider removing the CPU() around ENV_GET_CPU()? See: http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02889.html Cheers, Edgar
On 17 December 2013 00:34, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Mon, Dec 16, 2013 at 01:11:47PM +0100, Andreas Färber wrote: >> Why are you adding this field here rather than in CPUState alongside the >> other field? This being a pointer I can't imagine problems for >> non-softmmu, and I had posted patches a while ago to move the >> surrounding fields there. Having it in CPUState would avoid some of the >> env_ptr accesses above, which were supposed to be an interim solution only. > This was discussed when I posted the RFC. My first try had this member in > CPUState but some of the hot paths for mmio accesses needed to do > ENV_GET_CPU() to get hold of the CS and AS. ENV_GET_CPU does a runtime type > check that would slow things down. That's the reason I moved it to env. > > I'm not against moving the field back if it doesnt cost too much. Maybe we > should consider removing the CPU() around ENV_GET_CPU()? > > See: > http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02889.html I think that's the wrong way round. We should put the field in the right place in the code, which is CPUState. To the extent that that means we discover that our dynamic cast macros are not fast enough for hot paths we should make the actual error checking be debug builds only. (There's been pushback on dynamic-casts in fastpaths before and the preferred approach so far has been "optimise the cast". I think we should take "...and do it only in debug builds" over "do the wrong thing because a purely-debug-purposes type check isn't as fast as we'd like".) thanks -- PMM
On Tue, Dec 17, 2013 at 10:54 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 17 December 2013 00:34, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: >> On Mon, Dec 16, 2013 at 01:11:47PM +0100, Andreas Färber wrote: >>> Why are you adding this field here rather than in CPUState alongside the >>> other field? This being a pointer I can't imagine problems for >>> non-softmmu, and I had posted patches a while ago to move the >>> surrounding fields there. Having it in CPUState would avoid some of the >>> env_ptr accesses above, which were supposed to be an interim solution only. > >> This was discussed when I posted the RFC. My first try had this member in >> CPUState but some of the hot paths for mmio accesses needed to do >> ENV_GET_CPU() to get hold of the CS and AS. ENV_GET_CPU does a runtime type >> check that would slow things down. That's the reason I moved it to env. >> >> I'm not against moving the field back if it doesnt cost too much. Maybe we >> should consider removing the CPU() around ENV_GET_CPU()? >> >> See: >> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02889.html > > I think that's the wrong way round. We should put the field in the right > place in the code, which is CPUState. To the extent that that means we > discover that our dynamic cast macros are not fast enough for hot > paths we should make the actual error checking be debug builds > only. (There's been pushback on dynamic-casts in fastpaths before > and the preferred approach so far has been "optimise the cast". > I think we should take "...and do it only in debug builds" over "do > the wrong thing because a purely-debug-purposes type check > isn't as fast as we'd like".) > A levelled approach to debugging can help here too rather than having all-or-none with the QoM casts. Regards, Peter > thanks > -- PMM >
On Tue, Dec 17, 2013 at 12:54:19AM +0000, Peter Maydell wrote: > On 17 December 2013 00:34, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > > On Mon, Dec 16, 2013 at 01:11:47PM +0100, Andreas Färber wrote: > >> Why are you adding this field here rather than in CPUState alongside the > >> other field? This being a pointer I can't imagine problems for > >> non-softmmu, and I had posted patches a while ago to move the > >> surrounding fields there. Having it in CPUState would avoid some of the > >> env_ptr accesses above, which were supposed to be an interim solution only. > > > This was discussed when I posted the RFC. My first try had this member in > > CPUState but some of the hot paths for mmio accesses needed to do > > ENV_GET_CPU() to get hold of the CS and AS. ENV_GET_CPU does a runtime type > > check that would slow things down. That's the reason I moved it to env. > > > > I'm not against moving the field back if it doesnt cost too much. Maybe we > > should consider removing the CPU() around ENV_GET_CPU()? > > > > See: > > http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02889.html > > I think that's the wrong way round. We should put the field in the right > place in the code, which is CPUState. To the extent that that means we > discover that our dynamic cast macros are not fast enough for hot > paths we should make the actual error checking be debug builds > only. (There's been pushback on dynamic-casts in fastpaths before > and the preferred approach so far has been "optimise the cast". > I think we should take "...and do it only in debug builds" over "do > the wrong thing because a purely-debug-purposes type check > isn't as fast as we'd like".) Sounds reasonable to me, I'll move the AS back into CPUState for v2. Thanks, Edgar
diff --git a/cputlb.c b/cputlb.c index 0399172..a2264a3 100644 --- a/cputlb.c +++ b/cputlb.c @@ -254,7 +254,7 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr, } sz = size; - section = address_space_translate_for_iotlb(&address_space_memory, paddr, + section = address_space_translate_for_iotlb(env->as, paddr, &xlat, &sz); assert(sz >= TARGET_PAGE_SIZE); @@ -327,7 +327,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr) cpu_ldub_code(env1, addr); } pd = env1->iotlb[mmu_idx][page_index] & ~TARGET_PAGE_MASK; - mr = iotlb_to_region(&address_space_memory, pd); + mr = iotlb_to_region(env1->as, pd); if (memory_region_is_unassigned(mr)) { CPUState *cpu = ENV_GET_CPU(env1); CPUClass *cc = CPU_GET_CLASS(cpu); diff --git a/exec.c b/exec.c index 803bbde..edb6a43 100644 --- a/exec.c +++ b/exec.c @@ -136,6 +136,7 @@ typedef struct subpage_t { static void io_mem_init(void); static void memory_map_init(void); +static void tcg_commit(MemoryListener *listener); static MemoryRegion io_mem_watch; #endif @@ -434,6 +435,25 @@ CPUState *qemu_get_cpu(int index) return NULL; } +#if !defined(CONFIG_USER_ONLY) +void cpu_address_space_init(CPUState *cpu, AddressSpace *as) +{ + CPUArchState *env = cpu->env_ptr; + + if (tcg_enabled()) { + if (cpu->tcg_as_listener) { + memory_listener_unregister(cpu->tcg_as_listener); + } else { + cpu->tcg_as_listener = g_new0(MemoryListener, 1); + } + cpu->tcg_as_listener->commit = tcg_commit; + memory_listener_register(cpu->tcg_as_listener, as); + } + + env->as = as; +} +#endif + void cpu_exec_init(CPUArchState *env) { CPUState *cpu = ENV_GET_CPU(env); @@ -453,6 +473,7 @@ void cpu_exec_init(CPUArchState *env) QTAILQ_INIT(&env->breakpoints); QTAILQ_INIT(&env->watchpoints); #ifndef CONFIG_USER_ONLY + cpu_address_space_init(cpu, &address_space_memory); cpu->thread_id = qemu_get_thread_id(); #endif QTAILQ_INSERT_TAIL(&cpus, cpu, node); @@ -482,9 +503,10 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) #else static void breakpoint_invalidate(CPUState *cpu, target_ulong pc) { + CPUArchState *env = cpu->env_ptr; hwaddr phys = cpu_get_phys_page_debug(cpu, pc); if (phys != -1) { - tb_invalidate_phys_addr(&address_space_memory, + tb_invalidate_phys_addr(env->as, phys | (pc & ~TARGET_PAGE_MASK)); } } @@ -1810,10 +1832,6 @@ static MemoryListener core_memory_listener = { .priority = 1, }; -static MemoryListener tcg_memory_listener = { - .commit = tcg_commit, -}; - void address_space_init_dispatch(AddressSpace *as) { as->dispatch = NULL; @@ -1849,9 +1867,6 @@ static void memory_map_init(void) address_space_init(&address_space_io, system_io, "I/O"); memory_listener_register(&core_memory_listener, &address_space_memory); - if (tcg_enabled()) { - memory_listener_register(&tcg_memory_listener, &address_space_memory); - } } MemoryRegion *get_system_memory(void) diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h index 01cd8c7..406b36c 100644 --- a/include/exec/cpu-defs.h +++ b/include/exec/cpu-defs.h @@ -176,6 +176,9 @@ typedef struct CPUWatchpoint { sigjmp_buf jmp_env; \ int exception_index; \ \ + /* Per CPU address-space. */ \ + AddressSpace *as; \ + \ /* user data */ \ void *opaque; \ diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 6129365..61770ee 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -95,6 +95,7 @@ void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end, void tb_invalidate_phys_range(tb_page_addr_t start, tb_page_addr_t end, int is_cpu_write_access); #if !defined(CONFIG_USER_ONLY) +void cpu_address_space_init(CPUState *cpu, AddressSpace *as); /* cputlb.c */ void tlb_flush_page(CPUArchState *env, target_ulong addr); void tlb_flush(CPUArchState *env, int flush_global); diff --git a/include/exec/softmmu_template.h b/include/exec/softmmu_template.h index 69d856a..1dacb4d 100644 --- a/include/exec/softmmu_template.h +++ b/include/exec/softmmu_template.h @@ -119,7 +119,7 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env, uintptr_t retaddr) { uint64_t val; - MemoryRegion *mr = iotlb_to_region(&address_space_memory, physaddr); + MemoryRegion *mr = iotlb_to_region(env->as, physaddr); physaddr = (physaddr & TARGET_PAGE_MASK) + addr; env->mem_io_pc = retaddr; @@ -325,7 +325,7 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env, target_ulong addr, uintptr_t retaddr) { - MemoryRegion *mr = iotlb_to_region(&address_space_memory, physaddr); + MemoryRegion *mr = iotlb_to_region(env->as, physaddr); physaddr = (physaddr & TARGET_PAGE_MASK) + addr; if (mr != &io_mem_rom && mr != &io_mem_notdirty && !can_do_io(env)) { diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 7739e00..c1febae 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -186,6 +186,8 @@ struct CPUState { uint32_t interrupt_request; int singlestep_enabled; + MemoryListener *tcg_as_listener; + void *env_ptr; /* CPUArchState */ struct TranslationBlock *current_tb; struct GDBRegisterState *gdb_regs;