Richard Henderson <[email protected]> writes:
> We had a single ATOMIC_MMU_LOOKUP macro that probed for > read+write on all atomic ops. This is incorrect for > plain atomic load and atomic store. > > For user-only, we rely on the host page permissions. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/390 > Signed-off-by: Richard Henderson <[email protected]> > --- <snip> > > -/* Probe for a read-modify-write atomic operation. Do not allow unaligned > - * operations, or io operations to proceed. Return the host address. */ > +/* > + * Probe for an atomic operation. Do not allow unaligned operations, > + * or io operations to proceed. Return the host address. > + * > + * @prot may be PAGE_READ, PAGE_WRITE, or PAGE_READ|PAGE_WRITE. > + */ > static void *atomic_mmu_lookup(CPUArchState *env, target_ulong addr, > - TCGMemOpIdx oi, uintptr_t retaddr) > + TCGMemOpIdx oi, int prot, uintptr_t retaddr) > { > size_t mmu_idx = get_mmuidx(oi); > - uintptr_t index = tlb_index(env, mmu_idx, addr); > - CPUTLBEntry *tlbe = tlb_entry(env, mmu_idx, addr); > - target_ulong tlb_addr = tlb_addr_write(tlbe); > MemOp mop = get_memop(oi); > int a_bits = get_alignment_bits(mop); > int s_bits = mop & MO_SIZE; > + uintptr_t index; > + CPUTLBEntry *tlbe; > + target_ulong tlb_addr; > void *hostaddr; > > /* Adjust the given return address. */ > @@ -1775,15 +1779,45 @@ static void *atomic_mmu_lookup(CPUArchState *env, > target_ulong addr, > goto stop_the_world; > } > > + index = tlb_index(env, mmu_idx, addr); > + tlbe = tlb_entry(env, mmu_idx, addr); > + > /* Check TLB entry and enforce page permissions. */ > - if (!tlb_hit(tlb_addr, addr)) { > - if (!VICTIM_TLB_HIT(addr_write, addr)) { > - tlb_fill(env_cpu(env), addr, 1 << s_bits, MMU_DATA_STORE, > - mmu_idx, retaddr); > - index = tlb_index(env, mmu_idx, addr); > - tlbe = tlb_entry(env, mmu_idx, addr); > + if (prot & PAGE_WRITE) { > + tlb_addr = tlb_addr_write(tlbe); > + if (!tlb_hit(tlb_addr, addr)) { > + if (!VICTIM_TLB_HIT(addr_write, addr)) { > + tlb_fill(env_cpu(env), addr, 1 << s_bits, > + MMU_DATA_STORE, mmu_idx, retaddr); > + index = tlb_index(env, mmu_idx, addr); > + tlbe = tlb_entry(env, mmu_idx, addr); > + } > + tlb_addr = tlb_addr_write(tlbe) & ~TLB_INVALID_MASK; > + } > + > + /* Let the guest notice RMW on a write-only page. */ > + if ((prot & PAGE_READ) && > + unlikely(tlbe->addr_read != (tlb_addr & ~TLB_NOTDIRTY))) { > + tlb_fill(env_cpu(env), addr, 1 << s_bits, > + MMU_DATA_LOAD, mmu_idx, retaddr); > + /* > + * Since we don't support reads and writes to different > addresses, > + * and we do have the proper page loaded for write, this > shouldn't > + * ever return. But just in case, handle via stop-the-world. > + */ > + goto stop_the_world; > + } > + } else /* if (prot & PAGE_READ) */ { I guess the compiler doesn't have enough info to know that !PROT_WRITE implies PROT_READ and ends up doing an extra check it doesn't need to otherwise? Otherwise seems sane to me: Reviewed-by: Alex Bennée <[email protected]> -- Alex Bennée
