Re: [PATCH v3 04/18] accel/tcg: Add probe_access_flags

[Richard Henderson]

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~