probe_access_flags() as it is today uses probe_access_full(), which in
turn uses probe_access_internal() with size = 0. probe_access_internal()
then uses the size to call the tlb_fill() callback for the given CPU.
This size param ('fault_size' as probe_access_internal() calls it) is
ignored by most existing .tlb_fill callback implementations, e.g.
arm_cpu_tlb_fill(), ppc_cpu_tlb_fill(), x86_cpu_tlb_fill() and
mips_cpu_tlb_fill() to name a few.
But RISC-V riscv_cpu_tlb_fill() actually uses it. The 'size' parameter
is used to check for PMP (Physical Memory Protection) access. This is
necessary because PMP does not make any guarantees about all the bytes
of the same page having the same permissions, i.e. the same page can
have different PMP properties, so we're forced to make sub-page range
checks. To allow RISC-V emulation to do a probe_acess_flags() that
covers PMP, we need to either add a 'size' param to the existing
probe_acess_flags() or create a new interface (e.g.
probe_access_range_flags).
There are quite a few probe_* APIs already, so let's add a 'size' param
to probe_access_flags() and re-use this API. This is done by open coding
what probe_access_full() does inside probe_acess_flags() and passing the
'size' param to probe_acess_internal(). Existing probe_access_flags()
callers use size = 0 to not change their current API usage. 'size' is
asserted to enforce single page access like probe_access() already does.
No behavioral changes intended.
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
---
accel/stubs/tcg-stub.c | 2 +-
accel/tcg/cputlb.c | 17 ++++++++++++++---
accel/tcg/user-exec.c | 5 +++--
include/exec/exec-all.h | 3 ++-
semihosting/uaccess.c | 2 +-
target/arm/ptw.c | 2 +-
target/arm/sve_helper.c | 2 +-
target/s390x/tcg/mem_helper.c | 6 +++---
8 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/accel/stubs/tcg-stub.c b/accel/stubs/tcg-stub.c
index c1b05767c0..96af23dc5d 100644
--- a/accel/stubs/tcg-stub.c
+++ b/accel/stubs/tcg-stub.c
@@ -25,7 +25,7 @@ void tcg_flush_jmp_cache(CPUState *cpu)
{
}
-int probe_access_flags(CPUArchState *env, target_ulong addr,
+int probe_access_flags(CPUArchState *env, target_ulong addr, int size,
MMUAccessType access_type, int mmu_idx,
bool nonfault, void **phost, uintptr_t retaddr)
{
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 4812d83961..fc27e34457 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -1606,14 +1606,25 @@ int probe_access_full(CPUArchState *env, target_ulong addr,
return flags;
}
-int probe_access_flags(CPUArchState *env, target_ulong addr,
+int probe_access_flags(CPUArchState *env, target_ulong addr, int size,
MMUAccessType access_type, int mmu_idx,
bool nonfault, void **phost, uintptr_t retaddr)
{
CPUTLBEntryFull *full;
+ int flags;
+
+ g_assert(-(addr | TARGET_PAGE_MASK) >= size);
+
+ flags = probe_access_internal(env, addr, size, access_type, mmu_idx,
+ nonfault, phost, &full, retaddr);
- return probe_access_full(env, addr, access_type, mmu_idx,
- nonfault, phost, &full, retaddr);
+ /* Handle clean RAM pages. */
+ if (unlikely(flags & TLB_NOTDIRTY)) {
+ notdirty_write(env_cpu(env), addr, 1, full, retaddr);
+ flags &= ~TLB_NOTDIRTY;
+ }
+
+ return flags;
}
void *probe_access(CPUArchState *env, target_ulong addr, int size,
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index ae67d84638..7b37fd229e 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -761,13 +761,14 @@ static int probe_access_internal(CPUArchState *env, target_ulong addr,
cpu_loop_exit_sigsegv(env_cpu(env), addr, access_type, maperr, ra);
}
-int probe_access_flags(CPUArchState *env, target_ulong addr,
+int probe_access_flags(CPUArchState *env, target_ulong addr, int size,
MMUAccessType access_type, int mmu_idx,
bool nonfault, void **phost, uintptr_t ra)
{
int flags;
- flags = probe_access_internal(env, addr, 0, access_type, nonfault, ra);
+ g_assert(-(addr | TARGET_PAGE_MASK) >= size);
+ flags = probe_access_internal(env, addr, size, access_type, nonfault, ra);
*phost = flags ? NULL : g2h(env_cpu(env), addr);
return flags;
}
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 54585a9954..b0d4916573 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -446,6 +446,7 @@ static inline void *probe_read(CPUArchState *env, target_ulong addr, int size,
* probe_access_flags:
* @env: CPUArchState
* @addr: guest virtual address to look up
+ * @size: size of the access
* @access_type: read, write or execute permission
* @mmu_idx: MMU index to use for lookup
* @nonfault: suppress the fault
@@ -460,7 +461,7 @@ static inline void *probe_read(CPUArchState *env, target_ulong addr, int size,
* Do handle clean pages, so exclude TLB_NOTDIRY from the returned flags.
* For simplicity, all "mmio-like" flags are folded to TLB_MMIO.
*/
-int probe_access_flags(CPUArchState *env, target_ulong addr,
+int probe_access_flags(CPUArchState *env, target_ulong addr, int size,
MMUAccessType access_type, int mmu_idx,
bool nonfault, void **phost, uintptr_t retaddr);
diff --git a/semihosting/uaccess.c b/semihosting/uaccess.c
index 8018828069..7505eb6d1b 100644
--- a/semihosting/uaccess.c
+++ b/semihosting/uaccess.c
@@ -37,7 +37,7 @@ ssize_t softmmu_strlen_user(CPUArchState *env, target_ulong addr)
/* Find the number of bytes remaining in the page. */
left_in_page = TARGET_PAGE_SIZE - (addr & ~TARGET_PAGE_MASK);
- flags = probe_access_flags(env, addr, MMU_DATA_LOAD,
+ flags = probe_access_flags(env, addr, 0, MMU_DATA_LOAD,
mmu_idx, true, &h, 0);
if (flags & TLB_INVALID_MASK) {
return -1;
diff --git a/target/arm/ptw.c b/target/arm/ptw.c
index 2b125fff44..1ecefb2027 100644
--- a/target/arm/ptw.c
+++ b/target/arm/ptw.c
@@ -407,7 +407,7 @@ static uint64_t arm_casq_ptw(CPUARMState *env, uint64_t old_val,
void *discard;
env->tlb_fi = fi;
- flags = probe_access_flags(env, ptw->out_virt, MMU_DATA_STORE,
+ flags = probe_access_flags(env, ptw->out_virt, 0, MMU_DATA_STORE,
arm_to_core_mmu_idx(ptw->in_ptw_idx),
true, &discard, 0);
env->tlb_fi = NULL;
diff --git a/target/arm/sve_helper.c b/target/arm/sve_helper.c
index 521fc9b969..51909c44ac 100644
--- a/target/arm/sve_helper.c
+++ b/target/arm/sve_helper.c
@@ -5352,7 +5352,7 @@ bool sve_probe_page(SVEHostPage *info, bool nofault, CPUARMState *env,
addr = useronly_clean_ptr(addr);
#ifdef CONFIG_USER_ONLY
- flags = probe_access_flags(env, addr, access_type, mmu_idx, nofault,
+ flags = probe_access_flags(env, addr, 0, access_type, mmu_idx, nofault,
&info->host, retaddr);
#else
CPUTLBEntryFull *full;
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index d6725fd18c..c9fd4142f1 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -143,14 +143,14 @@ static int s390_probe_access(CPUArchState *env, target_ulong addr, int size,
bool nonfault, void **phost, uintptr_t ra)
{
#if defined(CONFIG_USER_ONLY)
- return probe_access_flags(env, addr, access_type, mmu_idx,
+ return probe_access_flags(env, addr, 0, access_type, mmu_idx,
nonfault, phost, ra);
#else
int flags;
env->tlb_fill_exc = 0;
- flags = probe_access_flags(env, addr, access_type, mmu_idx, nonfault, phost,
- ra);
+ flags = probe_access_flags(env, addr, 0, access_type, mmu_idx,
+ nonfault, phost, ra);
if (env->tlb_fill_exc) {
return env->tlb_fill_exc;
}
--
2.39.2On 2/23/23 13:44, Daniel Henrique Barboza wrote:
> + if (unlikely(flags & TLB_NOTDIRTY)) {
> + notdirty_write(env_cpu(env), addr, 1, full, retaddr);
That '1' should be 'size'. Fixed locally.
r~
On 2/23/23 14:19, Richard Henderson wrote:
> On 2/23/23 13:44, Daniel Henrique Barboza wrote:
>> + if (unlikely(flags & TLB_NOTDIRTY)) {
>> + notdirty_write(env_cpu(env), addr, 1, full, retaddr);
>
> That '1' should be 'size'. Fixed locally.
Hmph, well, that matches the interface, but it's only used to figure out how many pages to
process, so any len that doesn't cross a page boundary (which we have checked) is equal
bar the tracepoint.
r~
On 2/23/23 21:23, Richard Henderson wrote:
> On 2/23/23 14:19, Richard Henderson wrote:
>> On 2/23/23 13:44, Daniel Henrique Barboza wrote:
>>> + if (unlikely(flags & TLB_NOTDIRTY)) {
>>> + notdirty_write(env_cpu(env), addr, 1, full, retaddr);
>>
>> That '1' should be 'size'. Fixed locally.
>
> Hmph, well, that matches the interface, but it's only used to figure out how many pages to process, so any len that doesn't cross a page boundary (which we have checked) is equal bar the tracepoint.
I didn't touch that '1' because I figured it might not be related with how
'size' was going to be handled and any value > 0 would suffice.
Thanks,
Daniel
>
>
> r~
>
On 2/23/23 13:44, Daniel Henrique Barboza wrote:
> probe_access_flags() as it is today uses probe_access_full(), which in
> turn uses probe_access_internal() with size = 0. probe_access_internal()
> then uses the size to call the tlb_fill() callback for the given CPU.
> This size param ('fault_size' as probe_access_internal() calls it) is
> ignored by most existing .tlb_fill callback implementations, e.g.
> arm_cpu_tlb_fill(), ppc_cpu_tlb_fill(), x86_cpu_tlb_fill() and
> mips_cpu_tlb_fill() to name a few.
>
> But RISC-V riscv_cpu_tlb_fill() actually uses it. The 'size' parameter
> is used to check for PMP (Physical Memory Protection) access. This is
> necessary because PMP does not make any guarantees about all the bytes
> of the same page having the same permissions, i.e. the same page can
> have different PMP properties, so we're forced to make sub-page range
> checks. To allow RISC-V emulation to do a probe_acess_flags() that
> covers PMP, we need to either add a 'size' param to the existing
> probe_acess_flags() or create a new interface (e.g.
> probe_access_range_flags).
>
> There are quite a few probe_* APIs already, so let's add a 'size' param
> to probe_access_flags() and re-use this API. This is done by open coding
> what probe_access_full() does inside probe_acess_flags() and passing the
> 'size' param to probe_acess_internal(). Existing probe_access_flags()
> callers use size = 0 to not change their current API usage. 'size' is
> asserted to enforce single page access like probe_access() already does.
>
> No behavioral changes intended.
>
> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com>
> ---
> accel/stubs/tcg-stub.c | 2 +-
> accel/tcg/cputlb.c | 17 ++++++++++++++---
> accel/tcg/user-exec.c | 5 +++--
> include/exec/exec-all.h | 3 ++-
> semihosting/uaccess.c | 2 +-
> target/arm/ptw.c | 2 +-
> target/arm/sve_helper.c | 2 +-
> target/s390x/tcg/mem_helper.c | 6 +++---
> 8 files changed, 26 insertions(+), 13 deletions(-)
Queueing to tcg-next.
r~
On Thu, 23 Feb 2023 16:10:59 PST (-0800), Richard Henderson wrote:
> On 2/23/23 13:44, Daniel Henrique Barboza wrote:
>> probe_access_flags() as it is today uses probe_access_full(), which in
>> turn uses probe_access_internal() with size = 0. probe_access_internal()
>> then uses the size to call the tlb_fill() callback for the given CPU.
>> This size param ('fault_size' as probe_access_internal() calls it) is
>> ignored by most existing .tlb_fill callback implementations, e.g.
>> arm_cpu_tlb_fill(), ppc_cpu_tlb_fill(), x86_cpu_tlb_fill() and
>> mips_cpu_tlb_fill() to name a few.
>>
>> But RISC-V riscv_cpu_tlb_fill() actually uses it. The 'size' parameter
>> is used to check for PMP (Physical Memory Protection) access. This is
>> necessary because PMP does not make any guarantees about all the bytes
>> of the same page having the same permissions, i.e. the same page can
>> have different PMP properties, so we're forced to make sub-page range
>> checks. To allow RISC-V emulation to do a probe_acess_flags() that
>> covers PMP, we need to either add a 'size' param to the existing
>> probe_acess_flags() or create a new interface (e.g.
>> probe_access_range_flags).
>>
>> There are quite a few probe_* APIs already, so let's add a 'size' param
>> to probe_access_flags() and re-use this API. This is done by open coding
>> what probe_access_full() does inside probe_acess_flags() and passing the
>> 'size' param to probe_acess_internal(). Existing probe_access_flags()
>> callers use size = 0 to not change their current API usage. 'size' is
>> asserted to enforce single page access like probe_access() already does.
>>
>> No behavioral changes intended.
>>
>> Signed-off-by: Daniel Henrique Barboza<dbarboza@ventanamicro.com>
>> ---
>> accel/stubs/tcg-stub.c | 2 +-
>> accel/tcg/cputlb.c | 17 ++++++++++++++---
>> accel/tcg/user-exec.c | 5 +++--
>> include/exec/exec-all.h | 3 ++-
>> semihosting/uaccess.c | 2 +-
>> target/arm/ptw.c | 2 +-
>> target/arm/sve_helper.c | 2 +-
>> target/s390x/tcg/mem_helper.c | 6 +++---
>> 8 files changed, 26 insertions(+), 13 deletions(-)
>
> Queueing to tcg-next.
Unless I'm missing something, that's not in Peter's tree yet? I Ack'd
the cover, it's fine with me if you want to take these via the TCG tree.
Doing some sort of shared tag for the first one works for me too, I've
also got some other stuff in the RISC-V queue.
Thanks!
On 2/23/23 13:44, Daniel Henrique Barboza wrote:
> probe_access_flags() as it is today uses probe_access_full(), which in
> turn uses probe_access_internal() with size = 0. probe_access_internal()
> then uses the size to call the tlb_fill() callback for the given CPU.
> This size param ('fault_size' as probe_access_internal() calls it) is
> ignored by most existing .tlb_fill callback implementations, e.g.
> arm_cpu_tlb_fill(), ppc_cpu_tlb_fill(), x86_cpu_tlb_fill() and
> mips_cpu_tlb_fill() to name a few.
>
> But RISC-V riscv_cpu_tlb_fill() actually uses it. The 'size' parameter
> is used to check for PMP (Physical Memory Protection) access. This is
> necessary because PMP does not make any guarantees about all the bytes
> of the same page having the same permissions, i.e. the same page can
> have different PMP properties, so we're forced to make sub-page range
> checks. To allow RISC-V emulation to do a probe_acess_flags() that
> covers PMP, we need to either add a 'size' param to the existing
> probe_acess_flags() or create a new interface (e.g.
> probe_access_range_flags).
>
> There are quite a few probe_* APIs already, so let's add a 'size' param
> to probe_access_flags() and re-use this API. This is done by open coding
> what probe_access_full() does inside probe_acess_flags() and passing the
> 'size' param to probe_acess_internal(). Existing probe_access_flags()
> callers use size = 0 to not change their current API usage. 'size' is
> asserted to enforce single page access like probe_access() already does.
>
> No behavioral changes intended.
>
> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> ---
> accel/stubs/tcg-stub.c | 2 +-
> accel/tcg/cputlb.c | 17 ++++++++++++++---
> accel/tcg/user-exec.c | 5 +++--
> include/exec/exec-all.h | 3 ++-
> semihosting/uaccess.c | 2 +-
> target/arm/ptw.c | 2 +-
> target/arm/sve_helper.c | 2 +-
> target/s390x/tcg/mem_helper.c | 6 +++---
> 8 files changed, 26 insertions(+), 13 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
I'll change probe_access_full as well, just to keep them in sync.
> +int probe_access_flags(CPUArchState *env, target_ulong addr, int size,
> MMUAccessType access_type, int mmu_idx,
> bool nonfault, void **phost, uintptr_t retaddr)
> {
> CPUTLBEntryFull *full;
> + int flags;
> +
> + g_assert(-(addr | TARGET_PAGE_MASK) >= size);
> +
> + flags = probe_access_internal(env, addr, size, access_type, mmu_idx,
> + nonfault, phost, &full, retaddr);
>
> - return probe_access_full(env, addr, access_type, mmu_idx,
> - nonfault, phost, &full, retaddr);
> + /* Handle clean RAM pages. */
> + if (unlikely(flags & TLB_NOTDIRTY)) {
> + notdirty_write(env_cpu(env), addr, 1, full, retaddr);
> + flags &= ~TLB_NOTDIRTY;
> + }
> +
> + return flags;
But bypassing probe_access_full here is fine.
r~
© 2016 - 2025 Red Hat, Inc.