[Qemu-devel] [PATCH 0/2] cputlb: document iotlb.addr, fix txfail physaddr

[Qemu-devel] [PATCH 0/2] cputlb: document iotlb.addr, fix txfail physaddr

[Peter Maydell]

This patchset is two vaguely related patches that I wrote
while I was trying to understand the cputlb code enough to
add support for small-MPU-regions for v7M/v8M.

Patch 1 is just a docs comment patch, describing what the
CPUIOTLBEntry 'addr' field actually contains.

Patch 2 fixes a bug that doesn't yet have any visible effects
(we were passing cpu_transaction_failed() the wrong value for
the physaddr) roughly as suggested by Paolo back in
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg02419.html

thanks
-- PMM

Peter Maydell (2):
  cpu-defs.h: Document CPUIOTLBEntry 'addr' field
  cputlb: Pass cpu_transaction_failed() the correct physaddr

 include/exec/cpu-defs.h |  9 +++++++
 include/exec/exec-all.h | 13 ++++++++--
 accel/tcg/cputlb.c      | 56 +++++++++++++++++++++++++++++++----------
 exec.c                  |  5 ++--
 4 files changed, 66 insertions(+), 17 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] cpu-defs.h: Document CPUIOTLBEntry 'addr' field

[Peter Maydell]

The 'addr' field in the CPUIOTLBEntry struct has a rather non-obvious
use; add a comment documenting it (reverse-engineered from what
the code that sets it is doing).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu-defs.h |  9 +++++++++
 accel/tcg/cputlb.c      | 12 ++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/exec/cpu-defs.h b/include/exec/cpu-defs.h
index e43ff8346b1..452e82d21c6 100644
--- a/include/exec/cpu-defs.h
+++ b/include/exec/cpu-defs.h
@@ -127,6 +127,15 @@ QEMU_BUILD_BUG_ON(sizeof(CPUTLBEntry) != (1 << CPU_TLB_ENTRY_BITS));
  * structs into one.)
  */
 typedef struct CPUIOTLBEntry {
+    /*
+     * @addr contains:
+     *  - in the lower TARGET_PAGE_BITS, a physical section number
+     *  - with the lower TARGET_PAGE_BITS masked off, an offset which
+     *    must be added to the virtual address to obtain:
+     *     + the ramaddr_t of the target RAM (if the physical section
+     *       number is PHYS_SECTION_NOTDIRTY or PHYS_SECTION_ROM)
+     *     + the offset within the target MemoryRegion (otherwise)
+     */
     hwaddr addr;
     MemTxAttrs attrs;
 } CPUIOTLBEntry;
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 05439039e91..355ded27024 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -664,6 +664,18 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong vaddr,
     env->iotlb_v[mmu_idx][vidx] = env->iotlb[mmu_idx][index];
 
     /* refill the tlb */
+    /*
+     * At this point iotlb contains a physical section number in the lower
+     * TARGET_PAGE_BITS, and either
+     *  + the ramaddr_t of the page base of the target RAM (if NOTDIRTY or ROM)
+     *  + the offset within section->mr of the page base (otherwise)
+     * We subtract the vaddr (which is page aligned and thus won't
+     * disturb the low bits) to give an offset which can be added to the
+     * (non-page-aligned) vaddr of the eventual memory access to get
+     * the MemoryRegion offset for the access. Note that the vaddr we
+     * subtract here is that of the page base, and not the same as the
+     * vaddr we add back in io_readx()/io_writex()/get_page_addr_code().
+     */
     env->iotlb[mmu_idx][index].addr = iotlb - vaddr;
     env->iotlb[mmu_idx][index].attrs = attrs;
 
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] cputlb: Pass cpu_transaction_failed() the correct physaddr

[Peter Maydell]

The API for cpu_transaction_failed() says that it takes the physical
address for the failed transaction. However we were actually passing
it the offset within the target MemoryRegion. We don't currently
have any target CPU implementations of this hook that require the
physical address; fix this bug so we don't get confused if we ever
do add one.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/exec-all.h | 13 ++++++++++--
 accel/tcg/cputlb.c      | 44 +++++++++++++++++++++++++++++------------
 exec.c                  |  5 +++--
 3 files changed, 45 insertions(+), 17 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 4d09eaba72d..aed55aaaa7d 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -437,8 +437,17 @@ void tb_lock_reset(void);
 
 #if !defined(CONFIG_USER_ONLY)
 
-struct MemoryRegion *iotlb_to_region(CPUState *cpu,
-                                     hwaddr index, MemTxAttrs attrs);
+/**
+ * iotlb_to_section:
+ * @cpu: CPU performing the access
+ * @index: TCG CPU IOTLB entry
+ *
+ * Given a TCG CPU IOTLB entry, return the MemoryRegionSection that
+ * it refers to. @index will have been initially created and returned
+ * by memory_region_section_get_iotlb().
+ */
+struct MemoryRegionSection *iotlb_to_section(CPUState *cpu,
+                                             hwaddr index, MemTxAttrs attrs);
 
 void tlb_fill(CPUState *cpu, target_ulong addr, int size,
               MMUAccessType access_type, int mmu_idx, uintptr_t retaddr);
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 355ded27024..77aa982cfd8 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -777,13 +777,16 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                          target_ulong addr, uintptr_t retaddr, int size)
 {
     CPUState *cpu = ENV_GET_CPU(env);
-    hwaddr physaddr = iotlbentry->addr;
-    MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs);
+    hwaddr mr_offset;
+    MemoryRegionSection *section;
+    MemoryRegion *mr;
     uint64_t val;
     bool locked = false;
     MemTxResult r;
 
-    physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
+    section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
+    mr = section->mr;
+    mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
     cpu->mem_io_pc = retaddr;
     if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
         cpu_io_recompile(cpu, retaddr);
@@ -795,9 +798,13 @@ static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         qemu_mutex_lock_iothread();
         locked = true;
     }
-    r = memory_region_dispatch_read(mr, physaddr,
+    r = memory_region_dispatch_read(mr, mr_offset,
                                     &val, size, iotlbentry->attrs);
     if (r != MEMTX_OK) {
+        hwaddr physaddr = mr_offset +
+            section->offset_within_address_space -
+            section->offset_within_region;
+
         cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_LOAD,
                                mmu_idx, iotlbentry->attrs, r, retaddr);
     }
@@ -814,12 +821,15 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                       uintptr_t retaddr, int size)
 {
     CPUState *cpu = ENV_GET_CPU(env);
-    hwaddr physaddr = iotlbentry->addr;
-    MemoryRegion *mr = iotlb_to_region(cpu, physaddr, iotlbentry->attrs);
+    hwaddr mr_offset;
+    MemoryRegionSection *section;
+    MemoryRegion *mr;
     bool locked = false;
     MemTxResult r;
 
-    physaddr = (physaddr & TARGET_PAGE_MASK) + addr;
+    section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
+    mr = section->mr;
+    mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
     if (mr != &io_mem_rom && mr != &io_mem_notdirty && !cpu->can_do_io) {
         cpu_io_recompile(cpu, retaddr);
     }
@@ -830,9 +840,13 @@ static void io_writex(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
         qemu_mutex_lock_iothread();
         locked = true;
     }
-    r = memory_region_dispatch_write(mr, physaddr,
+    r = memory_region_dispatch_write(mr, mr_offset,
                                      val, size, iotlbentry->attrs);
     if (r != MEMTX_OK) {
+        hwaddr physaddr = mr_offset +
+            section->offset_within_address_space -
+            section->offset_within_region;
+
         cpu_transaction_failed(cpu, physaddr, addr, size, MMU_DATA_STORE,
                                mmu_idx, iotlbentry->attrs, r, retaddr);
     }
@@ -880,12 +894,13 @@ static bool victim_tlb_hit(CPUArchState *env, size_t mmu_idx, size_t index,
  */
 tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
 {
-    int mmu_idx, index, pd;
+    int mmu_idx, index;
     void *p;
     MemoryRegion *mr;
+    MemoryRegionSection *section;
     CPUState *cpu = ENV_GET_CPU(env);
     CPUIOTLBEntry *iotlbentry;
-    hwaddr physaddr;
+    hwaddr physaddr, mr_offset;
 
     index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = cpu_mmu_index(env, true);
@@ -896,8 +911,8 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
         }
     }
     iotlbentry = &env->iotlb[mmu_idx][index];
-    pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
-    mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
+    section = iotlb_to_section(cpu, iotlbentry->addr, iotlbentry->attrs);
+    mr = section->mr;
     if (memory_region_is_unassigned(mr)) {
         qemu_mutex_lock_iothread();
         if (memory_region_request_mmio_ptr(mr, addr)) {
@@ -918,7 +933,10 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
          * and use the MemTXResult it produced). However it is the
          * simplest place we have currently available for the check.
          */
-        physaddr = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
+        mr_offset = (iotlbentry->addr & TARGET_PAGE_MASK) + addr;
+        physaddr = mr_offset +
+            section->offset_within_address_space -
+            section->offset_within_region;
         cpu_transaction_failed(cpu, physaddr, addr, 0, MMU_INST_FETCH, mmu_idx,
                                iotlbentry->attrs, MEMTX_DECODE_ERROR, 0);
 
diff --git a/exec.c b/exec.c
index f6645ede0c2..9cbba6adcd3 100644
--- a/exec.c
+++ b/exec.c
@@ -2897,14 +2897,15 @@ static const MemoryRegionOps readonly_mem_ops = {
     },
 };
 
-MemoryRegion *iotlb_to_region(CPUState *cpu, hwaddr index, MemTxAttrs attrs)
+MemoryRegionSection *iotlb_to_section(CPUState *cpu,
+                                      hwaddr index, MemTxAttrs attrs)
 {
     int asidx = cpu_asidx_from_attrs(cpu, attrs);
     CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx];
     AddressSpaceDispatch *d = atomic_rcu_read(&cpuas->memory_dispatch);
     MemoryRegionSection *sections = d->map.sections;
 
-    return sections[index & ~TARGET_PAGE_MASK].mr;
+    return &sections[index & ~TARGET_PAGE_MASK];
 }
 
 static void io_mem_init(void)
-- 
2.17.1

Re: [Qemu-devel] [PATCH 0/2] cputlb: document iotlb.addr, fix txfail physaddr

[Paolo Bonzini]

On 11/06/2018 14:56, Peter Maydell wrote:
> This patchset is two vaguely related patches that I wrote
> while I was trying to understand the cputlb code enough to
> add support for small-MPU-regions for v7M/v8M.
> 
> Patch 1 is just a docs comment patch, describing what the
> CPUIOTLBEntry 'addr' field actually contains.
> 
> Patch 2 fixes a bug that doesn't yet have any visible effects
> (we were passing cpu_transaction_failed() the wrong value for
> the physaddr) roughly as suggested by Paolo back in
> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg02419.html
> 
> thanks
> -- PMM

Both patches

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 1/2] cpu-defs.h: Document CPUIOTLBEntry 'addr' field

[Richard Henderson]

On 06/11/2018 02:56 AM, Peter Maydell wrote:
> The 'addr' field in the CPUIOTLBEntry struct has a rather non-obvious
> use; add a comment documenting it (reverse-engineered from what
> the code that sets it is doing).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/cpu-defs.h |  9 +++++++++
>  accel/tcg/cputlb.c      | 12 ++++++++++++
>  2 files changed, 21 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/2] cputlb: Pass cpu_transaction_failed() the correct physaddr

[Richard Henderson]

On 06/11/2018 02:56 AM, Peter Maydell wrote:
> The API for cpu_transaction_failed() says that it takes the physical
> address for the failed transaction. However we were actually passing
> it the offset within the target MemoryRegion. We don't currently
> have any target CPU implementations of this hook that require the
> physical address; fix this bug so we don't get confused if we ever
> do add one.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/exec-all.h | 13 ++++++++++--
>  accel/tcg/cputlb.c      | 44 +++++++++++++++++++++++++++++------------
>  exec.c                  |  5 +++--
>  3 files changed, 45 insertions(+), 17 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 1/2] cpu-defs.h: Document CPUIOTLBEntry 'addr' field

[Emilio G. Cota]

On Mon, Jun 11, 2018 at 13:56:32 +0100, Peter Maydell wrote:
>  typedef struct CPUIOTLBEntry {
> +    /*
> +     * @addr contains:
> +     *  - in the lower TARGET_PAGE_BITS, a physical section number
> +     *  - with the lower TARGET_PAGE_BITS masked off, an offset which
> +     *    must be added to the virtual address to obtain:
> +     *     + the ramaddr_t of the target RAM (if the physical section

s/ramaddr_t/ram_addr_t/ ? Also in cputlb.c:

> +     * TARGET_PAGE_BITS, and either
> +     *  + the ramaddr_t of the page base of the target RAM (if NOTDIRTY or ROM)

Thanks,

		Emilio

Re: [Qemu-devel] [PATCH 0/2] cputlb: document iotlb.addr, fix txfail physaddr

[Peter Maydell]

On 11 June 2018 at 13:56, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset is two vaguely related patches that I wrote
> while I was trying to understand the cputlb code enough to
> add support for small-MPU-regions for v7M/v8M.
>
> Patch 1 is just a docs comment patch, describing what the
> CPUIOTLBEntry 'addr' field actually contains.
>
> Patch 2 fixes a bug that doesn't yet have any visible effects
> (we were passing cpu_transaction_failed() the wrong value for
> the physaddr) roughly as suggested by Paolo back in
> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg02419.html
>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   cpu-defs.h: Document CPUIOTLBEntry 'addr' field
>   cputlb: Pass cpu_transaction_failed() the correct physaddr

I'll put these in via target-arm.next (with the ramaddr_t/ram_addr_t
typo fixed), unless anybody would prefer them to go via another route.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/2] cputlb: document iotlb.addr, fix txfail physaddr

[Richard Henderson]

On 06/15/2018 12:13 AM, Peter Maydell wrote:
>> Peter Maydell (2):
>>   cpu-defs.h: Document CPUIOTLBEntry 'addr' field
>>   cputlb: Pass cpu_transaction_failed() the correct physaddr
> 
> I'll put these in via target-arm.next (with the ramaddr_t/ram_addr_t
> typo fixed), unless anybody would prefer them to go via another route.

Perfect, thanks.


r~