[Qemu-devel] [PATCH for-3.0 0/2] accel/tcg: fix get_page_addr_code() victim TLB lookups

[Qemu-devel] [PATCH for-3.0 0/2] accel/tcg: fix get_page_addr_code() victim TLB lookups

[Peter Maydell]

This patchset fixes a bug in get_page_addr_code()'s
lookup of the address in the victim TLB, which was another
source of "we end up with an invalid TLB entry even after
we've done a TLB fill for it".

The second patch then removes a check that we had that was
working around the existence of the bug patch 1 fixes and
the bug that commit 68fea038553039e fixes, and instead
puts in an assertion that we definitely do have a valid TLB.

Tested with Laurent's m68k image to check we didn't regress that.

thanks
-- PMM

Peter Maydell (2):
  accel/tcg: Use correct test when looking in victim TLB for code
  accel/tcg: Assert that tlb fill gave us a valid TLB entry

 accel/tcg/cputlb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH for-3.0 1/2] accel/tcg: Use correct test when looking in victim TLB for code

[Peter Maydell]

In get_page_addr_code(), we were incorrectly looking in the victim
TLB for an entry which matched the target address for reads, not
for code accesses. This meant that we could hit on a victim TLB
entry that indicated that the address was readable but not
executable, and incorrectly bypass the call to tlb_fill() which
should generate the guest MMU exception. Fix this bug.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/cputlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 20c147d6554..2d5fb15d9a3 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -967,7 +967,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
     index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = cpu_mmu_index(env, true);
     if (unlikely(!tlb_hit(env->tlb_table[mmu_idx][index].addr_code, addr))) {
-        if (!VICTIM_TLB_HIT(addr_read, addr)) {
+        if (!VICTIM_TLB_HIT(addr_code, addr)) {
             tlb_fill(ENV_GET_CPU(env), addr, 0, MMU_INST_FETCH, mmu_idx, 0);
         }
     }
-- 
2.17.1

Re: [Qemu-devel] [PATCH for-3.0 1/2] accel/tcg: Use correct test when looking in victim TLB for code

[Richard Henderson]

On 07/13/2018 07:16 AM, Peter Maydell wrote:
> @@ -967,7 +967,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>      index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>      mmu_idx = cpu_mmu_index(env, true);
>      if (unlikely(!tlb_hit(env->tlb_table[mmu_idx][index].addr_code, addr))) {
> -        if (!VICTIM_TLB_HIT(addr_read, addr)) {
> +        if (!VICTIM_TLB_HIT(addr_code, addr)) {

Oh geez.

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


r~

[Qemu-devel] [PATCH for-3.0 2/2] accel/tcg: Assert that tlb fill gave us a valid TLB entry

[Peter Maydell]

In commit 4b1a3e1e34ad97 we added a check for whether the TLB entry
we had following a tlb_fill had the INVALID bit set.  This could
happen in some circumstances because a stale or wrong TLB entry was
pulled out of the victim cache.  However, after commit
68fea038553039e (which prevents stale entries being in the victim
cache) and the previous commit (which ensures we don't incorrectly
hit in the victim cache)) this should never be possible.

Drop the check on TLB_INVALID_MASK from the "is this a TLB_RECHECK?"
condition, and instead assert that the tlb fill procedure has given
us a valid TLB entry (or longjumped out with a guest exception).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 accel/tcg/cputlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 2d5fb15d9a3..563fa30117e 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -970,10 +970,10 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
         if (!VICTIM_TLB_HIT(addr_code, addr)) {
             tlb_fill(ENV_GET_CPU(env), addr, 0, MMU_INST_FETCH, mmu_idx, 0);
         }
+        assert(tlb_hit(env->tlb_table[mmu_idx][index].addr_code, addr));
     }
 
-    if (unlikely((env->tlb_table[mmu_idx][index].addr_code &
-                  (TLB_RECHECK | TLB_INVALID_MASK)) == TLB_RECHECK)) {
+    if (unlikely(env->tlb_table[mmu_idx][index].addr_code & TLB_RECHECK)) {
         /*
          * This is a TLB_RECHECK access, where the MMU protection
          * covers a smaller range than a target page, and we must
-- 
2.17.1

Re: [Qemu-devel] [PATCH for-3.0 2/2] accel/tcg: Assert that tlb fill gave us a valid TLB entry

[Richard Henderson]

On 07/13/2018 07:16 AM, Peter Maydell wrote:
> In commit 4b1a3e1e34ad97 we added a check for whether the TLB entry
> we had following a tlb_fill had the INVALID bit set.  This could
> happen in some circumstances because a stale or wrong TLB entry was
> pulled out of the victim cache.  However, after commit
> 68fea038553039e (which prevents stale entries being in the victim
> cache) and the previous commit (which ensures we don't incorrectly
> hit in the victim cache)) this should never be possible.
> 
> Drop the check on TLB_INVALID_MASK from the "is this a TLB_RECHECK?"
> condition, and instead assert that the tlb fill procedure has given
> us a valid TLB entry (or longjumped out with a guest exception).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  accel/tcg/cputlb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH for-3.0 0/2] accel/tcg: fix get_page_addr_code() victim TLB lookups

[Peter Maydell]

On 13 July 2018 at 15:16, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset fixes a bug in get_page_addr_code()'s
> lookup of the address in the victim TLB, which was another
> source of "we end up with an invalid TLB entry even after
> we've done a TLB fill for it".
>
> The second patch then removes a check that we had that was
> working around the existence of the bug patch 1 fixes and
> the bug that commit 68fea038553039e fixes, and instead
> puts in an assertion that we definitely do have a valid TLB.
>
> Tested with Laurent's m68k image to check we didn't regress that.

Thanks for the review -- I'll take these via target-arm.next
since RTH doesn't have any other tcg patches for rc1.

-- PMM