Re: [PATCH] accel/tcg: Probe the proper permissions for atomic ops

["Alex Bennée" <alex.bennee@linaro.org>]

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