[Qemu-devel] [PATCH 00/16] Add support for multiple address spaces per CPU and use it for ARM TrustZone

[Qemu-devel] [PATCH 00/16] Add support for multiple address spaces per CPU and use it for ARM TrustZone

[Peter Maydell]

This patch series adds support to QEMU's core exec/memory code for
CPUs which have more than one address space, and uses it for
ARM TrustZone. In particular, a TZ CPU will have two physical
address spaces (Secure and Non-secure), and the patchset allows a
board model to create these both separately and connect them to
the CPU, so that we can have devices or memory which are visible
only in the Secure address space. (We already have support for
TZ in the CPU emulation itself, and support for devices like the
GIC which respond differently to Secure and Non-secure accesses,
so this is the last part of the puzzle for 32-bit.)

The general approach is that we allow a target-* cpu to define
more than one address space at initialization, allocating each
one a small integer "address space index" (asidx). The asidx
can then be specified when filling a TLB entry via tlb_set_page_with_attrs
and is used to look up the memory region in the appropriate AS.
The asidx is also stored in the iotlb, since for MMIO we need it at
the io_read/write stage when we call iotlb_to_region().
The first 8 or so patches implement the handling of this in core code.

Patches 9 and 10 are the target-arm specific code for multiple
AS support (passing back asidxes in the right places, and using
the correct AS when doing direct physical loads and stores from
target-specific code).

Patch 11 is Peter Crosthwaite's "share AddressSpaces if they're really
rooted on the same memory region" patch, though I have made some
changes to it (notably, not checking the debug-name when checking if
we can share an AS, and fixing a use-after-free bug). I think it
makes sense but I'd appreciate review from Paolo here in particular.

Patches 12 and 13 add new QOM properties to the base CPU object
and to ARM CPU objects for 'memory' and 'secure-memory' so the
board code can pass the AddressSpaces in. I'd quite like to see
us end up with a consistent naming scheme for all bus masters
(eg "should all be foo-memory" or something), rather than just
naming the properties based on random whim or whatever the hardware
calls the bus. Better naming proposals welcome.

Patches 14, 15 and 16 are changes to the 'virt' board. 14 is trivial.
15 adds a secure address space and a secure-only UART if the board
is started with '-machine secure=on'. It's marked RFC mostly because
the device tree binding to say "device is secure-only" is still
under discussion on the kernel mailing list. Patch 16 is a pure
hack, and is here for testing purposes only.

You can run OP-TEE on QEMU with these patches:
see https://github.com/OP-TEE/optee_os for details of how to
get, build and run it. The 'make run' will use the custom QEMU
version that comes with OP-TEE (do that first to make sure your
OP-TEE has built and works ok). To get it to use a locally built QEMU
with these patches do:

make run-only QEMU_PATH=/path/to/your/qemu/build/directory QEMU_EXTRA_ARGS='-machine secure=on'

Notes on a couple of things the patchset doesn't address:
 (1) image/romfile/kernel loading etc will load only into the nonsecure
address space. This would be conceptually simple to implement (you just
need to pass an AS into lots of functions) but since OP-TEE doesn't need
it I felt it could safely be left for later rather than making this
patchset bigger.

 (2) Using multiple address spaces in one CPU won't work with KVM
(and we assert if you try; nothing at the moment will attempt it).
Using different address spaces in different CPUs in an SMP setup
will also not work with KVM, but we don't assert on that because
I wasn't sure where best to put the assert. (Also, it would be
nice if we could do that, because the modelling for ARM SMP
setups would really be cleaner if we could put the per-CPU
devices and so on in a set of per-CPU ASes.)

You can find a git branch with this patchset in here:
 https://git.linaro.org/people/peter.maydell/qemu-arm.git multi-ases

thanks
-- PMM


Peter Crosthwaite (2):
  memory: Add address_space_init_shareable()
  qom/cpu: Add MemoryRegion property

Peter Maydell (14):
  exec.c: Don't set cpu->as until cpu_address_space_init
  exec.c: Allow target CPUs to define multiple AddressSpaces
  tlb_set_page_with_attrs: Take argument specifying AddressSpace to use
  exec.c: Add address space index to CPUIOTLBEntry
  exec.c: Add cpu_get_address_space()
  include/qom/cpu.h: Add new get_phys_page_asidx_debug method
  exec.c: Use cpu_get_phys_page_asidx_debug
  exec.c: Have one io_mem_watch per AddressSpace
  target-arm: Support multiple address spaces in page table walks
  target-arm: Implement cpu_get_phys_page_asidx_debug
  target-arm: Add QOM property for Secure memory region
  hw/arm/virt: Wire up memory region to CPUs explicitly
  [RFC] hw/arm/virt: add secure memory region and UART
  HACK: rearrange the virt memory map to suit OP-TEE

 cpus.c                  |  12 ++++-
 cputlb.c                |  13 +++--
 exec.c                  | 125 +++++++++++++++++++++++++++++++++---------------
 hw/arm/virt.c           |  66 ++++++++++++++++++++-----
 include/exec/cpu-defs.h |   1 +
 include/exec/exec-all.h |  60 +++++++++++++++++++++--
 include/exec/memory.h   |  20 ++++++++
 include/hw/arm/virt.h   |   1 +
 include/qom/cpu.h       |  37 +++++++++++++-
 memory.c                |  27 +++++++++++
 softmmu_template.h      |   4 +-
 target-arm/cpu-qom.h    |   5 +-
 target-arm/cpu.c        |  28 ++++++++++-
 target-arm/cpu.h        |  29 +++++++++++
 target-arm/helper.c     |  13 +++--
 target-i386/cpu.c       |   6 ++-
 target-i386/helper.c    |   2 +-
 17 files changed, 374 insertions(+), 75 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 01/16] exec.c: Don't set cpu->as until cpu_address_space_init

[Peter Maydell]

Rather than setting cpu->as unconditionally in cpu_exec_init
(and then having target-i386 override this later), don't set
it until the first call to cpu_address_space_init.

This requires us to initialise the address space for
both TCG and KVM (KVM doesn't need the AS listener but
it does require cpu->as to be set).

For target CPUs which don't set up any address spaces (currently
everything except i386), add the default address_space_memory
in qemu_init_vcpu().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpus.c                  | 10 ++++++++--
 exec.c                  | 16 ++++++++++++----
 include/exec/exec-all.h | 16 +++++++++++++++-
 target-i386/cpu.c       |  6 ++++--
 4 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/cpus.c b/cpus.c
index c6a5d0e..764ea75 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1291,8 +1291,6 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
     static QemuCond *tcg_halt_cond;
     static QemuThread *tcg_cpu_thread;
 
-    tcg_cpu_address_space_init(cpu, cpu->as);
-
     /* share a single thread for all cpus with TCG */
     if (!tcg_cpu_thread) {
         cpu->thread = g_malloc0(sizeof(QemuThread));
@@ -1353,6 +1351,14 @@ void qemu_init_vcpu(CPUState *cpu)
     cpu->nr_cores = smp_cores;
     cpu->nr_threads = smp_threads;
     cpu->stopped = true;
+
+    if (!cpu->as) {
+        /* If the target cpu hasn't set up any address spaces itself,
+         * give it the default one.
+         */
+        cpu_address_space_init(cpu, &address_space_memory, 0);
+    }
+
     if (kvm_enabled()) {
         qemu_kvm_start_vcpu(cpu);
     } else if (tcg_enabled()) {
diff --git a/exec.c b/exec.c
index 1e8b51b..b5490c8 100644
--- a/exec.c
+++ b/exec.c
@@ -550,8 +550,13 @@ CPUState *qemu_get_cpu(int index)
 }
 
 #if !defined(CONFIG_USER_ONLY)
-void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
+void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx)
 {
+    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);
 
@@ -563,8 +568,10 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *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);
+    if (tcg_enabled()) {
+        cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit;
+        memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as);
+    }
 }
 #endif
 
@@ -619,8 +626,9 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
     int cpu_index;
     Error *local_err = NULL;
 
+    cpu->as = NULL;
+
 #ifndef CONFIG_USER_ONLY
-    cpu->as = &address_space_memory;
     cpu->thread_id = qemu_get_thread_id();
 #endif
 
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 9b93b9b..90130ca 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -85,7 +85,21 @@ void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
 
 #if !defined(CONFIG_USER_ONLY)
 void cpu_reloading_memory_map(void);
-void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as);
+/**
+ * cpu_address_space_init:
+ * @cpu: CPU to add this address space to
+ * @as: address space to add
+ * @asidx: integer index of this address space
+ *
+ * Add the specified address space to the CPU's cpu_ases list.
+ * The address space added with @asidx 0 is the one used for the
+ * convenience pointer cpu->as.
+ * The target-specific code which registers ASes is responsible
+ * for defining what semantics address space 0, 1, 2, etc have.
+ *
+ * Note that with KVM only one address space is supported.
+ */
+void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx);
 /* cputlb.c */
 /**
  * tlb_flush_page:
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 9d0eedf..8096860 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2856,9 +2856,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
 
 #ifndef CONFIG_USER_ONLY
     if (tcg_enabled()) {
+        AddressSpace *newas = g_new(AddressSpace, 1);
+
         cpu->cpu_as_mem = g_new(MemoryRegion, 1);
         cpu->cpu_as_root = g_new(MemoryRegion, 1);
-        cs->as = g_new(AddressSpace, 1);
 
         /* Outer container... */
         memory_region_init(cpu->cpu_as_root, OBJECT(cpu), "memory", ~0ull);
@@ -2871,7 +2872,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
                                  get_system_memory(), 0, ~0ull);
         memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);
         memory_region_set_enabled(cpu->cpu_as_mem, true);
-        address_space_init(cs->as, cpu->cpu_as_root, "CPU");
+        address_space_init(newas, cpu->cpu_as_root, "CPU");
+        cpu_address_space_init(cs, newas, 0);
 
         /* ... SMRAM with higher priority, linked from /machine/smram.  */
         cpu->machine_done.notify = x86_cpu_machine_done;
-- 
1.9.1

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

[Peter Maydell]

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);
+        }
+        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

[Qemu-devel] [PATCH 03/16] tlb_set_page_with_attrs: Take argument specifying AddressSpace to use

[Peter Maydell]

Add an argument to tlb_set_page_with_attrs which allows the target CPU code
to tell the core code which AddressSpace to use.

The AddressSpace is specified by the index into the array of ASes which
were registered with cpu_address_space_init().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cputlb.c                |  6 +++---
 exec.c                  |  7 ++++---
 include/exec/exec-all.h | 33 ++++++++++++++++++++++++++++++---
 target-arm/helper.c     |  2 +-
 target-i386/helper.c    |  2 +-
 5 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index bf1d50a..e753083 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -343,7 +343,7 @@ static void tlb_add_large_page(CPUArchState *env, target_ulong vaddr,
  * Called from TCG-generated code, which is under an RCU read-side
  * critical section.
  */
-void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
+void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, int asidx,
                              hwaddr paddr, MemTxAttrs attrs, int prot,
                              int mmu_idx, target_ulong size)
 {
@@ -363,7 +363,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
     }
 
     sz = size;
-    section = address_space_translate_for_iotlb(cpu, paddr, &xlat, &sz);
+    section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz);
     assert(sz >= TARGET_PAGE_SIZE);
 
 #if defined(DEBUG_TLB)
@@ -433,7 +433,7 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
                   hwaddr paddr, int prot,
                   int mmu_idx, target_ulong size)
 {
-    tlb_set_page_with_attrs(cpu, vaddr, paddr, MEMTXATTRS_UNSPECIFIED,
+    tlb_set_page_with_attrs(cpu, vaddr, 0, paddr, MEMTXATTRS_UNSPECIFIED,
                             prot, mmu_idx, size);
 }
 
diff --git a/exec.c b/exec.c
index 6a2a694..92e76fa 100644
--- a/exec.c
+++ b/exec.c
@@ -445,12 +445,13 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
 
 /* Called from RCU critical section */
 MemoryRegionSection *
-address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr,
+address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
                                   hwaddr *xlat, hwaddr *plen)
 {
     MemoryRegionSection *section;
-    section = address_space_translate_internal(cpu->cpu_ases[0].memory_dispatch,
-                                               addr, xlat, plen, false);
+    AddressSpaceDispatch *d = cpu->cpu_ases[asidx].memory_dispatch;
+
+    section = address_space_translate_internal(d, addr, xlat, plen, false);
 
     assert(!section->mr->iommu_ops);
     return section;
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 90130ca..472d0fc 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -144,7 +144,34 @@ void tlb_flush_by_mmuidx(CPUState *cpu, ...);
 void tlb_set_page(CPUState *cpu, target_ulong vaddr,
                   hwaddr paddr, int prot,
                   int mmu_idx, target_ulong size);
-void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
+/**
+ * tlb_set_page_with_attrs:
+ * @cpu: CPU to add this TLB entry for
+ * @vaddr: virtual address of page to add entry for
+ * @asidx: index of the AddressSpace the physical address is in
+ * @paddr: physical address of the page
+ * @attrs: memory transaction attributes
+ * @prot: access permissions (PAGE_READ/PAGE_WRITE/PAGE_EXEC bits)
+ * @mmu_idx: MMU index to insert TLB entry for
+ * @size: size of the page in bytes
+ *
+ * Add an entry to this CPU's TLB (a mapping from virtual address
+ * @vaddr to physical address @paddr) with the specified memory
+ * transaction attributes. This is generally called by the target CPU
+ * specific code after it has been called through the tlb_fill()
+ * entry point and performed a successful page table walk to find
+ * the physical address and attributes for the virtual address
+ * which provoked the TLB miss.
+ *
+ * At most one entry for a given virtual address is permitted. Only a
+ * single TARGET_PAGE_SIZE region is mapped; the supplied @size is only
+ * used by tlb_flush_page.
+ *
+ * @asidx is the index of the AddressSpace in the cpu->ases array;
+ * if the CPU does not support multiple AddressSpaces then it will
+ * always be zero.
+ */
+void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, int asidx,
                              hwaddr paddr, MemTxAttrs attrs,
                              int prot, int mmu_idx, target_ulong size);
 void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr);
@@ -400,8 +427,8 @@ void tlb_set_dirty(CPUState *cpu, target_ulong vaddr);
 void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
 
 MemoryRegionSection *
-address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr, hwaddr *xlat,
-                                  hwaddr *plen);
+address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
+                                  hwaddr *xlat, hwaddr *plen);
 hwaddr memory_region_section_get_iotlb(CPUState *cpu,
                                        MemoryRegionSection *section,
                                        target_ulong vaddr,
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 4ecae61..174371b 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7315,7 +7315,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
         /* Map a single [sub]page.  */
         phys_addr &= TARGET_PAGE_MASK;
         address &= TARGET_PAGE_MASK;
-        tlb_set_page_with_attrs(cs, address, phys_addr, attrs,
+        tlb_set_page_with_attrs(cs, address, 0, phys_addr, attrs,
                                 prot, mmu_idx, page_size);
         return 0;
     }
diff --git a/target-i386/helper.c b/target-i386/helper.c
index d18be95..1c43717 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -962,7 +962,7 @@ do_check_protect_pse36:
     page_offset = vaddr & (page_size - 1);
     paddr = pte + page_offset;
 
-    tlb_set_page_with_attrs(cs, vaddr, paddr, cpu_get_mem_attrs(env),
+    tlb_set_page_with_attrs(cs, vaddr, 0, paddr, cpu_get_mem_attrs(env),
                             prot, mmu_idx, page_size);
     return 0;
  do_fault_rsvd:
-- 
1.9.1

[Qemu-devel] [PATCH 04/16] exec.c: Add address space index to CPUIOTLBEntry

[Peter Maydell]

Add the address space index to CPUIOTLBEntry, and use this to pass it
to iotlb_to_region(), so that we use the correct AddressSpace when
doing IO path lookups.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cputlb.c                | 7 +++++--
 exec.c                  | 4 ++--
 include/exec/cpu-defs.h | 1 +
 include/exec/exec-all.h | 2 +-
 softmmu_template.h      | 4 ++--
 5 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index e753083..ae55035 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -397,6 +397,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, int asidx,
     /* refill the tlb */
     env->iotlb[mmu_idx][index].addr = iotlb - vaddr;
     env->iotlb[mmu_idx][index].attrs = attrs;
+    env->iotlb[mmu_idx][index].asidx = asidx;
     te->addend = addend - vaddr;
     if (prot & PAGE_READ) {
         te->addr_read = address;
@@ -448,6 +449,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
     void *p;
     MemoryRegion *mr;
     CPUState *cpu = ENV_GET_CPU(env1);
+    CPUIOTLBEntry *iotlbentry;
 
     page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = cpu_mmu_index(env1, true);
@@ -455,8 +457,9 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
                  (addr & TARGET_PAGE_MASK))) {
         cpu_ldub_code(env1, addr);
     }
-    pd = env1->iotlb[mmu_idx][page_index].addr & ~TARGET_PAGE_MASK;
-    mr = iotlb_to_region(cpu, pd);
+    iotlbentry = &env1->iotlb[mmu_idx][page_index];
+    pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
+    mr = iotlb_to_region(cpu, pd, iotlbentry->asidx);
     if (memory_region_is_unassigned(mr)) {
         CPUClass *cc = CPU_GET_CLASS(cpu);
 
diff --git a/exec.c b/exec.c
index 92e76fa..5e78d82 100644
--- a/exec.c
+++ b/exec.c
@@ -2228,9 +2228,9 @@ static uint16_t dummy_section(PhysPageMap *map, AddressSpace *as,
     return phys_section_add(map, &section);
 }
 
-MemoryRegion *iotlb_to_region(CPUState *cpu, hwaddr index)
+MemoryRegion *iotlb_to_region(CPUState *cpu, hwaddr index, int asidx)
 {
-    CPUAddressSpace *cpuas = &cpu->cpu_ases[0];
+    CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
     AddressSpaceDispatch *d = atomic_rcu_read(&cpuas->memory_dispatch);
     MemoryRegionSection *sections = d->map.sections;
 
diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index 5093be2..d102d79 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -129,6 +129,7 @@ QEMU_BUILD_BUG_ON(sizeof(CPUTLBEntry) != (1 << CPU_TLB_ENTRY_BITS));
 typedef struct CPUIOTLBEntry {
     hwaddr addr;
     MemTxAttrs attrs;
+    int asidx;
 } CPUIOTLBEntry;
 
 #define CPU_COMMON_TLB \
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 472d0fc..5dba8aa 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -398,7 +398,7 @@ extern uintptr_t tci_tb_ptr;
 #if !defined(CONFIG_USER_ONLY)
 
 struct MemoryRegion *iotlb_to_region(CPUState *cpu,
-                                     hwaddr index);
+                                     hwaddr index, int asidx);
 
 void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int mmu_idx,
               uintptr_t retaddr);
diff --git a/softmmu_template.h b/softmmu_template.h
index 6803890..31a0f62 100644
--- a/softmmu_template.h
+++ b/softmmu_template.h
@@ -150,7 +150,7 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
     uint64_t val;
     CPUState *cpu = ENV_GET_CPU(env);
     hwaddr physaddr = iotlbentry->addr;
-    MemoryRegion *mr = iotlb_to_region(cpu, physaddr);
+    MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->asidx);
 
     physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
     cpu->mem_io_pc = retaddr;
@@ -357,7 +357,7 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env,
 {
     CPUState *cpu = ENV_GET_CPU(env);
     hwaddr physaddr = iotlbentry->addr;
-    MemoryRegion *mr = iotlb_to_region(cpu, physaddr);
+    MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->asidx);
 
     physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
     if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
-- 
1.9.1

[Qemu-devel] [PATCH 05/16] exec.c: Add cpu_get_address_space()

[Peter Maydell]

Add a function to return the AddressSpace for a CPU based on
its numerical index. (Callers outside exec.c don't have access
to the CPUAddressSpace struct so can't just fish it out of the
CPUState struct directly.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c                  | 6 ++++++
 include/exec/exec-all.h | 9 +++++++++
 2 files changed, 15 insertions(+)

diff --git a/exec.c b/exec.c
index 5e78d82..13da780 100644
--- a/exec.c
+++ b/exec.c
@@ -581,6 +581,12 @@ void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx)
         memory_listener_register(&newas->tcg_as_listener, as);
     }
 }
+
+AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
+{
+    /* Return the AddressSpace corresponding to the specified index */
+    return cpu->cpu_ases[asidx].as;
+}
 #endif
 
 #ifndef CONFIG_USER_ONLY
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 5dba8aa..fc05872 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -100,6 +100,15 @@ void cpu_reloading_memory_map(void);
  * Note that with KVM only one address space is supported.
  */
 void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx);
+/**
+ * cpu_get_address_space:
+ * @cpu: CPU to get address space from
+ * @asidx: index identifying which address space to get
+ *
+ * Return the requested address space of this CPU. @asidx
+ * specifies which address space to read.
+ */
+AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx);
 /* cputlb.c */
 /**
  * tlb_flush_page:
-- 
1.9.1

[Qemu-devel] [PATCH 06/16] include/qom/cpu.h: Add new get_phys_page_asidx_debug method

[Peter Maydell]

Add a new optional method get_phys_page_asidx_debug to CPUClass.
This is like the existing get_phys_page_debug, but also returns
the address space index to use for the access. This is necessary
for CPUs which have multiple address spaces.

We provide a wrapper function cpu_get_phys_page_asidx_debug()
which falls back to the existing get_phys_page_debug(), so we
don't need to change every target CPU.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/qom/cpu.h | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index ae17932..10ef5cc 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -98,6 +98,9 @@ struct TranslationBlock;
  * #TranslationBlock.
  * @handle_mmu_fault: Callback for handling an MMU fault.
  * @get_phys_page_debug: Callback for obtaining a physical address.
+ * @get_phys_page_asidx_debug: Callback for obtaining a physical address and the
+ *       associated address index. CPUs which have more than one AddressSpace
+ *       should implement this instead of get_phys_page_debug.
  * @gdb_read_register: Callback for letting GDB read a register.
  * @gdb_write_register: Callback for letting GDB write a register.
  * @debug_excp_handler: Callback for handling debug exceptions.
@@ -152,6 +155,7 @@ typedef struct CPUClass {
     int (*handle_mmu_fault)(CPUState *cpu, vaddr address, int rw,
                             int mmu_index);
     hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
+    hwaddr (*get_phys_page_asidx_debug)(CPUState *cpu, vaddr addr, int *asidx);
     int (*gdb_read_register)(CPUState *cpu, uint8_t *buf, int reg);
     int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
     void (*debug_excp_handler)(CPUState *cpu);
@@ -445,6 +449,32 @@ void cpu_dump_statistics(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
 
 #ifndef CONFIG_USER_ONLY
 /**
+ * cpu_get_phys_page_asidx_debug:
+ * @cpu: The CPU to obtain the physical page address for.
+ * @addr: The virtual address.
+ * @asidx: Updated on return with the address space index for this access.
+ *
+ * Obtains the physical page corresponding to a virtual one, together
+ * with the corresponding address space index indicating which AddressSpace
+ * to look the physical address up in.
+ * Use it only for debugging because no protection checks are done.
+ *
+ * Returns: Corresponding physical page address or -1 if no page found.
+ */
+static inline hwaddr cpu_get_phys_page_asidx_debug(CPUState *cpu, vaddr addr,
+                                                   int *asidx)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->get_phys_page_asidx_debug) {
+        return cc->get_phys_page_asidx_debug(cpu, addr, asidx);
+    }
+    /* Fallback for CPUs which don't have multiple address spaces */
+    *asidx = 0;
+    return cc->get_phys_page_debug(cpu, addr);
+}
+
+/**
  * cpu_get_phys_page_debug:
  * @cpu: The CPU to obtain the physical page address for.
  * @addr: The virtual address.
@@ -456,9 +486,9 @@ void cpu_dump_statistics(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
  */
 static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
+    int asidx;
 
-    return cc->get_phys_page_debug(cpu, addr);
+    return cpu_get_phys_page_asidx_debug(cpu, addr, &asidx);
 }
 #endif
 
-- 
1.9.1

[Qemu-devel] [PATCH 07/16] exec.c: Use cpu_get_phys_page_asidx_debug

[Peter Maydell]

Use cpu_get_phys_page_asidx_debug() when doing virtual-to-physical
conversions in debug related code, so that we can obtain the right
address space index and thus select the correct AddressSpace,
rather than always using cpu->as.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/exec.c b/exec.c
index 13da780..bc6ab8a 100644
--- a/exec.c
+++ b/exec.c
@@ -684,9 +684,10 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
 #else
 static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
 {
-    hwaddr phys = cpu_get_phys_page_debug(cpu, pc);
+    int asidx;
+    hwaddr phys = cpu_get_phys_page_asidx_debug(cpu, pc, &asidx);
     if (phys != -1) {
-        tb_invalidate_phys_addr(cpu->as,
+        tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as,
                                 phys | (pc & ~TARGET_PAGE_MASK));
     }
 }
@@ -3502,8 +3503,10 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
     target_ulong page;
 
     while (len > 0) {
+        int asidx;
+
         page = addr & TARGET_PAGE_MASK;
-        phys_addr = cpu_get_phys_page_debug(cpu, page);
+        phys_addr = cpu_get_phys_page_asidx_debug(cpu, page, &asidx);
         /* if no physical page mapped, return an error */
         if (phys_addr == -1)
             return -1;
@@ -3512,9 +3515,11 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
             l = len;
         phys_addr += (addr & ~TARGET_PAGE_MASK);
         if (is_write) {
-            cpu_physical_memory_write_rom(cpu->as, phys_addr, buf, l);
+            cpu_physical_memory_write_rom(cpu->cpu_ases[asidx].as,
+                                          phys_addr, buf, l);
         } else {
-            address_space_rw(cpu->as, phys_addr, MEMTXATTRS_UNSPECIFIED,
+            address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
+                             MEMTXATTRS_UNSPECIFIED,
                              buf, l, 0);
         }
         len -= l;
-- 
1.9.1

[Qemu-devel] [PATCH 08/16] exec.c: Have one io_mem_watch per AddressSpace

[Peter Maydell]

The io_mem_watch MemoryRegion's read and write callbacks pass the
accesses through to an underlying address space. Now that that
might be something other than address_space_memory, we need to
pass the correct AddressSpace in via the opaque pointer. That
means we need to have one io_mem_watch MemoryRegion per address
space, rather than sharing one between all address spaces.

This should also fix gdbstub watchpoints in x86 SMRAM, which would
previously not have worked correctly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 exec.c                | 40 +++++++++++++++++++++++-----------------
 include/exec/memory.h |  2 ++
 2 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/exec.c b/exec.c
index bc6ab8a..9998fa0 100644
--- a/exec.c
+++ b/exec.c
@@ -163,8 +163,6 @@ static void io_mem_init(void);
 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
@@ -334,12 +332,6 @@ static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr addr,
     }
 }
 
-bool memory_region_is_unassigned(MemoryRegion *mr)
-{
-    return mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
-        && mr != &io_mem_watch;
-}
-
 /* Called from RCU critical section */
 static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
                                                         hwaddr addr,
@@ -2045,17 +2037,18 @@ static MemTxResult watch_mem_read(void *opaque, hwaddr addr, uint64_t *pdata,
 {
     MemTxResult res;
     uint64_t data;
+    AddressSpace *as = opaque;
 
     check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_READ);
     switch (size) {
     case 1:
-        data = address_space_ldub(&address_space_memory, addr, attrs, &res);
+        data = address_space_ldub(as, addr, attrs, &res);
         break;
     case 2:
-        data = address_space_lduw(&address_space_memory, addr, attrs, &res);
+        data = address_space_lduw(as, addr, attrs, &res);
         break;
     case 4:
-        data = address_space_ldl(&address_space_memory, addr, attrs, &res);
+        data = address_space_ldl(as, addr, attrs, &res);
         break;
     default: abort();
     }
@@ -2068,17 +2061,18 @@ static MemTxResult watch_mem_write(void *opaque, hwaddr addr,
                                    MemTxAttrs attrs)
 {
     MemTxResult res;
+    AddressSpace *as = opaque;
 
     check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_WRITE);
     switch (size) {
     case 1:
-        address_space_stb(&address_space_memory, addr, val, attrs, &res);
+        address_space_stb(as, addr, val, attrs, &res);
         break;
     case 2:
-        address_space_stw(&address_space_memory, addr, val, attrs, &res);
+        address_space_stw(as, addr, val, attrs, &res);
         break;
     case 4:
-        address_space_stl(&address_space_memory, addr, val, attrs, &res);
+        address_space_stl(as, addr, val, attrs, &res);
         break;
     default: abort();
     }
@@ -2091,6 +2085,15 @@ static const MemoryRegionOps watch_mem_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+bool memory_region_is_unassigned(MemoryRegion *mr)
+{
+    /* Checking the ops pointer of the MemoryRegion is strictly
+     * speaking looking at private information of the MemoryRegion :-(
+     */
+    return mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
+        && mr->ops != &watch_mem_ops;
+}
+
 static MemTxResult subpage_read(void *opaque, hwaddr addr, uint64_t *data,
                                 unsigned len, MemTxAttrs attrs)
 {
@@ -2251,8 +2254,6 @@ static void io_mem_init(void)
                           NULL, UINT64_MAX);
     memory_region_init_io(&io_mem_notdirty, NULL, &notdirty_mem_ops, NULL,
                           NULL, UINT64_MAX);
-    memory_region_init_io(&io_mem_watch, NULL, &watch_mem_ops, NULL,
-                          NULL, UINT64_MAX);
 }
 
 static void mem_begin(MemoryListener *listener)
@@ -2267,7 +2268,7 @@ static void mem_begin(MemoryListener *listener)
     assert(n == PHYS_SECTION_NOTDIRTY);
     n = dummy_section(&d->map, as, &io_mem_rom);
     assert(n == PHYS_SECTION_ROM);
-    n = dummy_section(&d->map, as, &io_mem_watch);
+    n = dummy_section(&d->map, as, as->io_mem_watch);
     assert(n == PHYS_SECTION_WATCH);
 
     d->phys_map  = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip = 1 };
@@ -2315,6 +2316,10 @@ static void tcg_commit(MemoryListener *listener)
 
 void address_space_init_dispatch(AddressSpace *as)
 {
+    as->io_mem_watch = g_new0(MemoryRegion, 1);
+    memory_region_init_io(as->io_mem_watch, NULL, &watch_mem_ops, as,
+                          NULL, UINT64_MAX);
+
     as->dispatch = NULL;
     as->dispatch_listener = (MemoryListener) {
         .begin = mem_begin,
@@ -2339,6 +2344,7 @@ void address_space_destroy_dispatch(AddressSpace *as)
     if (d) {
         call_rcu(d, address_space_dispatch_free, rcu);
     }
+    memory_region_unref(as->io_mem_watch);
 }
 
 static void memory_map_init(void)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 0f07159..e5d98f8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -246,6 +246,8 @@ struct AddressSpace {
     struct AddressSpaceDispatch *next_dispatch;
     MemoryListener dispatch_listener;
 
+    MemoryRegion *io_mem_watch;
+
     QTAILQ_ENTRY(AddressSpace) address_spaces_link;
 };
 
-- 
1.9.1

[Qemu-devel] [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

[Peter Maydell]

If we have a secure address space, use it in page table walks:
 * when doing the physical accesses to read descriptors,
   make them through the correct address space
 * when the final result indicates a secure access, pass the
   correct address space index to tlb_set_page_with_attrs()

(The descriptor reads are the only direct physical accesses
made in target-arm/ for CPUs which might have TrustZone.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu.h    | 29 +++++++++++++++++++++++++++++
 target-arm/helper.c | 10 +++++++---
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 815fef8..8dbf4d4 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1720,6 +1720,12 @@ static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)
     return el;
 }
 
+/* Indexes used when registering address spaces with cpu_address_space_init */
+typedef enum ARMASIdx {
+    ARMASIdx_NS = 0,
+    ARMASIdx_S = 1,
+} ARMASIdx;
+
 /* Return the Exception Level targeted by debug exceptions;
  * currently always EL1 since we don't implement EL2 or EL3.
  */
@@ -1991,4 +1997,27 @@ enum {
     QEMU_PSCI_CONDUIT_HVC = 2,
 };
 
+#ifndef CONFIG_USER_ONLY
+/* Return the address space index to use for a memory access
+ * (which depends on whether the access is S or NS, and whether
+ * the board gave us a separate AddressSpace for S accesses).
+ */
+static inline int arm_asidx(CPUState *cs, bool is_secure)
+{
+    if (is_secure && cs->num_ases > 1) {
+        return ARMASIdx_S;
+    }
+    return ARMASIdx_NS;
+}
+
+/* Return the AddressSpace to use for a memory access
+ * (which depends on whether the access is S or NS, and whether
+ * the board gave us a separate AddressSpace for S accesses).
+ */
+static inline AddressSpace *arm_addressspace(CPUState *cs, bool is_secure)
+{
+    return cpu_get_address_space(cs, arm_asidx(cs, is_secure));
+}
+#endif
+
 #endif
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 174371b..242928d 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6260,13 +6260,14 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
     MemTxAttrs attrs = {};
+    AddressSpace *as = arm_addressspace(cs, is_secure);
 
     attrs.secure = is_secure;
     addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
     if (fi->s1ptw) {
         return 0;
     }
-    return address_space_ldl(cs->as, addr, attrs, NULL);
+    return address_space_ldl(as, addr, attrs, NULL);
 }
 
 static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
@@ -6276,13 +6277,14 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
     MemTxAttrs attrs = {};
+    AddressSpace *as = arm_addressspace(cs, is_secure);
 
     attrs.secure = is_secure;
     addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
     if (fi->s1ptw) {
         return 0;
     }
-    return address_space_ldq(cs->as, addr, attrs, NULL);
+    return address_space_ldq(as, addr, attrs, NULL);
 }
 
 static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
@@ -7307,6 +7309,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
     target_ulong page_size;
     int prot;
     int ret;
+    int asidx;
     MemTxAttrs attrs = {};
 
     ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
@@ -7315,7 +7318,8 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
         /* Map a single [sub]page.  */
         phys_addr &= TARGET_PAGE_MASK;
         address &= TARGET_PAGE_MASK;
-        tlb_set_page_with_attrs(cs, address, 0, phys_addr, attrs,
+        asidx = arm_asidx(cs, attrs.secure);
+        tlb_set_page_with_attrs(cs, address, asidx, phys_addr, attrs,
                                 prot, mmu_idx, page_size);
         return 0;
     }
-- 
1.9.1

[Qemu-devel] [PATCH 10/16] target-arm: Implement cpu_get_phys_page_asidx_debug

[Peter Maydell]

Implement cpu_get_phys_page_asidx_debug instead of cpu_get_phys_page_debug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu-qom.h | 2 +-
 target-arm/cpu.c     | 2 +-
 target-arm/helper.c  | 3 ++-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 25fb1ce..53d3129 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -216,7 +216,7 @@ bool arm_cpu_exec_interrupt(CPUState *cpu, int int_req);
 void arm_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
                         int flags);
 
-hwaddr arm_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
+hwaddr arm_cpu_get_phys_page_asidx_debug(CPUState *cpu, vaddr addr, int *asidx);
 
 int arm_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
 int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 30739fc..7789d50 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1417,7 +1417,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
     cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
 #else
     cc->do_interrupt = arm_cpu_do_interrupt;
-    cc->get_phys_page_debug = arm_cpu_get_phys_page_debug;
+    cc->get_phys_page_asidx_debug = arm_cpu_get_phys_page_asidx_debug;
     cc->vmsd = &vmstate_arm_cpu;
     cc->virtio_is_big_endian = arm_cpu_is_big_endian;
 #endif
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 242928d..db7e81a 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -7327,7 +7327,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
     return ret;
 }
 
-hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
+hwaddr arm_cpu_get_phys_page_asidx_debug(CPUState *cs, vaddr addr, int *asidx)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
@@ -7346,6 +7346,7 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
         return -1;
     }
 
+    *asidx = arm_asidx(cs, attrs.secure);
     return phys_addr;
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH 11/16] memory: Add address_space_init_shareable()

[Peter Maydell]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

This will either create a new AS or return a pointer to an
already existing equivalent one, if we have already created
an AS for the specified root memory region.

The motivation is to reuse address spaces as much as possible.
It's going to be quite common that bus masters out in device land
have pointers to the same memory region for their mastering yet
each will need to create its own address space. Let the memory
API implement sharing for them.

Aside from the perf optimisations, this should reduce the amount
of redundant output on info mtree as well.

Thee returned value will be malloced, but the malloc will be
automatically freed when the AS runs out of refs.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[PMM: dropped check for NULL root as unused; added doc-comment;
 squashed Peter C's reference-counting patch into this one;
 don't compare name string when deciding if we can share ASes;
 read as->malloced before the unref of as->root to avoid possible
 read-after-free if as->root was the owner of as]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memory.h | 18 ++++++++++++++++++
 memory.c              | 27 +++++++++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index e5d98f8..1d1740a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -236,6 +236,8 @@ struct AddressSpace {
     struct rcu_head rcu;
     char *name;
     MemoryRegion *root;
+    int ref_count;
+    bool malloced;
 
     /* Accessed via RCU.  */
     struct FlatView *current_map;
@@ -1167,6 +1169,22 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
  */
 void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
 
+/**
+ * address_space_init_shareable: return an address space for a memory region,
+ *                               creating it if it does not already exist
+ *
+ * @root: a #MemoryRegion that routes addresses for the address space
+ * @name: an address space name.  The name is only used for debugging
+ *        output.
+ *
+ * This function will return a pointer to an existing AddressSpace
+ * which was initialized with the specified MemoryRegion, or it will
+ * create and initialize one if it does not already exist. The ASes
+ * are reference-counted, so the memory will be freed automatically
+ * when the AddressSpace is destroyed via address_space_destroy.
+ */
+AddressSpace *address_space_init_shareable(MemoryRegion *root,
+                                           const char *name);
 
 /**
  * address_space_destroy: destroy an address space
diff --git a/memory.c b/memory.c
index c435c88..6c98a71 100644
--- a/memory.c
+++ b/memory.c
@@ -2101,7 +2101,9 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 {
     memory_region_ref(root);
     memory_region_transaction_begin();
+    as->ref_count = 1;
     as->root = root;
+    as->malloced = false;
     as->current_map = g_new(FlatView, 1);
     flatview_init(as->current_map);
     as->ioeventfd_nb = 0;
@@ -2116,6 +2118,7 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 static void do_address_space_destroy(AddressSpace *as)
 {
     MemoryListener *listener;
+    bool do_free = as->malloced;
 
     address_space_destroy_dispatch(as);
 
@@ -2127,12 +2130,36 @@ static void do_address_space_destroy(AddressSpace *as)
     g_free(as->name);
     g_free(as->ioeventfds);
     memory_region_unref(as->root);
+    if (do_free) {
+        g_free(as);
+    }
+}
+
+AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)
+{
+    AddressSpace *as;
+
+    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
+        if (root == as->root) {
+            as->ref_count++;
+            return as;
+        }
+    }
+
+    as = g_malloc0(sizeof *as);
+    address_space_init(as, root, name);
+    as->malloced = true;
+    return as;
 }
 
 void address_space_destroy(AddressSpace *as)
 {
     MemoryRegion *root = as->root;
 
+    as->ref_count--;
+    if (as->ref_count) {
+        return;
+    }
     /* Flush out anything from MemoryListeners listening in on this */
     memory_region_transaction_begin();
     as->root = NULL;
-- 
1.9.1

[Qemu-devel] [PATCH 12/16] qom/cpu: Add MemoryRegion property

[Peter Maydell]

From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Add a MemoryRegion property, which if set is used to construct
the CPU's initial (default) AddressSpace.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
[PMM: code is moved from qom/cpu.c to exec.c to avoid having to
 make qom/cpu.o be a non-common object file; code to use the
 MemoryRegion and to default it to system_memory added.]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpus.c            |  4 +++-
 exec.c            | 13 +++++++++++++
 include/qom/cpu.h |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index 764ea75..b7c19e0 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1356,7 +1356,9 @@ void qemu_init_vcpu(CPUState *cpu)
         /* If the target cpu hasn't set up any address spaces itself,
          * give it the default one.
          */
-        cpu_address_space_init(cpu, &address_space_memory, 0);
+        AddressSpace *as = address_space_init_shareable(cpu->memory,
+                                                        "cpu-memory");
+        cpu_address_space_init(cpu, as, 0);
     }
 
     if (kvm_enabled()) {
diff --git a/exec.c b/exec.c
index 9998fa0..21b1b57 100644
--- a/exec.c
+++ b/exec.c
@@ -637,6 +637,19 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
 
 #ifndef CONFIG_USER_ONLY
     cpu->thread_id = qemu_get_thread_id();
+
+    /* This is a softmmu CPU object, so create a property for it
+     * so users can wire up its memory. (This can't go in qom/cpu.c
+     * because that file is compiled only once for both user-mode
+     * and system builds.) The default if no link is set up is to use
+     * the system address space.
+     */
+    object_property_add_link(OBJECT(cpu), "memory", TYPE_MEMORY_REGION,
+                             (Object **)&cpu->memory,
+                             qdev_prop_allow_set_link_before_realize,
+                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                             &error_abort);
+    cpu->memory = system_memory;
 #endif
 
 #if defined(CONFIG_USER_ONLY)
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 10ef5cc..39a9a24 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -292,6 +292,7 @@ struct CPUState {
     CPUAddressSpace *cpu_ases;
     int num_ases;
     AddressSpace *as;
+    MemoryRegion *memory;
 
     void *env_ptr; /* CPUArchState */
     struct TranslationBlock *current_tb;
-- 
1.9.1

[Qemu-devel] [PATCH 13/16] target-arm: Add QOM property for Secure memory region

[Peter Maydell]

Add QOM property to the ARM CPU which boards can use to tell us what
memory region to use for secure accesses. Nonsecure accesses
go via the memory region specified with the base CPU class 'memory'
property.

By default, if no secure region is specified it is the same as the
nonsecure region, and if no nonsecure region is specified we will use
address_space_memory.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/cpu-qom.h |  3 +++
 target-arm/cpu.c     | 26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 53d3129..75dc74e 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -87,6 +87,9 @@ typedef struct ARMCPU {
     /* GPIO outputs for generic timer */
     qemu_irq gt_timer_outputs[NUM_GTIMERS];
 
+    /* MemoryRegion to use for secure physical accesses */
+    MemoryRegion *secure_memory;
+
     /* 'compatible' string for this CPU for Linux device trees */
     const char *dtb_compatible;
 
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 7789d50..85e2bc8 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -542,6 +542,15 @@ static void arm_cpu_post_init(Object *obj)
          */
         qdev_property_add_static(DEVICE(obj), &arm_cpu_has_el3_property,
                                  &error_abort);
+
+#ifndef CONFIG_USER_ONLY
+        object_property_add_link(obj, "secure-memory",
+                                 TYPE_MEMORY_REGION,
+                                 (Object **)&cpu->secure_memory,
+                                 qdev_prop_allow_set_link_before_realize,
+                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
+                                 &error_abort);
+#endif
     }
 
     if (arm_feature(&cpu->env, ARM_FEATURE_MPU)) {
@@ -665,6 +674,23 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
 
     init_cpreg_list(cpu);
 
+#ifndef CONFIG_USER_ONLY
+    if (cpu->has_el3) {
+        AddressSpace *as;
+
+        if (!cpu->secure_memory) {
+            cpu->secure_memory = cs->memory;
+        }
+        as = address_space_init_shareable(cpu->secure_memory,
+                                          "cpu-secure-memory");
+        cpu_address_space_init(cs, as, ARMASIdx_S);
+    }
+    cpu_address_space_init(cs,
+                           address_space_init_shareable(cs->memory,
+                                                        "cpu-memory"),
+                           ARMASIdx_NS);
+#endif
+
     qemu_init_vcpu(cs);
     cpu_reset(cs);
 
-- 
1.9.1

[Qemu-devel] [PATCH 14/16] hw/arm/virt: Wire up memory region to CPUs explicitly

[Peter Maydell]

Wire up the system memory region to the CPUs explicitly
by setting the QOM property. This doesn't change anything
over letting it default, but will be needed for adding
a secure memory region later.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 77d9267..3ab31e0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1020,6 +1020,9 @@ static void machvirt_init(MachineState *machine)
                                     "reset-cbar", &error_abort);
         }
 
+        object_property_set_link(cpuobj, OBJECT(sysmem), "memory",
+                                 &error_abort);
+
         object_property_set_bool(cpuobj, true, "realized", NULL);
     }
     g_strfreev(cpustr);
-- 
1.9.1

[Qemu-devel] [PATCH 15/16] [RFC] hw/arm/virt: add secure memory region and UART

[Peter Maydell]

Add a secure memory region to the virt board, which is the
same as the nonsecure memory region except that it also has
a secure-only UART in it. This is only created if the
board is started with the '-machine secure=on' property.

This is an RFC patch, beacuse the device tree bindings for
indicating secure vs nonsecure devices are still under
discussion upstream:
      https://lkml.org/lkml/2015/10/29/287

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c         | 55 ++++++++++++++++++++++++++++++++++++++++++++-------
 include/hw/arm/virt.h |  1 +
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 3ab31e0..162896f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -120,6 +120,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
+    [VIRT_SECURE_UART] =        { 0x09030000, 0x00001000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -135,6 +136,7 @@ static const int a15irqmap[] = {
     [VIRT_UART] = 1,
     [VIRT_RTC] = 2,
     [VIRT_PCIE] = 3, /* ... to 6 */
+    [VIRT_SECURE_UART] = 7,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
     [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
@@ -485,16 +487,22 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic, int type, bool secure)
     }
 }
 
-static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
+static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic, int uart,
+                        MemoryRegion *mem)
 {
     char *nodename;
-    hwaddr base = vbi->memmap[VIRT_UART].base;
-    hwaddr size = vbi->memmap[VIRT_UART].size;
-    int irq = vbi->irqmap[VIRT_UART];
+    hwaddr base = vbi->memmap[uart].base;
+    hwaddr size = vbi->memmap[uart].size;
+    int irq = vbi->irqmap[uart];
     const char compat[] = "arm,pl011\0arm,primecell";
     const char clocknames[] = "uartclk\0apb_pclk";
+    DeviceState *dev = qdev_create(NULL, "pl011");
+    SysBusDevice *s = SYS_BUS_DEVICE(dev);
 
-    sysbus_create_simple("pl011", base, pic[irq]);
+    qdev_init_nofail(dev);
+    memory_region_add_subregion(mem, base,
+                                sysbus_mmio_get_region(s, 0));
+    sysbus_connect_irq(s, 0, pic[irq]);
 
     nodename = g_strdup_printf("/pl011@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
@@ -511,7 +519,14 @@ static void create_uart(const VirtBoardInfo *vbi, qemu_irq *pic)
     qemu_fdt_setprop(vbi->fdt, nodename, "clock-names",
                          clocknames, sizeof(clocknames));
 
-    qemu_fdt_setprop_string(vbi->fdt, "/chosen", "stdout-path", nodename);
+    if (uart == VIRT_UART) {
+        qemu_fdt_setprop_string(vbi->fdt, "/chosen", "stdout-path", nodename);
+    } else {
+        /* Mark as not usable by the normal world */
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "status", "secure");
+        qemu_fdt_setprop_string(vbi->fdt, nodename, "secure-status", "okay");
+    }
+
     g_free(nodename);
 }
 
@@ -922,6 +937,7 @@ static void machvirt_init(MachineState *machine)
     VirtMachineState *vms = VIRT_MACHINE(machine);
     qemu_irq pic[NUM_IRQS];
     MemoryRegion *sysmem = get_system_memory();
+    MemoryRegion *secure_sysmem = NULL;
     int gic_version = vms->gic_version;
     int n, max_cpus;
     MemoryRegion *ram = g_new(MemoryRegion, 1);
@@ -980,6 +996,23 @@ static void machvirt_init(MachineState *machine)
         exit(1);
     }
 
+    if (vms->secure) {
+        if (kvm_enabled()) {
+            error_report("mach-virt: KVM does not support Security extensions");
+            exit(1);
+        }
+
+        /* The Secure view of the world is the same as the NonSecure,
+         * but with a few extra devices. Create it as a container region
+         * containing the system memory at low priority; any secure-only
+         * devices go in at higher priority and take precedence.
+         */
+        secure_sysmem = g_new(MemoryRegion, 1);
+        memory_region_init(secure_sysmem, OBJECT(machine), "secure-memory",
+                           UINT64_MAX);
+        memory_region_add_subregion_overlap(secure_sysmem, 0, sysmem, -1);
+    }
+
     create_fdt(vbi);
 
     for (n = 0; n < smp_cpus; n++) {
@@ -1022,6 +1055,10 @@ static void machvirt_init(MachineState *machine)
 
         object_property_set_link(cpuobj, OBJECT(sysmem), "memory",
                                  &error_abort);
+        if (vms->secure) {
+            object_property_set_link(cpuobj, OBJECT(secure_sysmem),
+                                     "secure-memory", &error_abort);
+        }
 
         object_property_set_bool(cpuobj, true, "realized", NULL);
     }
@@ -1038,7 +1075,11 @@ static void machvirt_init(MachineState *machine)
 
     create_gic(vbi, pic, gic_version, vms->secure);
 
-    create_uart(vbi, pic);
+    create_uart(vbi, pic, VIRT_UART, sysmem);
+
+    if (vms->secure) {
+        create_uart(vbi, pic, VIRT_SECURE_UART, secure_sysmem);
+    }
 
     create_rtc(vbi, pic);
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index f464586..9622e9f 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -59,6 +59,7 @@ enum {
     VIRT_PCIE_ECAM,
     VIRT_PLATFORM_BUS,
     VIRT_PCIE_MMIO_HIGH,
+    VIRT_SECURE_UART,
 };
 
 typedef struct MemMapEntry {
-- 
1.9.1

[Qemu-devel] [PATCH 16/16] HACK: rearrange the virt memory map to suit OP-TEE

[Peter Maydell]

The current OP-TEE codebase expects the secure UART to
be at 0x09010000 and irq 2 (it is based on an old
non-upstream patch to add a second uart, and upstream
used that memory map area for something else). When
the TZ support is upstream in QEMU we can move OP-TEE
on to a proper upstream QEMU and update it to use the
new UART location, but for now this hack patch allows
running a more-or-less unmodified OP-TEE.

Put the secure UART at the address and irq where OP-TEE
expects it, moving some other devices down to make space.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 162896f..a6b87ba 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -118,9 +118,9 @@ static const MemMapEntry a15memmap[] = {
     /* This redistributor space allows up to 2*64kB*123 CPUs */
     [VIRT_GIC_REDIST] =         { 0x080A0000, 0x00F60000 },
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
-    [VIRT_RTC] =                { 0x09010000, 0x00001000 },
-    [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
-    [VIRT_SECURE_UART] =        { 0x09030000, 0x00001000 },
+    [VIRT_RTC] =                { 0x09020000, 0x00001000 },
+    [VIRT_FW_CFG] =             { 0x09030000, 0x00000018 },
+    [VIRT_SECURE_UART] =        { 0x09010000, 0x00001000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -134,9 +134,9 @@ static const MemMapEntry a15memmap[] = {
 
 static const int a15irqmap[] = {
     [VIRT_UART] = 1,
-    [VIRT_RTC] = 2,
-    [VIRT_PCIE] = 3, /* ... to 6 */
-    [VIRT_SECURE_UART] = 7,
+    [VIRT_RTC] = 3,
+    [VIRT_PCIE] = 4, /* ... to 7 */
+    [VIRT_SECURE_UART] = 2,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
     [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
-- 
1.9.1

Re: [Qemu-devel] [PATCH 01/16] exec.c: Don't set cpu->as until cpu_address_space_init

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:43PM +0000, Peter Maydell wrote:
> Rather than setting cpu->as unconditionally in cpu_exec_init
> (and then having target-i386 override this later), don't set
> it until the first call to cpu_address_space_init.
> 
> This requires us to initialise the address space for
> both TCG and KVM (KVM doesn't need the AS listener but
> it does require cpu->as to be set).
> 
> For target CPUs which don't set up any address spaces (currently
> everything except i386), add the default address_space_memory
> in qemu_init_vcpu().


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  cpus.c                  | 10 ++++++++--
>  exec.c                  | 16 ++++++++++++----
>  include/exec/exec-all.h | 16 +++++++++++++++-
>  target-i386/cpu.c       |  6 ++++--
>  4 files changed, 39 insertions(+), 9 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index c6a5d0e..764ea75 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1291,8 +1291,6 @@ static void qemu_tcg_init_vcpu(CPUState *cpu)
>      static QemuCond *tcg_halt_cond;
>      static QemuThread *tcg_cpu_thread;
>  
> -    tcg_cpu_address_space_init(cpu, cpu->as);
> -
>      /* share a single thread for all cpus with TCG */
>      if (!tcg_cpu_thread) {
>          cpu->thread = g_malloc0(sizeof(QemuThread));
> @@ -1353,6 +1351,14 @@ void qemu_init_vcpu(CPUState *cpu)
>      cpu->nr_cores = smp_cores;
>      cpu->nr_threads = smp_threads;
>      cpu->stopped = true;
> +
> +    if (!cpu->as) {
> +        /* If the target cpu hasn't set up any address spaces itself,
> +         * give it the default one.
> +         */
> +        cpu_address_space_init(cpu, &address_space_memory, 0);
> +    }
> +
>      if (kvm_enabled()) {
>          qemu_kvm_start_vcpu(cpu);
>      } else if (tcg_enabled()) {
> diff --git a/exec.c b/exec.c
> index 1e8b51b..b5490c8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -550,8 +550,13 @@ CPUState *qemu_get_cpu(int index)
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
> +void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx)
>  {
> +    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);
>  
> @@ -563,8 +568,10 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *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);
> +    if (tcg_enabled()) {
> +        cpu->cpu_ases[0].tcg_as_listener.commit = tcg_commit;
> +        memory_listener_register(&cpu->cpu_ases[0].tcg_as_listener, as);
> +    }
>  }
>  #endif
>  
> @@ -619,8 +626,9 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      int cpu_index;
>      Error *local_err = NULL;
>  
> +    cpu->as = NULL;
> +
>  #ifndef CONFIG_USER_ONLY
> -    cpu->as = &address_space_memory;
>      cpu->thread_id = qemu_get_thread_id();
>  #endif
>  
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 9b93b9b..90130ca 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -85,7 +85,21 @@ void QEMU_NORETURN cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc);
>  
>  #if !defined(CONFIG_USER_ONLY)
>  void cpu_reloading_memory_map(void);
> -void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as);
> +/**
> + * cpu_address_space_init:
> + * @cpu: CPU to add this address space to
> + * @as: address space to add
> + * @asidx: integer index of this address space
> + *
> + * Add the specified address space to the CPU's cpu_ases list.
> + * The address space added with @asidx 0 is the one used for the
> + * convenience pointer cpu->as.
> + * The target-specific code which registers ASes is responsible
> + * for defining what semantics address space 0, 1, 2, etc have.
> + *
> + * Note that with KVM only one address space is supported.
> + */
> +void cpu_address_space_init(CPUState *cpu, AddressSpace *as, int asidx);
>  /* cputlb.c */
>  /**
>   * tlb_flush_page:
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9d0eedf..8096860 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2856,9 +2856,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>  #ifndef CONFIG_USER_ONLY
>      if (tcg_enabled()) {
> +        AddressSpace *newas = g_new(AddressSpace, 1);
> +
>          cpu->cpu_as_mem = g_new(MemoryRegion, 1);
>          cpu->cpu_as_root = g_new(MemoryRegion, 1);
> -        cs->as = g_new(AddressSpace, 1);
>  
>          /* Outer container... */
>          memory_region_init(cpu->cpu_as_root, OBJECT(cpu), "memory", ~0ull);
> @@ -2871,7 +2872,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>                                   get_system_memory(), 0, ~0ull);
>          memory_region_add_subregion_overlap(cpu->cpu_as_root, 0, cpu->cpu_as_mem, 0);
>          memory_region_set_enabled(cpu->cpu_as_mem, true);
> -        address_space_init(cs->as, cpu->cpu_as_root, "CPU");
> +        address_space_init(newas, cpu->cpu_as_root, "CPU");
> +        cpu_address_space_init(cs, newas, 0);
>  
>          /* ... SMRAM with higher priority, linked from /machine/smram.  */
>          cpu->machine_done.notify = x86_cpu_machine_done;
> -- 
> 1.9.1
> 

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

[Edgar E. Iglesias]

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
> 

Re: [Qemu-devel] [PATCH 03/16] tlb_set_page_with_attrs: Take argument specifying AddressSpace to use

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:45PM +0000, Peter Maydell wrote:
> Add an argument to tlb_set_page_with_attrs which allows the target CPU code
> to tell the core code which AddressSpace to use.
> 
> The AddressSpace is specified by the index into the array of ASes which
> were registered with cpu_address_space_init().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  cputlb.c                |  6 +++---
>  exec.c                  |  7 ++++---
>  include/exec/exec-all.h | 33 ++++++++++++++++++++++++++++++---
>  target-arm/helper.c     |  2 +-
>  target-i386/helper.c    |  2 +-
>  5 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index bf1d50a..e753083 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -343,7 +343,7 @@ static void tlb_add_large_page(CPUArchState *env, target_ulong vaddr,
>   * Called from TCG-generated code, which is under an RCU read-side
>   * critical section.
>   */
> -void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
> +void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, int asidx,
>                               hwaddr paddr, MemTxAttrs attrs, int prot,
>                               int mmu_idx, target_ulong size)
>  {
> @@ -363,7 +363,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
>      }
>  
>      sz = size;
> -    section = address_space_translate_for_iotlb(cpu, paddr, &xlat, &sz);
> +    section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz);
>      assert(sz >= TARGET_PAGE_SIZE);
>  
>  #if defined(DEBUG_TLB)
> @@ -433,7 +433,7 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                    hwaddr paddr, int prot,
>                    int mmu_idx, target_ulong size)
>  {
> -    tlb_set_page_with_attrs(cpu, vaddr, paddr, MEMTXATTRS_UNSPECIFIED,
> +    tlb_set_page_with_attrs(cpu, vaddr, 0, paddr, MEMTXATTRS_UNSPECIFIED,
>                              prot, mmu_idx, size);
>  }
>  
> diff --git a/exec.c b/exec.c
> index 6a2a694..92e76fa 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -445,12 +445,13 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>  
>  /* Called from RCU critical section */
>  MemoryRegionSection *
> -address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr,
> +address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
>                                    hwaddr *xlat, hwaddr *plen)

Does it make sense to replace the CPUState argument with an AddressSpace *
and have the callers do the cpu->cpu_ases[asidx]?
It would be more consistent and eventually maybe eliminate the need for
address_space_translate_for_iotlb in favor of calling address_space_translate
directly?



>  {
>      MemoryRegionSection *section;
> -    section = address_space_translate_internal(cpu->cpu_ases[0].memory_dispatch,
> -                                               addr, xlat, plen, false);
> +    AddressSpaceDispatch *d = cpu->cpu_ases[asidx].memory_dispatch;
> +
> +    section = address_space_translate_internal(d, addr, xlat, plen, false);
>  
>      assert(!section->mr->iommu_ops);
>      return section;
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 90130ca..472d0fc 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -144,7 +144,34 @@ void tlb_flush_by_mmuidx(CPUState *cpu, ...);
>  void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                    hwaddr paddr, int prot,
>                    int mmu_idx, target_ulong size);
> -void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
> +/**
> + * tlb_set_page_with_attrs:
> + * @cpu: CPU to add this TLB entry for
> + * @vaddr: virtual address of page to add entry for
> + * @asidx: index of the AddressSpace the physical address is in
> + * @paddr: physical address of the page
> + * @attrs: memory transaction attributes
> + * @prot: access permissions (PAGE_READ/PAGE_WRITE/PAGE_EXEC bits)
> + * @mmu_idx: MMU index to insert TLB entry for
> + * @size: size of the page in bytes
> + *
> + * Add an entry to this CPU's TLB (a mapping from virtual address
> + * @vaddr to physical address @paddr) with the specified memory
> + * transaction attributes. This is generally called by the target CPU
> + * specific code after it has been called through the tlb_fill()
> + * entry point and performed a successful page table walk to find
> + * the physical address and attributes for the virtual address
> + * which provoked the TLB miss.
> + *
> + * At most one entry for a given virtual address is permitted. Only a
> + * single TARGET_PAGE_SIZE region is mapped; the supplied @size is only
> + * used by tlb_flush_page.
> + *
> + * @asidx is the index of the AddressSpace in the cpu->ases array;
> + * if the CPU does not support multiple AddressSpaces then it will
> + * always be zero.
> + */
> +void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, int asidx,
>                               hwaddr paddr, MemTxAttrs attrs,
>                               int prot, int mmu_idx, target_ulong size);
>  void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr);
> @@ -400,8 +427,8 @@ void tlb_set_dirty(CPUState *cpu, target_ulong vaddr);
>  void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
>  
>  MemoryRegionSection *
> -address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr, hwaddr *xlat,
> -                                  hwaddr *plen);
> +address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
> +                                  hwaddr *xlat, hwaddr *plen);
>  hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>                                         MemoryRegionSection *section,
>                                         target_ulong vaddr,
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4ecae61..174371b 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7315,7 +7315,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>          /* Map a single [sub]page.  */
>          phys_addr &= TARGET_PAGE_MASK;
>          address &= TARGET_PAGE_MASK;
> -        tlb_set_page_with_attrs(cs, address, phys_addr, attrs,
> +        tlb_set_page_with_attrs(cs, address, 0, phys_addr, attrs,
>                                  prot, mmu_idx, page_size);
>          return 0;
>      }
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index d18be95..1c43717 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -962,7 +962,7 @@ do_check_protect_pse36:
>      page_offset = vaddr & (page_size - 1);
>      paddr = pte + page_offset;
>  
> -    tlb_set_page_with_attrs(cs, vaddr, paddr, cpu_get_mem_attrs(env),
> +    tlb_set_page_with_attrs(cs, vaddr, 0, paddr, cpu_get_mem_attrs(env),
>                              prot, mmu_idx, page_size);
>      return 0;
>   do_fault_rsvd:
> -- 
> 1.9.1
> 

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

[Peter Maydell]

On 6 November 2015 at 13:21, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> 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>

>> +    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.

Oops, yes, you're right.

Maybe we should just have the target CPU say in advance what the
maximum number of AddressSpaces it will have is -- my expectation
is that this will be (a) small (b) known in advance anyway.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/16] exec.c: Add address space index to CPUIOTLBEntry

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:46PM +0000, Peter Maydell wrote:
> Add the address space index to CPUIOTLBEntry, and use this to pass it
> to iotlb_to_region(), so that we use the correct AddressSpace when
> doing IO path lookups.

Hi Peter,

I think this works but eventually when we add support for CPUs behind
IOMMUs I think things will be easier if we store a pointer to the
AS instead of an index in the IOTLB. address_space_translate_for_iotlb()
with IOMMU support may be implemented so that it returns a different
address space as it walks tha memory hierarchy and translate things.

Do you see any issues with storing pointers to the AS instead of an
index?

Cheers,
Edgar


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  cputlb.c                | 7 +++++--
>  exec.c                  | 4 ++--
>  include/exec/cpu-defs.h | 1 +
>  include/exec/exec-all.h | 2 +-
>  softmmu_template.h      | 4 ++--
>  5 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index e753083..ae55035 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -397,6 +397,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, int asidx,
>      /* refill the tlb */
>      env->iotlb[mmu_idx][index].addr = iotlb - vaddr;
>      env->iotlb[mmu_idx][index].attrs = attrs;
> +    env->iotlb[mmu_idx][index].asidx = asidx;
>      te->addend = addend - vaddr;
>      if (prot & PAGE_READ) {
>          te->addr_read = address;
> @@ -448,6 +449,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
>      void *p;
>      MemoryRegion *mr;
>      CPUState *cpu = ENV_GET_CPU(env1);
> +    CPUIOTLBEntry *iotlbentry;
>  
>      page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>      mmu_idx = cpu_mmu_index(env1, true);
> @@ -455,8 +457,9 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
>                   (addr & TARGET_PAGE_MASK))) {
>          cpu_ldub_code(env1, addr);
>      }
> -    pd = env1->iotlb[mmu_idx][page_index].addr & ~TARGET_PAGE_MASK;
> -    mr = iotlb_to_region(cpu, pd);
> +    iotlbentry = &env1->iotlb[mmu_idx][page_index];
> +    pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
> +    mr = iotlb_to_region(cpu, pd, iotlbentry->asidx);
>      if (memory_region_is_unassigned(mr)) {
>          CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> diff --git a/exec.c b/exec.c
> index 92e76fa..5e78d82 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2228,9 +2228,9 @@ static uint16_t dummy_section(PhysPageMap *map, AddressSpace *as,
>      return phys_section_add(map, &section);
>  }
>  
> -MemoryRegion *iotlb_to_region(CPUState *cpu, hwaddr index)
> +MemoryRegion *iotlb_to_region(CPUState *cpu, hwaddr index, int asidx)
>  {
> -    CPUAddressSpace *cpuas = &cpu->cpu_ases[0];
> +    CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
>      AddressSpaceDispatch *d = atomic_rcu_read(&cpuas->memory_dispatch);
>      MemoryRegionSection *sections = d->map.sections;
>  
> diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
> index 5093be2..d102d79 100644
> --- a/include/exec/cpu-defs.h
> +++ b/include/exec/cpu-defs.h
> @@ -129,6 +129,7 @@ QEMU_BUILD_BUG_ON(sizeof(CPUTLBEntry) != (1 << CPU_TLB_ENTRY_BITS));
>  typedef struct CPUIOTLBEntry {
>      hwaddr addr;
>      MemTxAttrs attrs;
> +    int asidx;
>  } CPUIOTLBEntry;
>  
>  #define CPU_COMMON_TLB \
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 472d0fc..5dba8aa 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -398,7 +398,7 @@ extern uintptr_t tci_tb_ptr;
>  #if !defined(CONFIG_USER_ONLY)
>  
>  struct MemoryRegion *iotlb_to_region(CPUState *cpu,
> -                                     hwaddr index);
> +                                     hwaddr index, int asidx);
>  
>  void tlb_fill(CPUState *cpu, target_ulong addr, int is_write, int mmu_idx,
>                uintptr_t retaddr);
> diff --git a/softmmu_template.h b/softmmu_template.h
> index 6803890..31a0f62 100644
> --- a/softmmu_template.h
> +++ b/softmmu_template.h
> @@ -150,7 +150,7 @@ static inline DATA_TYPE glue(io_read, SUFFIX)(CPUArchState *env,
>      uint64_t val;
>      CPUState *cpu = ENV_GET_CPU(env);
>      hwaddr physaddr = iotlbentry->addr;
> -    MemoryRegion *mr = iotlb_to_region(cpu, physaddr);
> +    MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->asidx);
>  
>      physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
>      cpu->mem_io_pc = retaddr;
> @@ -357,7 +357,7 @@ static inline void glue(io_write, SUFFIX)(CPUArchState *env,
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
>      hwaddr physaddr = iotlbentry->addr;
> -    MemoryRegion *mr = iotlb_to_region(cpu, physaddr);
> +    MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->asidx);
>  
>      physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
>      if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 06/16] include/qom/cpu.h: Add new get_phys_page_asidx_debug method

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:48PM +0000, Peter Maydell wrote:
> Add a new optional method get_phys_page_asidx_debug to CPUClass.
> This is like the existing get_phys_page_debug, but also returns
> the address space index to use for the access. This is necessary
> for CPUs which have multiple address spaces.
> 
> We provide a wrapper function cpu_get_phys_page_asidx_debug()
> which falls back to the existing get_phys_page_debug(), so we
> don't need to change every target CPU.


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/qom/cpu.h | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index ae17932..10ef5cc 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -98,6 +98,9 @@ struct TranslationBlock;
>   * #TranslationBlock.
>   * @handle_mmu_fault: Callback for handling an MMU fault.
>   * @get_phys_page_debug: Callback for obtaining a physical address.
> + * @get_phys_page_asidx_debug: Callback for obtaining a physical address and the
> + *       associated address index. CPUs which have more than one AddressSpace
> + *       should implement this instead of get_phys_page_debug.
>   * @gdb_read_register: Callback for letting GDB read a register.
>   * @gdb_write_register: Callback for letting GDB write a register.
>   * @debug_excp_handler: Callback for handling debug exceptions.
> @@ -152,6 +155,7 @@ typedef struct CPUClass {
>      int (*handle_mmu_fault)(CPUState *cpu, vaddr address, int rw,
>                              int mmu_index);
>      hwaddr (*get_phys_page_debug)(CPUState *cpu, vaddr addr);
> +    hwaddr (*get_phys_page_asidx_debug)(CPUState *cpu, vaddr addr, int *asidx);
>      int (*gdb_read_register)(CPUState *cpu, uint8_t *buf, int reg);
>      int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
>      void (*debug_excp_handler)(CPUState *cpu);
> @@ -445,6 +449,32 @@ void cpu_dump_statistics(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
>  
>  #ifndef CONFIG_USER_ONLY
>  /**
> + * cpu_get_phys_page_asidx_debug:
> + * @cpu: The CPU to obtain the physical page address for.
> + * @addr: The virtual address.
> + * @asidx: Updated on return with the address space index for this access.
> + *
> + * Obtains the physical page corresponding to a virtual one, together
> + * with the corresponding address space index indicating which AddressSpace
> + * to look the physical address up in.
> + * Use it only for debugging because no protection checks are done.
> + *
> + * Returns: Corresponding physical page address or -1 if no page found.
> + */
> +static inline hwaddr cpu_get_phys_page_asidx_debug(CPUState *cpu, vaddr addr,
> +                                                   int *asidx)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->get_phys_page_asidx_debug) {
> +        return cc->get_phys_page_asidx_debug(cpu, addr, asidx);
> +    }
> +    /* Fallback for CPUs which don't have multiple address spaces */
> +    *asidx = 0;
> +    return cc->get_phys_page_debug(cpu, addr);
> +}
> +
> +/**
>   * cpu_get_phys_page_debug:
>   * @cpu: The CPU to obtain the physical page address for.
>   * @addr: The virtual address.
> @@ -456,9 +486,9 @@ void cpu_dump_statistics(CPUState *cpu, FILE *f, fprintf_function cpu_fprintf,
>   */
>  static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    int asidx;
>  
> -    return cc->get_phys_page_debug(cpu, addr);
> +    return cpu_get_phys_page_asidx_debug(cpu, addr, &asidx);
>  }
>  #endif
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 07/16] exec.c: Use cpu_get_phys_page_asidx_debug

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:49PM +0000, Peter Maydell wrote:
> Use cpu_get_phys_page_asidx_debug() when doing virtual-to-physical
> conversions in debug related code, so that we can obtain the right
> address space index and thus select the correct AddressSpace,
> rather than always using cpu->as.


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  exec.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 13da780..bc6ab8a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -684,9 +684,10 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
>  #else
>  static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
>  {
> -    hwaddr phys = cpu_get_phys_page_debug(cpu, pc);
> +    int asidx;
> +    hwaddr phys = cpu_get_phys_page_asidx_debug(cpu, pc, &asidx);
>      if (phys != -1) {
> -        tb_invalidate_phys_addr(cpu->as,
> +        tb_invalidate_phys_addr(cpu->cpu_ases[asidx].as,
>                                  phys | (pc & ~TARGET_PAGE_MASK));
>      }
>  }
> @@ -3502,8 +3503,10 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>      target_ulong page;
>  
>      while (len > 0) {
> +        int asidx;
> +
>          page = addr & TARGET_PAGE_MASK;
> -        phys_addr = cpu_get_phys_page_debug(cpu, page);
> +        phys_addr = cpu_get_phys_page_asidx_debug(cpu, page, &asidx);
>          /* if no physical page mapped, return an error */
>          if (phys_addr == -1)
>              return -1;
> @@ -3512,9 +3515,11 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
>              l = len;
>          phys_addr += (addr & ~TARGET_PAGE_MASK);
>          if (is_write) {
> -            cpu_physical_memory_write_rom(cpu->as, phys_addr, buf, l);
> +            cpu_physical_memory_write_rom(cpu->cpu_ases[asidx].as,
> +                                          phys_addr, buf, l);
>          } else {
> -            address_space_rw(cpu->as, phys_addr, MEMTXATTRS_UNSPECIFIED,
> +            address_space_rw(cpu->cpu_ases[asidx].as, phys_addr,
> +                             MEMTXATTRS_UNSPECIFIED,
>                               buf, l, 0);
>          }
>          len -= l;
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 03/16] tlb_set_page_with_attrs: Take argument specifying AddressSpace to use

[Peter Maydell]

On 6 November 2015 at 13:27, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Thu, Nov 05, 2015 at 06:15:45PM +0000, Peter Maydell wrote:
>> Add an argument to tlb_set_page_with_attrs which allows the target CPU code
>> to tell the core code which AddressSpace to use.
>>
>> The AddressSpace is specified by the index into the array of ASes which
>> were registered with cpu_address_space_init().

>> --- a/exec.c
>> +++ b/exec.c
>> @@ -445,12 +445,13 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>>
>>  /* Called from RCU critical section */
>>  MemoryRegionSection *
>> -address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr,
>> +address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
>>                                    hwaddr *xlat, hwaddr *plen)
>
> Does it make sense to replace the CPUState argument with an AddressSpace *
> and have the callers do the cpu->cpu_ases[asidx]?
> It would be more consistent and eventually maybe eliminate the need for
> address_space_translate_for_iotlb in favor of calling address_space_translate
> directly?

We can't accept an arbitrary AddressSpace, it has to be one which is
embedded in a CPUAddressSpace and which we can thus find the
memory_dispatch for. So you could pass a CPUAddressSpace*, but not
an AddressSpace*. But to pass a CPUAddressSpace we would have to
expose the currently-private-to-exec.c layout of the CPUAddressSpace
struct. I chose not to do that (and you can see the results elsewhere
in the patch series, like the function that's basically just "do
the cs_ases array lookup for me"); there's an argument for making
the structure more widely available to avoid some of that.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/16] exec.c: Add address space index to CPUIOTLBEntry

[Peter Maydell]

On 6 November 2015 at 13:34, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Thu, Nov 05, 2015 at 06:15:46PM +0000, Peter Maydell wrote:
>> Add the address space index to CPUIOTLBEntry, and use this to pass it
>> to iotlb_to_region(), so that we use the correct AddressSpace when
>> doing IO path lookups.
>
> Hi Peter,
>
> I think this works but eventually when we add support for CPUs behind
> IOMMUs I think things will be easier if we store a pointer to the
> AS instead of an index in the IOTLB. address_space_translate_for_iotlb()
> with IOMMU support may be implemented so that it returns a different
> address space as it walks tha memory hierarchy and translate things.
>
> Do you see any issues with storing pointers to the AS instead of an
> index?

Yeah, we can't deal with arbitrary AddressSpaces here. We need a
CPUAddressSpace, because we use the cached RCU-protected memory_dispatch
pointer. Once you know that you have to be dealing with one of the fixed
CPUAddressSpaces, then you might as well just keep the index rather than
an arbitrary pointer.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/16] exec.c: Have one io_mem_watch per AddressSpace

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:50PM +0000, Peter Maydell wrote:
> The io_mem_watch MemoryRegion's read and write callbacks pass the
> accesses through to an underlying address space. Now that that
> might be something other than address_space_memory, we need to
> pass the correct AddressSpace in via the opaque pointer. That
> means we need to have one io_mem_watch MemoryRegion per address
> space, rather than sharing one between all address spaces.
> 
> This should also fix gdbstub watchpoints in x86 SMRAM, which would
> previously not have worked correctly.


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  exec.c                | 40 +++++++++++++++++++++++-----------------
>  include/exec/memory.h |  2 ++
>  2 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index bc6ab8a..9998fa0 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -163,8 +163,6 @@ static void io_mem_init(void);
>  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
> @@ -334,12 +332,6 @@ static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr addr,
>      }
>  }
>  
> -bool memory_region_is_unassigned(MemoryRegion *mr)
> -{
> -    return mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
> -        && mr != &io_mem_watch;
> -}
> -
>  /* Called from RCU critical section */
>  static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>                                                          hwaddr addr,
> @@ -2045,17 +2037,18 @@ static MemTxResult watch_mem_read(void *opaque, hwaddr addr, uint64_t *pdata,
>  {
>      MemTxResult res;
>      uint64_t data;
> +    AddressSpace *as = opaque;
>  
>      check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_READ);
>      switch (size) {
>      case 1:
> -        data = address_space_ldub(&address_space_memory, addr, attrs, &res);
> +        data = address_space_ldub(as, addr, attrs, &res);
>          break;
>      case 2:
> -        data = address_space_lduw(&address_space_memory, addr, attrs, &res);
> +        data = address_space_lduw(as, addr, attrs, &res);
>          break;
>      case 4:
> -        data = address_space_ldl(&address_space_memory, addr, attrs, &res);
> +        data = address_space_ldl(as, addr, attrs, &res);
>          break;
>      default: abort();
>      }
> @@ -2068,17 +2061,18 @@ static MemTxResult watch_mem_write(void *opaque, hwaddr addr,
>                                     MemTxAttrs attrs)
>  {
>      MemTxResult res;
> +    AddressSpace *as = opaque;
>  
>      check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_WRITE);
>      switch (size) {
>      case 1:
> -        address_space_stb(&address_space_memory, addr, val, attrs, &res);
> +        address_space_stb(as, addr, val, attrs, &res);
>          break;
>      case 2:
> -        address_space_stw(&address_space_memory, addr, val, attrs, &res);
> +        address_space_stw(as, addr, val, attrs, &res);
>          break;
>      case 4:
> -        address_space_stl(&address_space_memory, addr, val, attrs, &res);
> +        address_space_stl(as, addr, val, attrs, &res);
>          break;
>      default: abort();
>      }
> @@ -2091,6 +2085,15 @@ static const MemoryRegionOps watch_mem_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> +bool memory_region_is_unassigned(MemoryRegion *mr)
> +{
> +    /* Checking the ops pointer of the MemoryRegion is strictly
> +     * speaking looking at private information of the MemoryRegion :-(
> +     */
> +    return mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
> +        && mr->ops != &watch_mem_ops;
> +}
> +
>  static MemTxResult subpage_read(void *opaque, hwaddr addr, uint64_t *data,
>                                  unsigned len, MemTxAttrs attrs)
>  {
> @@ -2251,8 +2254,6 @@ static void io_mem_init(void)
>                            NULL, UINT64_MAX);
>      memory_region_init_io(&io_mem_notdirty, NULL, &notdirty_mem_ops, NULL,
>                            NULL, UINT64_MAX);
> -    memory_region_init_io(&io_mem_watch, NULL, &watch_mem_ops, NULL,
> -                          NULL, UINT64_MAX);
>  }
>  
>  static void mem_begin(MemoryListener *listener)
> @@ -2267,7 +2268,7 @@ static void mem_begin(MemoryListener *listener)
>      assert(n == PHYS_SECTION_NOTDIRTY);
>      n = dummy_section(&d->map, as, &io_mem_rom);
>      assert(n == PHYS_SECTION_ROM);
> -    n = dummy_section(&d->map, as, &io_mem_watch);
> +    n = dummy_section(&d->map, as, as->io_mem_watch);
>      assert(n == PHYS_SECTION_WATCH);
>  
>      d->phys_map  = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .skip = 1 };
> @@ -2315,6 +2316,10 @@ static void tcg_commit(MemoryListener *listener)
>  
>  void address_space_init_dispatch(AddressSpace *as)
>  {
> +    as->io_mem_watch = g_new0(MemoryRegion, 1);
> +    memory_region_init_io(as->io_mem_watch, NULL, &watch_mem_ops, as,
> +                          NULL, UINT64_MAX);
> +
>      as->dispatch = NULL;
>      as->dispatch_listener = (MemoryListener) {
>          .begin = mem_begin,
> @@ -2339,6 +2344,7 @@ void address_space_destroy_dispatch(AddressSpace *as)
>      if (d) {
>          call_rcu(d, address_space_dispatch_free, rcu);
>      }
> +    memory_region_unref(as->io_mem_watch);
>  }
>  
>  static void memory_map_init(void)
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 0f07159..e5d98f8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -246,6 +246,8 @@ struct AddressSpace {
>      struct AddressSpaceDispatch *next_dispatch;
>      MemoryListener dispatch_listener;
>  
> +    MemoryRegion *io_mem_watch;
> +
>      QTAILQ_ENTRY(AddressSpace) address_spaces_link;
>  };
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 03/16] tlb_set_page_with_attrs: Take argument specifying AddressSpace to use

[Edgar E. Iglesias]

On Fri, Nov 06, 2015 at 01:41:54PM +0000, Peter Maydell wrote:
> On 6 November 2015 at 13:27, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Thu, Nov 05, 2015 at 06:15:45PM +0000, Peter Maydell wrote:
> >> Add an argument to tlb_set_page_with_attrs which allows the target CPU code
> >> to tell the core code which AddressSpace to use.
> >>
> >> The AddressSpace is specified by the index into the array of ASes which
> >> were registered with cpu_address_space_init().
> 
> >> --- a/exec.c
> >> +++ b/exec.c
> >> @@ -445,12 +445,13 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
> >>
> >>  /* Called from RCU critical section */
> >>  MemoryRegionSection *
> >> -address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr,
> >> +address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
> >>                                    hwaddr *xlat, hwaddr *plen)
> >
> > Does it make sense to replace the CPUState argument with an AddressSpace *
> > and have the callers do the cpu->cpu_ases[asidx]?
> > It would be more consistent and eventually maybe eliminate the need for
> > address_space_translate_for_iotlb in favor of calling address_space_translate
> > directly?
> 
> We can't accept an arbitrary AddressSpace, it has to be one which is
> embedded in a CPUAddressSpace and which we can thus find the
> memory_dispatch for. So you could pass a CPUAddressSpace*, but not
> an AddressSpace*. But to pass a CPUAddressSpace we would have to
> expose the currently-private-to-exec.c layout of the CPUAddressSpace
> struct. I chose not to do that (and you can see the results elsewhere
> in the patch series, like the function that's basically just "do
> the cs_ases array lookup for me"); there's an argument for making
> the structure more widely available to avoid some of that.

Aha, I see.. Thanks for clarifying.

Cheers,
Edgar

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

[Edgar E. Iglesias]

On Fri, Nov 06, 2015 at 01:34:44PM +0000, Peter Maydell wrote:
> On 6 November 2015 at 13:21, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > 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>
> 
> >> +    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.
> 
> Oops, yes, you're right.
> 
> Maybe we should just have the target CPU say in advance what the
> maximum number of AddressSpaces it will have is -- my expectation
> is that this will be (a) small (b) known in advance anyway.

Yes, that sounds good to me.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH 03/16] tlb_set_page_with_attrs: Take argument specifying AddressSpace to use

[Edgar E. Iglesias]

On Fri, Nov 06, 2015 at 02:49:05PM +0100, Edgar E. Iglesias wrote:
> On Fri, Nov 06, 2015 at 01:41:54PM +0000, Peter Maydell wrote:
> > On 6 November 2015 at 13:27, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > > On Thu, Nov 05, 2015 at 06:15:45PM +0000, Peter Maydell wrote:
> > >> Add an argument to tlb_set_page_with_attrs which allows the target CPU code
> > >> to tell the core code which AddressSpace to use.
> > >>
> > >> The AddressSpace is specified by the index into the array of ASes which
> > >> were registered with cpu_address_space_init().
> > 
> > >> --- a/exec.c
> > >> +++ b/exec.c
> > >> @@ -445,12 +445,13 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
> > >>
> > >>  /* Called from RCU critical section */
> > >>  MemoryRegionSection *
> > >> -address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr,
> > >> +address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
> > >>                                    hwaddr *xlat, hwaddr *plen)
> > >
> > > Does it make sense to replace the CPUState argument with an AddressSpace *
> > > and have the callers do the cpu->cpu_ases[asidx]?
> > > It would be more consistent and eventually maybe eliminate the need for
> > > address_space_translate_for_iotlb in favor of calling address_space_translate
> > > directly?
> > 
> > We can't accept an arbitrary AddressSpace, it has to be one which is
> > embedded in a CPUAddressSpace and which we can thus find the
> > memory_dispatch for. So you could pass a CPUAddressSpace*, but not
> > an AddressSpace*. But to pass a CPUAddressSpace we would have to
> > expose the currently-private-to-exec.c layout of the CPUAddressSpace
> > struct. I chose not to do that (and you can see the results elsewhere
> > in the patch series, like the function that's basically just "do
> > the cs_ases array lookup for me"); there's an argument for making
> > the structure more widely available to avoid some of that.
> 
> Aha, I see.. Thanks for clarifying.
>

That was the only comment I had so:

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
 

Re: [Qemu-devel] [PATCH 04/16] exec.c: Add address space index to CPUIOTLBEntry

[Edgar E. Iglesias]

On Fri, Nov 06, 2015 at 01:45:29PM +0000, Peter Maydell wrote:
> On 6 November 2015 at 13:34, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > On Thu, Nov 05, 2015 at 06:15:46PM +0000, Peter Maydell wrote:
> >> Add the address space index to CPUIOTLBEntry, and use this to pass it
> >> to iotlb_to_region(), so that we use the correct AddressSpace when
> >> doing IO path lookups.
> >
> > Hi Peter,
> >
> > I think this works but eventually when we add support for CPUs behind
> > IOMMUs I think things will be easier if we store a pointer to the
> > AS instead of an index in the IOTLB. address_space_translate_for_iotlb()
> > with IOMMU support may be implemented so that it returns a different
> > address space as it walks tha memory hierarchy and translate things.
> >
> > Do you see any issues with storing pointers to the AS instead of an
> > index?
> 
> Yeah, we can't deal with arbitrary AddressSpaces here. We need a
> CPUAddressSpace, because we use the cached RCU-protected memory_dispatch
> pointer. Once you know that you have to be dealing with one of the fixed
> CPUAddressSpaces, then you might as well just keep the index rather than
> an arbitrary pointer.

OK, I think we'll have to have another look once we do the IOMMU stuff. My
view has probably been to simplistic.

Best regards,
Edgar

Re: [Qemu-devel] [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:51PM +0000, Peter Maydell wrote:
> If we have a secure address space, use it in page table walks:
>  * when doing the physical accesses to read descriptors,
>    make them through the correct address space
>  * when the final result indicates a secure access, pass the
>    correct address space index to tlb_set_page_with_attrs()
> 
> (The descriptor reads are the only direct physical accesses
> made in target-arm/ for CPUs which might have TrustZone.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Nice to see how this falls into place like this :-)

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> ---
>  target-arm/cpu.h    | 29 +++++++++++++++++++++++++++++
>  target-arm/helper.c | 10 +++++++---
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 815fef8..8dbf4d4 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1720,6 +1720,12 @@ static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)
>      return el;
>  }
>  
> +/* Indexes used when registering address spaces with cpu_address_space_init */
> +typedef enum ARMASIdx {
> +    ARMASIdx_NS = 0,
> +    ARMASIdx_S = 1,
> +} ARMASIdx;
> +
>  /* Return the Exception Level targeted by debug exceptions;
>   * currently always EL1 since we don't implement EL2 or EL3.
>   */
> @@ -1991,4 +1997,27 @@ enum {
>      QEMU_PSCI_CONDUIT_HVC = 2,
>  };
>  
> +#ifndef CONFIG_USER_ONLY
> +/* Return the address space index to use for a memory access
> + * (which depends on whether the access is S or NS, and whether
> + * the board gave us a separate AddressSpace for S accesses).
> + */
> +static inline int arm_asidx(CPUState *cs, bool is_secure)
> +{
> +    if (is_secure && cs->num_ases > 1) {
> +        return ARMASIdx_S;
> +    }
> +    return ARMASIdx_NS;
> +}
> +
> +/* Return the AddressSpace to use for a memory access
> + * (which depends on whether the access is S or NS, and whether
> + * the board gave us a separate AddressSpace for S accesses).
> + */
> +static inline AddressSpace *arm_addressspace(CPUState *cs, bool is_secure)
> +{
> +    return cpu_get_address_space(cs, arm_asidx(cs, is_secure));
> +}
> +#endif
> +
>  #endif
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 174371b..242928d 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6260,13 +6260,14 @@ static uint32_t arm_ldl_ptw(CPUState *cs, hwaddr addr, bool is_secure,
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>      MemTxAttrs attrs = {};
> +    AddressSpace *as = arm_addressspace(cs, is_secure);
>  
>      attrs.secure = is_secure;
>      addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
>      if (fi->s1ptw) {
>          return 0;
>      }
> -    return address_space_ldl(cs->as, addr, attrs, NULL);
> +    return address_space_ldl(as, addr, attrs, NULL);
>  }
>  
>  static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
> @@ -6276,13 +6277,14 @@ static uint64_t arm_ldq_ptw(CPUState *cs, hwaddr addr, bool is_secure,
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
>      MemTxAttrs attrs = {};
> +    AddressSpace *as = arm_addressspace(cs, is_secure);
>  
>      attrs.secure = is_secure;
>      addr = S1_ptw_translate(env, mmu_idx, addr, attrs, fsr, fi);
>      if (fi->s1ptw) {
>          return 0;
>      }
> -    return address_space_ldq(cs->as, addr, attrs, NULL);
> +    return address_space_ldq(as, addr, attrs, NULL);
>  }
>  
>  static bool get_phys_addr_v5(CPUARMState *env, uint32_t address,
> @@ -7307,6 +7309,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>      target_ulong page_size;
>      int prot;
>      int ret;
> +    int asidx;
>      MemTxAttrs attrs = {};
>  
>      ret = get_phys_addr(env, address, access_type, mmu_idx, &phys_addr,
> @@ -7315,7 +7318,8 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>          /* Map a single [sub]page.  */
>          phys_addr &= TARGET_PAGE_MASK;
>          address &= TARGET_PAGE_MASK;
> -        tlb_set_page_with_attrs(cs, address, 0, phys_addr, attrs,
> +        asidx = arm_asidx(cs, attrs.secure);
> +        tlb_set_page_with_attrs(cs, address, asidx, phys_addr, attrs,
>                                  prot, mmu_idx, page_size);
>          return 0;
>      }
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 10/16] target-arm: Implement cpu_get_phys_page_asidx_debug

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:52PM +0000, Peter Maydell wrote:
> Implement cpu_get_phys_page_asidx_debug instead of cpu_get_phys_page_debug.


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/cpu-qom.h | 2 +-
>  target-arm/cpu.c     | 2 +-
>  target-arm/helper.c  | 3 ++-
>  3 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index 25fb1ce..53d3129 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -216,7 +216,7 @@ bool arm_cpu_exec_interrupt(CPUState *cpu, int int_req);
>  void arm_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
>                          int flags);
>  
> -hwaddr arm_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> +hwaddr arm_cpu_get_phys_page_asidx_debug(CPUState *cpu, vaddr addr, int *asidx);
>  
>  int arm_cpu_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
>  int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 30739fc..7789d50 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -1417,7 +1417,7 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>      cc->handle_mmu_fault = arm_cpu_handle_mmu_fault;
>  #else
>      cc->do_interrupt = arm_cpu_do_interrupt;
> -    cc->get_phys_page_debug = arm_cpu_get_phys_page_debug;
> +    cc->get_phys_page_asidx_debug = arm_cpu_get_phys_page_asidx_debug;
>      cc->vmsd = &vmstate_arm_cpu;
>      cc->virtio_is_big_endian = arm_cpu_is_big_endian;
>  #endif
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 242928d..db7e81a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7327,7 +7327,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>      return ret;
>  }
>  
> -hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
> +hwaddr arm_cpu_get_phys_page_asidx_debug(CPUState *cs, vaddr addr, int *asidx)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> @@ -7346,6 +7346,7 @@ hwaddr arm_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
>          return -1;
>      }
>  
> +    *asidx = arm_asidx(cs, attrs.secure);
>      return phys_addr;
>  }
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 11/16] memory: Add address_space_init_shareable()

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:53PM +0000, Peter Maydell wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> This will either create a new AS or return a pointer to an
> already existing equivalent one, if we have already created
> an AS for the specified root memory region.
> 
> The motivation is to reuse address spaces as much as possible.
> It's going to be quite common that bus masters out in device land
> have pointers to the same memory region for their mastering yet
> each will need to create its own address space. Let the memory
> API implement sharing for them.
> 
> Aside from the perf optimisations, this should reduce the amount
> of redundant output on info mtree as well.
> 
> Thee returned value will be malloced, but the malloc will be
> automatically freed when the AS runs out of refs.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> [PMM: dropped check for NULL root as unused; added doc-comment;
>  squashed Peter C's reference-counting patch into this one;
>  don't compare name string when deciding if we can share ASes;
>  read as->malloced before the unref of as->root to avoid possible
>  read-after-free if as->root was the owner of as]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/memory.h | 18 ++++++++++++++++++
>  memory.c              | 27 +++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e5d98f8..1d1740a 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -236,6 +236,8 @@ struct AddressSpace {
>      struct rcu_head rcu;
>      char *name;
>      MemoryRegion *root;
> +    int ref_count;
> +    bool malloced;
>  
>      /* Accessed via RCU.  */
>      struct FlatView *current_map;
> @@ -1167,6 +1169,22 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
>   */
>  void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name);
>  
> +/**
> + * address_space_init_shareable: return an address space for a memory region,
> + *                               creating it if it does not already exist
> + *
> + * @root: a #MemoryRegion that routes addresses for the address space
> + * @name: an address space name.  The name is only used for debugging
> + *        output.
> + *
> + * This function will return a pointer to an existing AddressSpace
> + * which was initialized with the specified MemoryRegion, or it will
> + * create and initialize one if it does not already exist. The ASes
> + * are reference-counted, so the memory will be freed automatically
> + * when the AddressSpace is destroyed via address_space_destroy.
> + */
> +AddressSpace *address_space_init_shareable(MemoryRegion *root,
> +                                           const char *name);
>  
>  /**
>   * address_space_destroy: destroy an address space
> diff --git a/memory.c b/memory.c
> index c435c88..6c98a71 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2101,7 +2101,9 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>  {
>      memory_region_ref(root);
>      memory_region_transaction_begin();
> +    as->ref_count = 1;
>      as->root = root;
> +    as->malloced = false;
>      as->current_map = g_new(FlatView, 1);
>      flatview_init(as->current_map);
>      as->ioeventfd_nb = 0;
> @@ -2116,6 +2118,7 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
>  static void do_address_space_destroy(AddressSpace *as)
>  {
>      MemoryListener *listener;
> +    bool do_free = as->malloced;
>  
>      address_space_destroy_dispatch(as);
>  
> @@ -2127,12 +2130,36 @@ static void do_address_space_destroy(AddressSpace *as)
>      g_free(as->name);
>      g_free(as->ioeventfds);
>      memory_region_unref(as->root);
> +    if (do_free) {
> +        g_free(as);
> +    }
> +}
> +
> +AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)
> +{
> +    AddressSpace *as;
> +
> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +        if (root == as->root) {
> +            as->ref_count++;
> +            return as;
> +        }
> +    }
> +
> +    as = g_malloc0(sizeof *as);
> +    address_space_init(as, root, name);

Nit-pick but IIUC, address_space_init does not need AS to be zeroed
so this could be changed to a g_malloc().

either-way:

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> +    as->malloced = true;
> +    return as;
>  }
>  
>  void address_space_destroy(AddressSpace *as)
>  {
>      MemoryRegion *root = as->root;
>  
> +    as->ref_count--;
> +    if (as->ref_count) {
> +        return;
> +    }
>      /* Flush out anything from MemoryListeners listening in on this */
>      memory_region_transaction_begin();
>      as->root = NULL;
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 12/16] qom/cpu: Add MemoryRegion property

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:54PM +0000, Peter Maydell wrote:
> From: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Add a MemoryRegion property, which if set is used to construct
> the CPU's initial (default) AddressSpace.
> 
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> [PMM: code is moved from qom/cpu.c to exec.c to avoid having to
>  make qom/cpu.o be a non-common object file; code to use the
>  MemoryRegion and to default it to system_memory added.]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> ---
>  cpus.c            |  4 +++-
>  exec.c            | 13 +++++++++++++
>  include/qom/cpu.h |  1 +
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 764ea75..b7c19e0 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1356,7 +1356,9 @@ void qemu_init_vcpu(CPUState *cpu)
>          /* If the target cpu hasn't set up any address spaces itself,
>           * give it the default one.
>           */
> -        cpu_address_space_init(cpu, &address_space_memory, 0);
> +        AddressSpace *as = address_space_init_shareable(cpu->memory,
> +                                                        "cpu-memory");
> +        cpu_address_space_init(cpu, as, 0);
>      }
>  
>      if (kvm_enabled()) {
> diff --git a/exec.c b/exec.c
> index 9998fa0..21b1b57 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -637,6 +637,19 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>  
>  #ifndef CONFIG_USER_ONLY
>      cpu->thread_id = qemu_get_thread_id();
> +
> +    /* This is a softmmu CPU object, so create a property for it
> +     * so users can wire up its memory. (This can't go in qom/cpu.c
> +     * because that file is compiled only once for both user-mode
> +     * and system builds.) The default if no link is set up is to use
> +     * the system address space.
> +     */
> +    object_property_add_link(OBJECT(cpu), "memory", TYPE_MEMORY_REGION,
> +                             (Object **)&cpu->memory,
> +                             qdev_prop_allow_set_link_before_realize,
> +                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
> +                             &error_abort);
> +    cpu->memory = system_memory;
>  #endif
>  
>  #if defined(CONFIG_USER_ONLY)
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 10ef5cc..39a9a24 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -292,6 +292,7 @@ struct CPUState {
>      CPUAddressSpace *cpu_ases;
>      int num_ases;
>      AddressSpace *as;
> +    MemoryRegion *memory;
>  
>      void *env_ptr; /* CPUArchState */
>      struct TranslationBlock *current_tb;
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 13/16] target-arm: Add QOM property for Secure memory region

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:55PM +0000, Peter Maydell wrote:
> Add QOM property to the ARM CPU which boards can use to tell us what
> memory region to use for secure accesses. Nonsecure accesses
> go via the memory region specified with the base CPU class 'memory'
> property.
> 
> By default, if no secure region is specified it is the same as the
> nonsecure region, and if no nonsecure region is specified we will use
> address_space_memory.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> ---
>  target-arm/cpu-qom.h |  3 +++
>  target-arm/cpu.c     | 26 ++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index 53d3129..75dc74e 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -87,6 +87,9 @@ typedef struct ARMCPU {
>      /* GPIO outputs for generic timer */
>      qemu_irq gt_timer_outputs[NUM_GTIMERS];
>  
> +    /* MemoryRegion to use for secure physical accesses */
> +    MemoryRegion *secure_memory;
> +
>      /* 'compatible' string for this CPU for Linux device trees */
>      const char *dtb_compatible;
>  
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 7789d50..85e2bc8 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -542,6 +542,15 @@ static void arm_cpu_post_init(Object *obj)
>           */
>          qdev_property_add_static(DEVICE(obj), &arm_cpu_has_el3_property,
>                                   &error_abort);
> +
> +#ifndef CONFIG_USER_ONLY
> +        object_property_add_link(obj, "secure-memory",
> +                                 TYPE_MEMORY_REGION,
> +                                 (Object **)&cpu->secure_memory,
> +                                 qdev_prop_allow_set_link_before_realize,
> +                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
> +                                 &error_abort);
> +#endif
>      }
>  
>      if (arm_feature(&cpu->env, ARM_FEATURE_MPU)) {
> @@ -665,6 +674,23 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>  
>      init_cpreg_list(cpu);
>  
> +#ifndef CONFIG_USER_ONLY
> +    if (cpu->has_el3) {
> +        AddressSpace *as;
> +
> +        if (!cpu->secure_memory) {
> +            cpu->secure_memory = cs->memory;
> +        }
> +        as = address_space_init_shareable(cpu->secure_memory,
> +                                          "cpu-secure-memory");
> +        cpu_address_space_init(cs, as, ARMASIdx_S);
> +    }
> +    cpu_address_space_init(cs,
> +                           address_space_init_shareable(cs->memory,
> +                                                        "cpu-memory"),
> +                           ARMASIdx_NS);
> +#endif
> +
>      qemu_init_vcpu(cs);
>      cpu_reset(cs);
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 14/16] hw/arm/virt: Wire up memory region to CPUs explicitly

[Edgar E. Iglesias]

On Thu, Nov 05, 2015 at 06:15:56PM +0000, Peter Maydell wrote:
> Wire up the system memory region to the CPUs explicitly
> by setting the QOM property. This doesn't change anything
> over letting it default, but will be needed for adding
> a secure memory region later.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I'm not sure I understand this, if not set, wouldn't "memory"
just default to sysmem anyway regardless of if we set
"secure-memory" or not? I'm probably missing something
in the init/setup sequence...

Anyway, I don't mind explicitely setting "memory":
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/arm/virt.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 77d9267..3ab31e0 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1020,6 +1020,9 @@ static void machvirt_init(MachineState *machine)
>                                      "reset-cbar", &error_abort);
>          }
>  
> +        object_property_set_link(cpuobj, OBJECT(sysmem), "memory",
> +                                 &error_abort);
> +
>          object_property_set_bool(cpuobj, true, "realized", NULL);
>      }
>      g_strfreev(cpustr);
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH 11/16] memory: Add address_space_init_shareable()

[Peter Maydell]

On 6 November 2015 at 14:29, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Thu, Nov 05, 2015 at 06:15:53PM +0000, Peter Maydell wrote:

>> +AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)
>> +{
>> +    AddressSpace *as;
>> +
>> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>> +        if (root == as->root) {
>> +            as->ref_count++;
>> +            return as;
>> +        }
>> +    }
>> +
>> +    as = g_malloc0(sizeof *as);
>> +    address_space_init(as, root, name);
>
> Nit-pick but IIUC, address_space_init does not need AS to be zeroed
> so this could be changed to a g_malloc().

I tend to prefer to be conservative about these things,
so I use zero-allocation unless I'm really sure it's
unnecessary.

> either-way:
>
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 14/16] hw/arm/virt: Wire up memory region to CPUs explicitly

[Peter Maydell]

On 6 November 2015 at 14:45, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Thu, Nov 05, 2015 at 06:15:56PM +0000, Peter Maydell wrote:
>> Wire up the system memory region to the CPUs explicitly
>> by setting the QOM property. This doesn't change anything
>> over letting it default, but will be needed for adding
>> a secure memory region later.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> I'm not sure I understand this, if not set, wouldn't "memory"
> just default to sysmem anyway regardless of if we set
> "secure-memory" or not? I'm probably missing something
> in the init/setup sequence...
>
> Anyway, I don't mind explicitely setting "memory":
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Yes, you're right, it would default to the same thing;
but it seemed clearer to me if the board is going to be
setting memory regions explicitly on the CPU to set them
all rather than letting one default and one not.
(Also it provides an example in the code base of how to
do it right ;-))

thanks
-- PMM

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

[Paolo Bonzini]

On 05/11/2015 19:15, 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.

Actually it won't work _now_ with KVM, but it could.

It would be a good idea to map the multiple CPU AddressSpaces to KVM's
own multiple address spaces.  It's possible to modify i386 to do this,
using address space 0 for normal operation and address space 1 for SMM,
just like KVM.

More on this as I reply to the remainder of the series...

Paolo

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

[Paolo Bonzini]

On 06/11/2015 14:34, Peter Maydell wrote:
>> > 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.
> Oops, yes, you're right.
> 
> Maybe we should just have the target CPU say in advance what the
> maximum number of AddressSpaces it will have is -- my expectation
> is that this will be (a) small (b) known in advance anyway.

I agree.  Or even just allocate room statically, for the largest amount
that all targets in QEMU use.  My expectation is that this will be 2. :)

Paolo

Re: [Qemu-devel] [PATCH 03/16] tlb_set_page_with_attrs: Take argument specifying AddressSpace to use

[Paolo Bonzini]

On 05/11/2015 19:15, Peter Maydell wrote:
> Add an argument to tlb_set_page_with_attrs which allows the target CPU code
> to tell the core code which AddressSpace to use.
> 
> The AddressSpace is specified by the index into the array of ASes which
> were registered with cpu_address_space_init().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Can it be deduced from the attributes instead?  Basically, you would have

   int cpu_get_asidx(MemTxAttrs attrs);

in cpu.h, which is called by tlb_set_page_with_attrs.
cpu_get_phys_page_asidx_debug could also be replaced by
cpu_get_phys_page_attrs_debug.

Paolo

> ---
>  cputlb.c                |  6 +++---
>  exec.c                  |  7 ++++---
>  include/exec/exec-all.h | 33 ++++++++++++++++++++++++++++++---
>  target-arm/helper.c     |  2 +-
>  target-i386/helper.c    |  2 +-
>  5 files changed, 39 insertions(+), 11 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index bf1d50a..e753083 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -343,7 +343,7 @@ static void tlb_add_large_page(CPUArchState *env, target_ulong vaddr,
>   * Called from TCG-generated code, which is under an RCU read-side
>   * critical section.
>   */
> -void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
> +void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, int asidx,
>                               hwaddr paddr, MemTxAttrs attrs, int prot,
>                               int mmu_idx, target_ulong size)
>  {
> @@ -363,7 +363,7 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
>      }
>  
>      sz = size;
> -    section = address_space_translate_for_iotlb(cpu, paddr, &xlat, &sz);
> +    section = address_space_translate_for_iotlb(cpu, asidx, paddr, &xlat, &sz);
>      assert(sz >= TARGET_PAGE_SIZE);
>  
>  #if defined(DEBUG_TLB)
> @@ -433,7 +433,7 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                    hwaddr paddr, int prot,
>                    int mmu_idx, target_ulong size)
>  {
> -    tlb_set_page_with_attrs(cpu, vaddr, paddr, MEMTXATTRS_UNSPECIFIED,
> +    tlb_set_page_with_attrs(cpu, vaddr, 0, paddr, MEMTXATTRS_UNSPECIFIED,
>                              prot, mmu_idx, size);
>  }
>  
> diff --git a/exec.c b/exec.c
> index 6a2a694..92e76fa 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -445,12 +445,13 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
>  
>  /* Called from RCU critical section */
>  MemoryRegionSection *
> -address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr,
> +address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
>                                    hwaddr *xlat, hwaddr *plen)
>  {
>      MemoryRegionSection *section;
> -    section = address_space_translate_internal(cpu->cpu_ases[0].memory_dispatch,
> -                                               addr, xlat, plen, false);
> +    AddressSpaceDispatch *d = cpu->cpu_ases[asidx].memory_dispatch;
> +
> +    section = address_space_translate_internal(d, addr, xlat, plen, false);
>  
>      assert(!section->mr->iommu_ops);
>      return section;
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 90130ca..472d0fc 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -144,7 +144,34 @@ void tlb_flush_by_mmuidx(CPUState *cpu, ...);
>  void tlb_set_page(CPUState *cpu, target_ulong vaddr,
>                    hwaddr paddr, int prot,
>                    int mmu_idx, target_ulong size);
> -void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
> +/**
> + * tlb_set_page_with_attrs:
> + * @cpu: CPU to add this TLB entry for
> + * @vaddr: virtual address of page to add entry for
> + * @asidx: index of the AddressSpace the physical address is in
> + * @paddr: physical address of the page
> + * @attrs: memory transaction attributes
> + * @prot: access permissions (PAGE_READ/PAGE_WRITE/PAGE_EXEC bits)
> + * @mmu_idx: MMU index to insert TLB entry for
> + * @size: size of the page in bytes
> + *
> + * Add an entry to this CPU's TLB (a mapping from virtual address
> + * @vaddr to physical address @paddr) with the specified memory
> + * transaction attributes. This is generally called by the target CPU
> + * specific code after it has been called through the tlb_fill()
> + * entry point and performed a successful page table walk to find
> + * the physical address and attributes for the virtual address
> + * which provoked the TLB miss.
> + *
> + * At most one entry for a given virtual address is permitted. Only a
> + * single TARGET_PAGE_SIZE region is mapped; the supplied @size is only
> + * used by tlb_flush_page.
> + *
> + * @asidx is the index of the AddressSpace in the cpu->ases array;
> + * if the CPU does not support multiple AddressSpaces then it will
> + * always be zero.
> + */
> +void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr, int asidx,
>                               hwaddr paddr, MemTxAttrs attrs,
>                               int prot, int mmu_idx, target_ulong size);
>  void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr);
> @@ -400,8 +427,8 @@ void tlb_set_dirty(CPUState *cpu, target_ulong vaddr);
>  void tb_flush_jmp_cache(CPUState *cpu, target_ulong addr);
>  
>  MemoryRegionSection *
> -address_space_translate_for_iotlb(CPUState *cpu, hwaddr addr, hwaddr *xlat,
> -                                  hwaddr *plen);
> +address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
> +                                  hwaddr *xlat, hwaddr *plen);
>  hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>                                         MemoryRegionSection *section,
>                                         target_ulong vaddr,
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 4ecae61..174371b 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -7315,7 +7315,7 @@ bool arm_tlb_fill(CPUState *cs, vaddr address,
>          /* Map a single [sub]page.  */
>          phys_addr &= TARGET_PAGE_MASK;
>          address &= TARGET_PAGE_MASK;
> -        tlb_set_page_with_attrs(cs, address, phys_addr, attrs,
> +        tlb_set_page_with_attrs(cs, address, 0, phys_addr, attrs,
>                                  prot, mmu_idx, page_size);
>          return 0;
>      }
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index d18be95..1c43717 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -962,7 +962,7 @@ do_check_protect_pse36:
>      page_offset = vaddr & (page_size - 1);
>      paddr = pte + page_offset;
>  
> -    tlb_set_page_with_attrs(cs, vaddr, paddr, cpu_get_mem_attrs(env),
> +    tlb_set_page_with_attrs(cs, vaddr, 0, paddr, cpu_get_mem_attrs(env),
>                              prot, mmu_idx, page_size);
>      return 0;
>   do_fault_rsvd:
> 

Re: [Qemu-devel] [PATCH 08/16] exec.c: Have one io_mem_watch per AddressSpace

[Paolo Bonzini]

On 05/11/2015 19:15, Peter Maydell wrote:
> The io_mem_watch MemoryRegion's read and write callbacks pass the
> accesses through to an underlying address space. Now that that
> might be something other than address_space_memory, we need to
> pass the correct AddressSpace in via the opaque pointer. That
> means we need to have one io_mem_watch MemoryRegion per address
> space, rather than sharing one between all address spaces.
> 
> This should also fix gdbstub watchpoints in x86 SMRAM, which would
> previously not have worked correctly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  exec.c                | 40 +++++++++++++++++++++++-----------------
>  include/exec/memory.h |  2 ++
>  2 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index bc6ab8a..9998fa0 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -163,8 +163,6 @@ static void io_mem_init(void);
>  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
> @@ -334,12 +332,6 @@ static MemoryRegionSection *phys_page_find(PhysPageEntry lp, hwaddr addr,
>      }
>  }
>  
> -bool memory_region_is_unassigned(MemoryRegion *mr)
> -{
> -    return mr != &io_mem_rom && mr != &io_mem_notdirty && !mr->rom_device
> -        && mr != &io_mem_watch;
> -}
> -
>  /* Called from RCU critical section */
>  static MemoryRegionSection *address_space_lookup_region(AddressSpaceDispatch *d,
>                                                          hwaddr addr,
> @@ -2045,17 +2037,18 @@ static MemTxResult watch_mem_read(void *opaque, hwaddr addr, uint64_t *pdata,
>  {
>      MemTxResult res;
>      uint64_t data;
> +    AddressSpace *as = opaque;
>  
>      check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_READ);
>      switch (size) {
>      case 1:
> -        data = address_space_ldub(&address_space_memory, addr, attrs, &res);
> +        data = address_space_ldub(as, addr, attrs, &res);
>          break;
>      case 2:
> -        data = address_space_lduw(&address_space_memory, addr, attrs, &res);
> +        data = address_space_lduw(as, addr, attrs, &res);
>          break;
>      case 4:
> -        data = address_space_ldl(&address_space_memory, addr, attrs, &res);
> +        data = address_space_ldl(as, addr, attrs, &res);
>          break;
>      default: abort();
>      }

current_cpu is available here, so it should be possible to have only one
io_mem_watch per CPU address space index (i.e. two).

But actually, if the address space can be derived from the attributes,
you just need to fish the right address space out of current_cpu.

> +    as->io_mem_watch = g_new0(MemoryRegion, 1);

This is leaked when the address space goes away.  You can allocate it
statically inside AddressSpace if my alternative plan from above doesn't
work out.

> +    memory_region_init_io(as->io_mem_watch, NULL, &watch_mem_ops, as,
> +                          NULL, UINT64_MAX);
> +
>      as->dispatch = NULL;
>      as->dispatch_listener = (MemoryListener) {
>          .begin = mem_begin,
> @@ -2339,6 +2344,7 @@ void address_space_destroy_dispatch(AddressSpace *as)
>      if (d) {
>          call_rcu(d, address_space_dispatch_free, rcu);
>      }
> +    memory_region_unref(as->io_mem_watch);

Re: [Qemu-devel] [PATCH 03/16] tlb_set_page_with_attrs: Take argument specifying AddressSpace to use

[Peter Maydell]

On 9 November 2015 at 10:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 05/11/2015 19:15, Peter Maydell wrote:
>> Add an argument to tlb_set_page_with_attrs which allows the target CPU code
>> to tell the core code which AddressSpace to use.
>>
>> The AddressSpace is specified by the index into the array of ASes which
>> were registered with cpu_address_space_init().
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Can it be deduced from the attributes instead?  Basically, you would have
>
>    int cpu_get_asidx(MemTxAttrs attrs);
>
> in cpu.h, which is called by tlb_set_page_with_attrs.
> cpu_get_phys_page_asidx_debug could also be replaced by
> cpu_get_phys_page_attrs_debug.

For ARM it could, certainly (and as you say, if we go that
way then some of the extra passing around of asidxes collapses,
and in particular we don't need to keep them separately in the
iotlb entries). I wasn't convinced that this would be true for
all possible uses of AddressSpaces.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

[Paolo Bonzini]

On 05/11/2015 19:15, Peter Maydell wrote:
> If we have a secure address space, use it in page table walks:
>  * when doing the physical accesses to read descriptors,
>    make them through the correct address space
>  * when the final result indicates a secure access, pass the
>    correct address space index to tlb_set_page_with_attrs()
> 
> (The descriptor reads are the only direct physical accesses
> made in target-arm/ for CPUs which might have TrustZone.)

What is the case where you have no secure address space and you have
TrustZone?  KVM doesn't have TrustZone, so it should never be in a
secure regime, should it?

Paolo

Re: [Qemu-devel] [PATCH 08/16] exec.c: Have one io_mem_watch per AddressSpace

[Peter Maydell]

On 9 November 2015 at 10:49, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 05/11/2015 19:15, Peter Maydell wrote:
>> @@ -2045,17 +2037,18 @@ static MemTxResult watch_mem_read(void *opaque, hwaddr addr, uint64_t *pdata,
>>  {
>>      MemTxResult res;
>>      uint64_t data;
>> +    AddressSpace *as = opaque;
>>
>>      check_watchpoint(addr & ~TARGET_PAGE_MASK, size, attrs, BP_MEM_READ);
>>      switch (size) {
>>      case 1:
>> -        data = address_space_ldub(&address_space_memory, addr, attrs, &res);
>> +        data = address_space_ldub(as, addr, attrs, &res);
>>          break;
>>      case 2:
>> -        data = address_space_lduw(&address_space_memory, addr, attrs, &res);
>> +        data = address_space_lduw(as, addr, attrs, &res);
>>          break;
>>      case 4:
>> -        data = address_space_ldl(&address_space_memory, addr, attrs, &res);
>> +        data = address_space_ldl(as, addr, attrs, &res);
>>          break;
>>      default: abort();
>>      }
>
> current_cpu is available here, so it should be possible to have only one
> io_mem_watch per CPU address space index (i.e. two).

So the opaque gives you the asidx and then you look at current_cpu's
cpu_ases[asidx]? Yeah, that works and is simpler. (Good argument for
making "number of ASes per CPU" a compile-time constant I guess.)

> But actually, if the address space can be derived from the attributes,
> you just need to fish the right address space out of current_cpu.
>
>> +    as->io_mem_watch = g_new0(MemoryRegion, 1);
>
> This is leaked when the address space goes away.  You can allocate it
> statically inside AddressSpace if my alternative plan from above doesn't
> work out.

Oh, right, it's not sufficient to unref it, it doesn't auto-free
itself when the refcount drops to 0.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 11/16] memory: Add address_space_init_shareable()

[Paolo Bonzini]

On 05/11/2015 19:15, Peter Maydell wrote:
> +AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)
> +{
> +    AddressSpace *as;
> +
> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> +        if (root == as->root) {
> +            as->ref_count++;
> +            return as;
> +        }
> +    }
> +
> +    as = g_malloc0(sizeof *as);
> +    address_space_init(as, root, name);
> +    as->malloced = true;
> +    return as;
>  }

You shouldn't return a non-shareable address space here, I think,
because it might be contained into another object and that object might
disappear.  I haven't thought this through very much, but adding an " &&
as->malloced" to the conditional seems easy and safe.

Paolo

>  
>  void address_space_destroy(AddressSpace *as)
>  {
>      MemoryRegion *root = as->root;
>  
> +    as->ref_count--;
> +    if (as->ref_count) {
> +        return;
> +    }
>      /* Flush out anything from MemoryListeners listening in on this */
>      memory_region_transaction_begin();
>      as->root = NULL;
> 

Re: [Qemu-devel] [PATCH 12/16] qom/cpu: Add MemoryRegion property

[Paolo Bonzini]

On 05/11/2015 19:15, Peter Maydell wrote:
> +
> +    /* This is a softmmu CPU object, so create a property for it
> +     * so users can wire up its memory. (This can't go in qom/cpu.c
> +     * because that file is compiled only once for both user-mode
> +     * and system builds.) The default if no link is set up is to use
> +     * the system address space.
> +     */
> +    object_property_add_link(OBJECT(cpu), "memory", TYPE_MEMORY_REGION,
> +                             (Object **)&cpu->memory,
> +                             qdev_prop_allow_set_link_before_realize,
> +                             OBJ_PROP_LINK_UNREF_ON_RELEASE,
> +                             &error_abort);
> +    cpu->memory = system_memory;

You need object_ref(cpu->memory) here, because setting cpu->memory will
drop a reference from the previously-set value.

Paolo

>  #endif

Re: [Qemu-devel] [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

[Peter Maydell]

On 9 November 2015 at 10:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 05/11/2015 19:15, Peter Maydell wrote:
>> If we have a secure address space, use it in page table walks:
>>  * when doing the physical accesses to read descriptors,
>>    make them through the correct address space
>>  * when the final result indicates a secure access, pass the
>>    correct address space index to tlb_set_page_with_attrs()
>>
>> (The descriptor reads are the only direct physical accesses
>> made in target-arm/ for CPUs which might have TrustZone.)
>
> What is the case where you have no secure address space and you have
> TrustZone?  KVM doesn't have TrustZone, so it should never be in a
> secure regime, should it?

You mean "what is the case where is_secure but cpu->num_ases == 1" ?
That happens if you have a TrustZone CPU but the board has only
connected up one address space, because there is no difference
in the view from Secure and NonSecure. (vexpress is like this
in hardware, and most of our board models for TZ CPUS are like
that now even if the real h/w makes a distinction.)

I could have handled that by making the CPU init code always
register two ASes (using the same one twice if the board code
only passes one or using system_address_space twice if the
board code doesn't pass one at all), but that seemed a bit wasteful.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 11/16] memory: Add address_space_init_shareable()

[Peter Maydell]

On 9 November 2015 at 10:55, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 05/11/2015 19:15, Peter Maydell wrote:
>> +AddressSpace *address_space_init_shareable(MemoryRegion *root, const char *name)
>> +{
>> +    AddressSpace *as;
>> +
>> +    QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>> +        if (root == as->root) {
>> +            as->ref_count++;
>> +            return as;
>> +        }
>> +    }
>> +
>> +    as = g_malloc0(sizeof *as);
>> +    address_space_init(as, root, name);
>> +    as->malloced = true;
>> +    return as;
>>  }
>
> You shouldn't return a non-shareable address space here, I think,
> because it might be contained into another object and that object might
> disappear.  I haven't thought this through very much, but adding an " &&
> as->malloced" to the conditional seems easy and safe.

That would prevent sharing the system address space, which is one
you really commonly want to share. I guess we could make that one
be alloced-shareable.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/16] exec.c: Have one io_mem_watch per AddressSpace

[Paolo Bonzini]

On 09/11/2015 11:54, Peter Maydell wrote:
> > current_cpu is available here, so it should be possible to have only one
> > io_mem_watch per CPU address space index (i.e. two).
> 
> So the opaque gives you the asidx and then you look at current_cpu's
> cpu_ases[asidx]? Yeah, that works and is simpler. (Good argument for
> making "number of ASes per CPU" a compile-time constant I guess.)

Yes.  Of course it works well only if the compile-time constant is
small---but again, my guess is that it'll be two.

It would be even better if you can use the attrs instead of the opaque
to compute the asidx.  Then the only change you need is to watch_mem_*.

Paolo

Re: [Qemu-devel] [PATCH 11/16] memory: Add address_space_init_shareable()

[Paolo Bonzini]

On 09/11/2015 11:59, Peter Maydell wrote:
>>> >> +    as = g_malloc0(sizeof *as);
>>> >> +    address_space_init(as, root, name);
>>> >> +    as->malloced = true;
>>> >> +    return as;
>>> >>  }
>> >
>> > You shouldn't return a non-shareable address space here, I think,
>> > because it might be contained into another object and that object might
>> > disappear.  I haven't thought this through very much, but adding an " &&
>> > as->malloced" to the conditional seems easy and safe.
> That would prevent sharing the system address space, which is one
> you really commonly want to share. I guess we could make that one
> be alloced-shareable.

You would only allocate one duplicate of the system address space
though.  The second call to address_space_init_shareable would not
create a new AS.

Paolo

Re: [Qemu-devel] [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

[Paolo Bonzini]

On 09/11/2015 11:58, Peter Maydell wrote:
> On 9 November 2015 at 10:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 05/11/2015 19:15, Peter Maydell wrote:
>>> If we have a secure address space, use it in page table walks:
>>>  * when doing the physical accesses to read descriptors,
>>>    make them through the correct address space
>>>  * when the final result indicates a secure access, pass the
>>>    correct address space index to tlb_set_page_with_attrs()
>>>
>>> (The descriptor reads are the only direct physical accesses
>>> made in target-arm/ for CPUs which might have TrustZone.)
>>
>> What is the case where you have no secure address space and you have
>> TrustZone?  KVM doesn't have TrustZone, so it should never be in a
>> secure regime, should it?
> 
> You mean "what is the case where is_secure but cpu->num_ases == 1" ?
> That happens if you have a TrustZone CPU but the board has only
> connected up one address space, because there is no difference
> in the view from Secure and NonSecure. (vexpress is like this
> in hardware, and most of our board models for TZ CPUS are like
> that now even if the real h/w makes a distinction.)
> 
> I could have handled that by making the CPU init code always
> register two ASes (using the same one twice if the board code
> only passes one or using system_address_space twice if the
> board code doesn't pass one at all), but that seemed a bit wasteful.

I think it's simpler though.  Complicating the init code is better than
handling the condition throughout all the helpers...

Paolo

Re: [Qemu-devel] [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

[Peter Maydell]

On 9 November 2015 at 11:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 09/11/2015 11:58, Peter Maydell wrote:
>> You mean "what is the case where is_secure but cpu->num_ases == 1" ?
>> That happens if you have a TrustZone CPU but the board has only
>> connected up one address space, because there is no difference
>> in the view from Secure and NonSecure. (vexpress is like this
>> in hardware, and most of our board models for TZ CPUS are like
>> that now even if the real h/w makes a distinction.)
>>
>> I could have handled that by making the CPU init code always
>> register two ASes (using the same one twice if the board code
>> only passes one or using system_address_space twice if the
>> board code doesn't pass one at all), but that seemed a bit wasteful.
>
> I think it's simpler though.  Complicating the init code is better than
> handling the condition throughout all the helpers...

It was the overhead of having an extra AddressSpace that concerned
me (plus it shows up in things like 'info mtree' somewhat confusingly
if you didn't expect your board to really have 2 ASes). But I don't
feel very strongly about it (or have anything solid to base an
argument either way on).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

[Paolo Bonzini]

On 09/11/2015 12:09, Peter Maydell wrote:
>>> >> I could have handled that by making the CPU init code always
>>> >> register two ASes (using the same one twice if the board code
>>> >> only passes one or using system_address_space twice if the
>>> >> board code doesn't pass one at all), but that seemed a bit wasteful.
>> >
>> > I think it's simpler though.  Complicating the init code is better than
>> > handling the condition throughout all the helpers...
> It was the overhead of having an extra AddressSpace that concerned
> me (plus it shows up in things like 'info mtree' somewhat confusingly
> if you didn't expect your board to really have 2 ASes).

I don't think it shows up twice with address_space_init_shareable, does it?

Paolo

> But I don't
> feel very strongly about it (or have anything solid to base an
> argument either way on).

Re: [Qemu-devel] [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

[Peter Maydell]

On 9 November 2015 at 11:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 09/11/2015 12:09, Peter Maydell wrote:
>>>> >> I could have handled that by making the CPU init code always
>>>> >> register two ASes (using the same one twice if the board code
>>>> >> only passes one or using system_address_space twice if the
>>>> >> board code doesn't pass one at all), but that seemed a bit wasteful.
>>> >
>>> > I think it's simpler though.  Complicating the init code is better than
>>> > handling the condition throughout all the helpers...
>> It was the overhead of having an extra AddressSpace that concerned
>> me (plus it shows up in things like 'info mtree' somewhat confusingly
>> if you didn't expect your board to really have 2 ASes).
>
> I don't think it shows up twice with address_space_init_shareable, does it?

Yes, looking at the code you're right.

-- PMM

Re: [Qemu-devel] [PATCH 03/16] tlb_set_page_with_attrs: Take argument specifying AddressSpace to use

[Peter Maydell]

On 9 November 2015 at 10:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 November 2015 at 10:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 05/11/2015 19:15, Peter Maydell wrote:
>>> Add an argument to tlb_set_page_with_attrs which allows the target CPU code
>>> to tell the core code which AddressSpace to use.
>>>
>>> The AddressSpace is specified by the index into the array of ASes which
>>> were registered with cpu_address_space_init().
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Can it be deduced from the attributes instead?  Basically, you would have
>>
>>    int cpu_get_asidx(MemTxAttrs attrs);
>>
>> in cpu.h, which is called by tlb_set_page_with_attrs.
>> cpu_get_phys_page_asidx_debug could also be replaced by
>> cpu_get_phys_page_attrs_debug.
>
> For ARM it could, certainly (and as you say, if we go that
> way then some of the extra passing around of asidxes collapses,
> and in particular we don't need to keep them separately in the
> iotlb entries). I wasn't convinced that this would be true for
> all possible uses of AddressSpaces.

Having thought a bit more about this, I think you're right. At
any rate, our current planned multi-as use will certainly work
with the existing attrs. And we could always steal a few bits
in the MemTxAttrs to indicate the asidx for some hypothetical
future usage even if they wouldn't otherwise have to be
exposed to the rest of the system outside the CPU.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/16] target-arm: Support multiple address spaces in page table walks

[Peter Maydell]

On 9 November 2015 at 10:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 November 2015 at 10:51, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 05/11/2015 19:15, Peter Maydell wrote:
>>> If we have a secure address space, use it in page table walks:
>>>  * when doing the physical accesses to read descriptors,
>>>    make them through the correct address space
>>>  * when the final result indicates a secure access, pass the
>>>    correct address space index to tlb_set_page_with_attrs()
>>>
>>> (The descriptor reads are the only direct physical accesses
>>> made in target-arm/ for CPUs which might have TrustZone.)
>>
>> What is the case where you have no secure address space and you have
>> TrustZone?  KVM doesn't have TrustZone, so it should never be in a
>> secure regime, should it?
>
> You mean "what is the case where is_secure but cpu->num_ases == 1" ?
> That happens if you have a TrustZone CPU but the board has only
> connected up one address space, because there is no difference
> in the view from Secure and NonSecure. (vexpress is like this
> in hardware, and most of our board models for TZ CPUS are like
> that now even if the real h/w makes a distinction.)

Looking more closely at my own code I was wrong here. The
case where you have only one AS is when the CPU is using KVM.
In that case in theory we should never end up being asked to
pick an AddressSpace for a set of MemTxAttrs that say "secure"
[in the new design where we figure out the asidx from the
txattrs], but it's a bit tricky to be absolutely certain that never
happens...

thanks
-- PMM