Re: [Qemu-devel] [PATCH 3/3] cputlb: Introduce TLB_BSWAP

[Richard Henderson]

On 9/11/19 10:56 AM, Tony Nguyen wrote:
>> @@ -1372,26 +1364,27 @@ load_helper(CPUArchState *env, target_ulong addr, 
>> TCGMemOpIdx oi,
>>              /* On watchpoint hit, this will longjmp out.  */
>>              cpu_check_watchpoint(env_cpu(env), addr, size,
>>                                   iotlbentry->attrs, BP_MEM_READ, retaddr);
>> -
>> -            /* The backing page may or may not require I/O.  */
>> -            tlb_addr &= ~TLB_WATCHPOINT;
>> -            if ((tlb_addr & ~TARGET_PAGE_MASK) == 0) {
>> -                goto do_aligned_access;
>> -            }
>>          }
>>  
>>          /* Handle I/O access.  */
>> -        return io_readx(env, iotlbentry, mmu_idx, addr,
>> -                        retaddr, access_type, op);
>> -    }
>> +        if (likely(tlb_addr & TLB_MMIO)) {
>> +            return io_readx(env, iotlbentry, mmu_idx, addr,
>> +                            retaddr, access_type,
>> +                            op ^ (tlb_addr & TLB_BSWAP ? MO_BSWAP : 0));
>> +        }
> 
> Previously, the end of if (unlikely(tlb_addr & ~TARGET_PAGE_MASK)) branch
> called and returned the result of io_readx.

Correct.  However, rather thank clearing TLB_WATCHPOINT and TLB_BSWAP, it
seemed easier to test for those bits that *do* require that we call io_readx.

As we've seen from the bug leading to this patch set, it's invalid to call
io_readx on anything that doesn't have TLB_MMIO set -- we'll either crash due
to the missing read accessor or reach the point at which we issue a bus error
for an i/o operation without a device.

BTW, there's a bug in this same location for store_helper in that I need to
also test for TLB_NOTDIRTY, which also goes through io_writex for the moment.
That bug is trivially shown during the make check migration tests.  Due to the
late hour I failed to run those before posting this patch set.  Will be fixed
in v2.


r~