Re: [PATCH] accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW

[Jonathan Cameron via <qemu-devel@nongnu.org>]

On Sat, 24 May 2025 15:40:31 +0100
Richard Henderson <richard.henderson@linaro.org> wrote:

> When we moved TLB_MMIO and TLB_DISCARD_WRITE to TLB_SLOW_FLAGS_MASK,
> we failed to update atomic_mmu_lookup to properly reconstruct flags.
> 
> Fixes: 24b5e0fdb543 ("include/exec: Move TLB_MMIO, TLB_DISCARD_WRITE to slow flags")
> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

I've run basic tests (the ones that were tripping over this 100% of the time)
and all looks good.  Thanks!  I'll run some more comprehensive testing this afternoon
but looking good.

Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Way outside my comfort zone so not appropriate for me to say more than
I tested it!

> ---
>  accel/tcg/cputlb.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
> index 5f6d7c601c..86d0deb08c 100644
> --- a/accel/tcg/cputlb.c
> +++ b/accel/tcg/cputlb.c
> @@ -1871,8 +1871,12 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
>          goto stop_the_world;
>      }
>  
> -    /* Collect tlb flags for read. */
> +    /* Finish collecting tlb flags for both read and write. */
> +    full = &cpu->neg.tlb.d[mmu_idx].fulltlb[index];
>      tlb_addr |= tlbe->addr_read;
> +    tlb_addr &= TLB_FLAGS_MASK & ~TLB_FORCE_SLOW;
> +    tlb_addr |= full->slow_flags[MMU_DATA_STORE];
> +    tlb_addr |= full->slow_flags[MMU_DATA_LOAD];
>  
>      /* Notice an IO access or a needs-MMU-lookup access */
>      if (unlikely(tlb_addr & (TLB_MMIO | TLB_DISCARD_WRITE))) {
> @@ -1882,13 +1886,12 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
>      }
>  
>      hostaddr = (void *)((uintptr_t)addr + tlbe->addend);
> -    full = &cpu->neg.tlb.d[mmu_idx].fulltlb[index];
>  
>      if (unlikely(tlb_addr & TLB_NOTDIRTY)) {
>          notdirty_write(cpu, addr, size, full, retaddr);
>      }
>  
> -    if (unlikely(tlb_addr & TLB_FORCE_SLOW)) {
> +    if (unlikely(tlb_addr & TLB_WATCHPOINT)) {
>          int wp_flags = 0;
>  
>          if (full->slow_flags[MMU_DATA_STORE] & TLB_WATCHPOINT) {
> @@ -1897,10 +1900,8 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi,
>          if (full->slow_flags[MMU_DATA_LOAD] & TLB_WATCHPOINT) {
>              wp_flags |= BP_MEM_READ;
>          }
> -        if (wp_flags) {
> -            cpu_check_watchpoint(cpu, addr, size,
> -                                 full->attrs, wp_flags, retaddr);
> -        }
> +        cpu_check_watchpoint(cpu, addr, size,
> +                             full->attrs, wp_flags, retaddr);
>      }
>  
>      return hostaddr;