| Message ID | 20241009150855.804605-6-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg: Convert victim tlb to IntervalTree | expand |
On 10/9/24 08:08, Richard Henderson wrote: > The INVALID bit should only be auto-cleared when we have > just called tlb_fill, not along the victim_tlb_hit path. > > In atomic_mmu_lookup, rename tlb_addr to flags, as that > is what we're actually carrying around. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > accel/tcg/cputlb.c | 33 ++++++++++++++++++++++----------- > 1 file changed, 22 insertions(+), 11 deletions(-) > > diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c > index 6773874f2d..fd8da8586f 100644 > --- a/accel/tcg/cputlb.c > +++ b/accel/tcg/cputlb.c > @@ -1657,7 +1657,7 @@ static bool mmu_lookup1(CPUState *cpu, MMULookupPageData *data, MemOp memop, > uint64_t tlb_addr = tlb_read_idx(entry, access_type); > bool maybe_resized = false; > CPUTLBEntryFull *full; > - int flags; > + int flags = TLB_FLAGS_MASK & ~TLB_FORCE_SLOW; > > /* If the TLB entry is for a different page, reload and try again. */ > if (!tlb_hit(tlb_addr, addr)) { > @@ -1668,8 +1668,14 @@ static bool mmu_lookup1(CPUState *cpu, MMULookupPageData *data, MemOp memop, > maybe_resized = true; > index = tlb_index(cpu, mmu_idx, addr); > entry = tlb_entry(cpu, mmu_idx, addr); > + /* > + * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately, > + * to force the next access through tlb_fill. We've just > + * called tlb_fill, so we know that this entry *is* valid. > + */ > + flags &= ~TLB_INVALID_MASK; > } > - tlb_addr = tlb_read_idx(entry, access_type) & ~TLB_INVALID_MASK; > + tlb_addr = tlb_read_idx(entry, access_type); > } > > full = &cpu->neg.tlb.d[mmu_idx].fulltlb[index]; > @@ -1819,10 +1825,10 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi, > MemOp mop = get_memop(oi); > uintptr_t index; > CPUTLBEntry *tlbe; > - vaddr tlb_addr; > void *hostaddr; > CPUTLBEntryFull *full; > bool did_tlb_fill = false; > + int flags; > > tcg_debug_assert(mmu_idx < NB_MMU_MODES); > > @@ -1833,8 +1839,8 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi, > tlbe = tlb_entry(cpu, mmu_idx, addr); > > /* Check TLB entry and enforce page permissions. */ > - tlb_addr = tlb_addr_write(tlbe); > - if (!tlb_hit(tlb_addr, addr)) { > + flags = TLB_FLAGS_MASK; > + if (!tlb_hit(tlb_addr_write(tlbe), addr)) { > if (!victim_tlb_hit(cpu, mmu_idx, index, MMU_DATA_STORE, > addr & TARGET_PAGE_MASK)) { > tlb_fill_align(cpu, addr, MMU_DATA_STORE, mmu_idx, > @@ -1842,8 +1848,13 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi, > did_tlb_fill = true; > index = tlb_index(cpu, mmu_idx, addr); > tlbe = tlb_entry(cpu, mmu_idx, addr); > + /* > + * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately, > + * to force the next access through tlb_fill. We've just > + * called tlb_fill, so we know that this entry *is* valid. > + */ > + flags &= ~TLB_INVALID_MASK; > } > - tlb_addr = tlb_addr_write(tlbe) & ~TLB_INVALID_MASK; > } > > /* > @@ -1879,11 +1890,11 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi, > goto stop_the_world; > } > > - /* Collect tlb flags for read. */ > - tlb_addr |= tlbe->addr_read; > + /* Collect tlb flags for read and write. */ > + flags &= tlbe->addr_read | tlb_addr_write(tlbe); > > /* Notice an IO access or a needs-MMU-lookup access */ > - if (unlikely(tlb_addr & (TLB_MMIO | TLB_DISCARD_WRITE))) { > + if (unlikely(flags & (TLB_MMIO | TLB_DISCARD_WRITE))) { > /* There's really nothing that can be done to > support this apart from stop-the-world. */ > goto stop_the_world; > @@ -1892,11 +1903,11 @@ 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)) { > + if (unlikely(flags & TLB_NOTDIRTY)) { > notdirty_write(cpu, addr, size, full, retaddr); > } > > - if (unlikely(tlb_addr & TLB_FORCE_SLOW)) { > + if (unlikely(flags & TLB_FORCE_SLOW)) { > int wp_flags = 0; > > if (full->slow_flags[MMU_DATA_STORE] & TLB_WATCHPOINT) { Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c index 6773874f2d..fd8da8586f 100644 --- a/accel/tcg/cputlb.c +++ b/accel/tcg/cputlb.c @@ -1657,7 +1657,7 @@ static bool mmu_lookup1(CPUState *cpu, MMULookupPageData *data, MemOp memop, uint64_t tlb_addr = tlb_read_idx(entry, access_type); bool maybe_resized = false; CPUTLBEntryFull *full; - int flags; + int flags = TLB_FLAGS_MASK & ~TLB_FORCE_SLOW; /* If the TLB entry is for a different page, reload and try again. */ if (!tlb_hit(tlb_addr, addr)) { @@ -1668,8 +1668,14 @@ static bool mmu_lookup1(CPUState *cpu, MMULookupPageData *data, MemOp memop, maybe_resized = true; index = tlb_index(cpu, mmu_idx, addr); entry = tlb_entry(cpu, mmu_idx, addr); + /* + * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately, + * to force the next access through tlb_fill. We've just + * called tlb_fill, so we know that this entry *is* valid. + */ + flags &= ~TLB_INVALID_MASK; } - tlb_addr = tlb_read_idx(entry, access_type) & ~TLB_INVALID_MASK; + tlb_addr = tlb_read_idx(entry, access_type); } full = &cpu->neg.tlb.d[mmu_idx].fulltlb[index]; @@ -1819,10 +1825,10 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi, MemOp mop = get_memop(oi); uintptr_t index; CPUTLBEntry *tlbe; - vaddr tlb_addr; void *hostaddr; CPUTLBEntryFull *full; bool did_tlb_fill = false; + int flags; tcg_debug_assert(mmu_idx < NB_MMU_MODES); @@ -1833,8 +1839,8 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi, tlbe = tlb_entry(cpu, mmu_idx, addr); /* Check TLB entry and enforce page permissions. */ - tlb_addr = tlb_addr_write(tlbe); - if (!tlb_hit(tlb_addr, addr)) { + flags = TLB_FLAGS_MASK; + if (!tlb_hit(tlb_addr_write(tlbe), addr)) { if (!victim_tlb_hit(cpu, mmu_idx, index, MMU_DATA_STORE, addr & TARGET_PAGE_MASK)) { tlb_fill_align(cpu, addr, MMU_DATA_STORE, mmu_idx, @@ -1842,8 +1848,13 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi, did_tlb_fill = true; index = tlb_index(cpu, mmu_idx, addr); tlbe = tlb_entry(cpu, mmu_idx, addr); + /* + * With PAGE_WRITE_INV, we set TLB_INVALID_MASK immediately, + * to force the next access through tlb_fill. We've just + * called tlb_fill, so we know that this entry *is* valid. + */ + flags &= ~TLB_INVALID_MASK; } - tlb_addr = tlb_addr_write(tlbe) & ~TLB_INVALID_MASK; } /* @@ -1879,11 +1890,11 @@ static void *atomic_mmu_lookup(CPUState *cpu, vaddr addr, MemOpIdx oi, goto stop_the_world; } - /* Collect tlb flags for read. */ - tlb_addr |= tlbe->addr_read; + /* Collect tlb flags for read and write. */ + flags &= tlbe->addr_read | tlb_addr_write(tlbe); /* Notice an IO access or a needs-MMU-lookup access */ - if (unlikely(tlb_addr & (TLB_MMIO | TLB_DISCARD_WRITE))) { + if (unlikely(flags & (TLB_MMIO | TLB_DISCARD_WRITE))) { /* There's really nothing that can be done to support this apart from stop-the-world. */ goto stop_the_world; @@ -1892,11 +1903,11 @@ 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)) { + if (unlikely(flags & TLB_NOTDIRTY)) { notdirty_write(cpu, addr, size, full, retaddr); } - if (unlikely(tlb_addr & TLB_FORCE_SLOW)) { + if (unlikely(flags & TLB_FORCE_SLOW)) { int wp_flags = 0; if (full->slow_flags[MMU_DATA_STORE] & TLB_WATCHPOINT) {
The INVALID bit should only be auto-cleared when we have just called tlb_fill, not along the victim_tlb_hit path. In atomic_mmu_lookup, rename tlb_addr to flags, as that is what we're actually carrying around. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- accel/tcg/cputlb.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)