On 4/27/20 3:48 AM, Peter Maydell wrote:
> probe_access() handles watchpoints. Why doesn't probe_access_flags()
> have to do that?
Because we are explicitly deferring that work to the caller. That's a good
fraction of the point of the new interface.
>> + /* Handle clean RAM pages. */
>> + if (flags & TLB_NOTDIRTY) {
>> + notdirty_write(env_cpu(env), addr, 1, iotlbentry, retaddr);
>> + }
>> +
>> + /* Handle watchpoints. */
>> + if (flags & TLB_WATCHPOINT) {
>> + int wp_access = (access_type == MMU_DATA_STORE
>> + ? BP_MEM_WRITE : BP_MEM_READ);
>> + cpu_check_watchpoint(env_cpu(env), addr, size,
>> + iotlbentry->attrs, wp_access, retaddr);
>> + }
>
> The old code checked for watchpoints first, and then handled notdirty-writes,
> which seems like the more correct order. Why has the new
> version switched them around?
Not an intentional change, but I shouldn't think it would matter in the end.
> The probe_access_internal() doc comment doesn't say that it
> guarantees to set host to NULL for the TLB_MMIO/TLB_INVALID_MASK
> cases, but we implicitly rely on it here.
Eh? probe_access_internal doesn't have a doc comment. Call that a bug if you
like, but you seem to be talking about something else.
>> +void *probe_access(CPUArchState *env, target_ulong addr, int size,
>> + MMUAccessType access_type, int mmu_idx, uintptr_t
>> retaddr)
>> +{
>> + void *host;
>> +
>> + g_assert(-(addr | TARGET_PAGE_MASK) >= size);
>> + probe_access_flags(env, addr, access_type, mmu_idx, false, &host,
>> retaddr);
>> + return host;
>
> The old code returned NULL for a zero size; the new version does not.
Granted.
> The old code passed size into cc->tlb_fill; the new version does not.
> The old code passed size into page_check_range(); the new version does not.
This is the user-only version, and size is not used for tlb_fill. It is only
trivially used in page_change_range; we have just verified that addr+size does
not cross a page boundary.
r~
