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

[Richard Henderson]

On 6/14/21 3:18 AM, Alex Bennée wrote:
> 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?

I didn't even look at the compiler output.

r~

> Otherwise seems sane to me:
>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>