[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] accel/tcg: Probe the proper permissions for atomic ops
From: |
Alex Bennée |
Subject: |
Re: [PATCH] accel/tcg: Probe the proper permissions for atomic ops |
Date: |
Mon, 14 Jun 2021 11:18:12 +0100 |
User-agent: |
mu4e 1.5.13; emacs 28.0.50 |
Richard Henderson <richard.henderson@linaro.org> 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 <richard.henderson@linaro.org>
> ---
<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 <alex.bennee@linaro.org>
--
Alex Bennée