Re: [Qemu-devel] [PATCH v1 08/22] cpu: Add per-cpu address space

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

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