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

[Richard Henderson <richard.henderson@linaro.org>]

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>
>