| Message ID | 20250524144031.49810-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | accel/tcg: Fix atomic_mmu_lookup vs TLB_FORCE_SLOW | expand |
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>
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;
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.
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>
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~
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~
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;
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(-)