Re: [Qemu-devel] [PATCH 02/16] exec.c: Allow target CPUs to define multiple AddressSpaces

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

On Thu, Nov 05, 2015 at 06:15:44PM +0000, Peter Maydell wrote:
> Allow multiple calls to cpu_address_space_init(); each
> call adds an entry to the cpu->ases array at the specified
> index. It is up to the target-specific CPU code to actually use
> these extra address spaces.
> 
> Since this multiple AddressSpace support won't work with
> KVM, add an assertion to avoid confusing failures.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  exec.c            | 28 ++++++++++++++++++----------
>  include/qom/cpu.h |  2 ++
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index b5490c8..6a2a694 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -552,25 +552,32 @@ CPUState *qemu_get_cpu(int index)
>  #if !defined(CONFIG_USER_ONLY)
>  void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx)
>  {
> +    CPUAddressSpace *newas;
> +
>      if (asidx == 0) {
>          /* address space 0 gets the convenience alias */
>          cpu->as = as;
>      }
>  
> -    /* We only support one address space per cpu at the moment.  */
> -    assert(cpu->as == as);
> +    /* KVM cannot currently support multiple address spaces. */
> +    assert(asidx == 0 || !kvm_enabled());
>  
> -    if (cpu->cpu_ases) {
> -        /* We've already registered the listener for our only AS */
> -        return;
> +    if (asidx >= cpu->num_ases) {
> +        if (cpu->num_ases == 0) {
> +            cpu->cpu_ases = g_new(CPUAddressSpace, asidx + 1);
> +        } else {
> +            cpu->cpu_ases = g_renew(CPUAddressSpace, cpu->cpu_ases, asidx + 1);

IIUC, g_renew may move the entire cpu_ases area. The internals of
memory_listener_register (called below) seem to put away the pointers to listeners
so a renew+move would leave invalid pointers to listeners in memory.c wouldn't it?

There are various ways of solving this, (e.g dynamic allocation of the listener,
static allocation of the cpu_ases, invalidate all listeners and restore them after
each as init and more). I'm sure you'll figure something out.



> +        }
> +        cpu->num_ases = asidx + 1;
>      }
>  
> -    cpu->cpu_ases = g_new0(CPUAddressSpace, 1);
> -    cpu->cpu_ases[0].cpu = cpu;
> -    cpu->cpu_ases[0].as = as;
> +    newas = &cpu->cpu_ases[asidx];
> +    memset(newas, 0, sizeof(*newas));
> +    newas->cpu = cpu;
> +    newas->as = as;
>      if (tcg_enabled()) {
> -        cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit;
> -        memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as);
> +        newas->tcg_as_listener.commit = tcg_commit;
> +        memory_listener_register(&newas->tcg_as_listener, as);
>      }
>  }
>  #endif
> @@ -627,6 +634,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      Error *local_err = NULL;
>  
>      cpu->as = NULL;
> +    cpu->num_ases = 0;
>  
>  #ifndef CONFIG_USER_ONLY
>      cpu->thread_id = qemu_get_thread_id();
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 51a1323..ae17932 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -236,6 +236,7 @@ struct kvm_run;
>   * so that interrupts take effect immediately.
>   * @cpu_ases: Pointer to array of CPUAddressSpaces (which define the
>   *            AddressSpaces this CPU has)
> + * @num_ases: number of CPUAddressSpaces in @cpu_ases
>   * @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.
> @@ -285,6 +286,7 @@ struct CPUState {
>      struct qemu_work_item *queued_work_first, *queued_work_last;
>  
>      CPUAddressSpace *cpu_ases;
> +    int num_ases;
>      AddressSpace *as;
>  
>      void *env_ptr; /* CPUArchState */
> -- 
> 1.9.1
>