1. Patchew
  2. QEMU
  3. View series

[PATCH] accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW

Richard Henderson posted 1 patch 5 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20250524144031.49810-1-richard.henderson@linaro.org
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>
accel/tcg/cputlb.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
[PATCH] accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW
Posted by Richard Henderson 5 months, 3 weeks ago 
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>
---
 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;
-- 
2.43.0
Re: [PATCH] accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW
Posted by Pierrick Bouvier 5 months, 3 weeks ago 
On 5/24/25 7:40 AM, Richard Henderson 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>
> ---
>   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

[...]

> @@ -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;

The watchpoint part is an additional cleanup, (BP_MEM_READ or 
BP_MEM_WRITE implies TLB_WATCHPOINT is set). No problem to include it 
though, it might just be confusing for the reviewer.
Re: [PATCH] accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW
Posted by Richard Henderson 5 months, 3 weeks ago 
On 5/27/25 21:45, Pierrick Bouvier wrote:
> On 5/24/25 7:40 AM, Richard Henderson 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>
>> ---
>>   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
> 
> [...]
> 
>> @@ -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;
> 
> The watchpoint part is an additional cleanup, (BP_MEM_READ or BP_MEM_WRITE implies 
> TLB_WATCHPOINT is set). No problem to include it though, it might just be confusing for 
> the reviewer.

The watchpoint cleanup is required, since I remove TLB_FORCE_SLOW from the flags.  I 
suppose *that* isn't strictly necessary, but it's what we do elsewhere while combining 
"fast" and slow_flags.


r~
Re: [PATCH] accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW
Posted by Pierrick Bouvier 5 months, 3 weeks ago 
On 5/27/25 11:42 PM, Richard Henderson wrote:
> On 5/27/25 21:45, Pierrick Bouvier wrote:
>> On 5/24/25 7:40 AM, Richard Henderson 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>
>>> ---
>>>    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
>>
>> [...]
>>
>>> @@ -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;
>>
>> The watchpoint part is an additional cleanup, (BP_MEM_READ or BP_MEM_WRITE implies
>> TLB_WATCHPOINT is set). No problem to include it though, it might just be confusing for
>> the reviewer.
> 
> The watchpoint cleanup is required, since I remove TLB_FORCE_SLOW from the flags.  I
> suppose *that* isn't strictly necessary, but it's what we do elsewhere while combining
> "fast" and slow_flags.
> 

Yes! I was just referring to the last part where you remove if 
(wp_flags) but kept the whole diff chunk for convenience.

> 
> r~
Re: [PATCH] accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW
Posted by Pierrick Bouvier 5 months, 3 weeks ago 
On 5/24/25 7:40 AM, Richard Henderson 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>
> ---
>   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];
>

[...]

Looks good to me.
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Re: [PATCH] accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW
Posted by Jonathan Cameron via 5 months, 3 weeks ago 
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;
Re: [PATCH] accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW
Posted by Philippe Mathieu-Daudé 5 months, 3 weeks ago 
On 24/5/25 16:40, Richard Henderson 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")

Cc'ing Pierrick

> Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   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;

Patch LGTM but this is outside my comfort zone, so better wait for
a second review ;)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.