[Qemu-devel] [PATCH 0/2]

[Qemu-devel] [PATCH 0/2]

[Emilio G. Cota]

Fix a bug introduced with dynamic TLB sizing -- we forgot to account
for possible TLB resizes during tlb_fill().

Max reported the bug and provided a reproducer here:
  https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg02283.html

Thanks,

		Emilio

[Qemu-devel] [PATCH 1/2] exec-all: document that tlb_fill can trigger a TLB resize

[Emilio G. Cota]

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 include/exec/exec-all.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index aa7b81aaf0..97b90cb0db 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -475,6 +475,11 @@ static inline void assert_no_pages_locked(void)
 struct MemoryRegionSection *iotlb_to_section(CPUState *cpu,
                                              hwaddr index, MemTxAttrs attrs);
 
+/*
+ * Note: tlb_fill() can trigger a resize of the TLB. This means that all of the
+ * caller's prior references to the TLB table (e.g. CPUTLBEntry pointers) must
+ * be discarded and looked up again (e.g. via tlb_entry()).
+ */
 void tlb_fill(CPUState *cpu, target_ulong addr, int size,
               MMUAccessType access_type, int mmu_idx, uintptr_t retaddr);
 
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] cputlb: update TLB entry/index after tlb_fill

[Emilio G. Cota]

We are failing to take into account that tlb_fill() can cause a
TLB resize, which renders prior TLB entry pointers/indices stale.
Fix it by re-doing the TLB entry lookups immediately after tlb_fill.

Fixes: 86e1eff8bc ("tcg: introduce dynamic TLB sizing", 2019-01-28)
Reported-by: Max Filippov <jcmvbkbc@gmail.com>
Tested-by: Max Filippov <jcmvbkbc@gmail.com>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 accel/tcg/softmmu_template.h | 8 ++++++++
 accel/tcg/cputlb.c           | 4 ++++
 2 files changed, 12 insertions(+)

diff --git a/accel/tcg/softmmu_template.h b/accel/tcg/softmmu_template.h
index 1fdd262ea4..e970a8b378 100644
--- a/accel/tcg/softmmu_template.h
+++ b/accel/tcg/softmmu_template.h
@@ -129,6 +129,8 @@ WORD_TYPE helper_le_ld_name(CPUArchState *env, target_ulong addr,
         if (!VICTIM_TLB_HIT(ADDR_READ, addr)) {
             tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, READ_ACCESS_TYPE,
                      mmu_idx, retaddr);
+            index = tlb_index(env, mmu_idx, addr);
+            entry = tlb_entry(env, mmu_idx, addr);
         }
         tlb_addr = entry->ADDR_READ;
     }
@@ -198,6 +200,8 @@ WORD_TYPE helper_be_ld_name(CPUArchState *env, target_ulong addr,
         if (!VICTIM_TLB_HIT(ADDR_READ, addr)) {
             tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, READ_ACCESS_TYPE,
                      mmu_idx, retaddr);
+            index = tlb_index(env, mmu_idx, addr);
+            entry = tlb_entry(env, mmu_idx, addr);
         }
         tlb_addr = entry->ADDR_READ;
     }
@@ -294,6 +298,8 @@ void helper_le_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
         if (!VICTIM_TLB_HIT(addr_write, addr)) {
             tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, MMU_DATA_STORE,
                      mmu_idx, retaddr);
+            index = tlb_index(env, mmu_idx, addr);
+            entry = tlb_entry(env, mmu_idx, addr);
         }
         tlb_addr = tlb_addr_write(entry) & ~TLB_INVALID_MASK;
     }
@@ -372,6 +378,8 @@ void helper_be_st_name(CPUArchState *env, target_ulong addr, DATA_TYPE val,
         if (!VICTIM_TLB_HIT(addr_write, addr)) {
             tlb_fill(ENV_GET_CPU(env), addr, DATA_SIZE, MMU_DATA_STORE,
                      mmu_idx, retaddr);
+            index = tlb_index(env, mmu_idx, addr);
+            entry = tlb_entry(env, mmu_idx, addr);
         }
         tlb_addr = tlb_addr_write(entry) & ~TLB_INVALID_MASK;
     }
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index f580e4dd7e..88cc8389e9 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1045,6 +1045,8 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
     if (unlikely(!tlb_hit(entry->addr_code, addr))) {
         if (!VICTIM_TLB_HIT(addr_code, addr)) {
             tlb_fill(ENV_GET_CPU(env), addr, 0, MMU_INST_FETCH, mmu_idx, 0);
+            index = tlb_index(env, mmu_idx, addr);
+            entry = tlb_entry(env, mmu_idx, addr);
         }
         assert(tlb_hit(entry->addr_code, addr));
     }
@@ -1125,6 +1127,8 @@ static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr,
         if (!VICTIM_TLB_HIT(addr_write, addr)) {
             tlb_fill(ENV_GET_CPU(env), addr, 1 << s_bits, MMU_DATA_STORE,
                      mmu_idx, retaddr);
+            index = tlb_index(env, mmu_idx, addr);
+            tlbe = tlb_entry(env, mmu_idx, addr);
         }
         tlb_addr = tlb_addr_write(tlbe) & ~TLB_INVALID_MASK;
     }
-- 
2.17.1

Re: [Qemu-devel] [PATCH 0/2]

[Richard Henderson]

On 2/9/19 8:27 AM, Emilio G. Cota wrote:
> Fix a bug introduced with dynamic TLB sizing -- we forgot to account
> for possible TLB resizes during tlb_fill().
> 
> Max reported the bug and provided a reproducer here:
>   https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg02283.html

Thanks, queued.


r~

Re: [Qemu-devel] [PATCH 1/2] exec-all: document that tlb_fill can trigger a TLB resize

[Alex Bennée]

Emilio G. Cota <cota@braap.org> writes:

> Signed-off-by: Emilio G. Cota <cota@braap.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  include/exec/exec-all.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index aa7b81aaf0..97b90cb0db 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -475,6 +475,11 @@ static inline void assert_no_pages_locked(void)
>  struct MemoryRegionSection *iotlb_to_section(CPUState *cpu,
>                                               hwaddr index, MemTxAttrs attrs);
>
> +/*
> + * Note: tlb_fill() can trigger a resize of the TLB. This means that all of the
> + * caller's prior references to the TLB table (e.g. CPUTLBEntry pointers) must
> + * be discarded and looked up again (e.g. via tlb_entry()).
> + */
>  void tlb_fill(CPUState *cpu, target_ulong addr, int size,
>                MMUAccessType access_type, int mmu_idx, uintptr_t retaddr);


--
Alex Bennée