[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 04/18] accel/tcg: Add probe_access_flags
From: |
Richard Henderson |
Subject: |
Re: [PATCH v3 04/18] accel/tcg: Add probe_access_flags |
Date: |
Mon, 27 Apr 2020 09:00:23 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 |
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~
- [PATCH v3 00/18] target/arm: sve load/store improvements, Richard Henderson, 2020/04/22
- [PATCH v3 01/18] exec: Add block comments for watchpoint routines, Richard Henderson, 2020/04/22
- [PATCH v3 02/18] exec: Fix cpu_watchpoint_address_matches address length, Richard Henderson, 2020/04/22
- [PATCH v3 03/18] accel/tcg: Add block comment for probe_access, Richard Henderson, 2020/04/22
- [PATCH v3 04/18] accel/tcg: Add probe_access_flags, Richard Henderson, 2020/04/22
- [PATCH v3 06/18] target/arm: Use cpu_*_data_ra for sve_ldst_tlb_fn, Richard Henderson, 2020/04/22
- [PATCH v3 07/18] target/arm: Drop manual handling of set/clear_helper_retaddr, Richard Henderson, 2020/04/22
- [PATCH v3 05/18] accel/tcg: Add endian-specific cpu_{ld, st}* operations, Richard Henderson, 2020/04/22
- [PATCH v3 09/18] target/arm: Adjust interface of sve_ld1_host_fn, Richard Henderson, 2020/04/22
- [PATCH v3 08/18] target/arm: Add sve infrastructure for page lookup, Richard Henderson, 2020/04/22
- [PATCH v3 10/18] target/arm: Use SVEContLdSt in sve_ld1_r, Richard Henderson, 2020/04/22
- [PATCH v3 11/18] target/arm: Handle watchpoints in sve_ld1_r, Richard Henderson, 2020/04/22