| Message ID | 20200508154359.7494-5-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: sve load/store improvements | expand |
On Fri, 8 May 2020 at 16:44, Richard Henderson <richard.henderson@linaro.org> wrote: > > We have validated that addr+size does not cross a page boundary. > Therefore we need to validate exactly one page. We can achieve > that passing any value 1 <= x <= size to page_check_range. > > Passing 1 will simplify the next patch. It's not clear to me how it simplifies the next patch, though -- we have the size right there in the new function which calls page_check_range(), don't we? So I still don't understand why we're using '1' -- it isn't allowing us to avoid passing the size into probe_access_internal(), because we need to pass it anyway. We've gone round this multiple times now so I feel like I must be missing something here. thanks -- PMM
On 5/8/20 9:13 AM, Peter Maydell wrote: > On Fri, 8 May 2020 at 16:44, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> We have validated that addr+size does not cross a page boundary. >> Therefore we need to validate exactly one page. We can achieve >> that passing any value 1 <= x <= size to page_check_range. >> >> Passing 1 will simplify the next patch. > > It's not clear to me how it simplifies the next patch, though -- > we have the size right there in the new function which > calls page_check_range(), don't we? So I still don't > understand why we're using '1' -- it isn't allowing > us to avoid passing the size into probe_access_internal(), > because we need to pass it anyway. > > We've gone round this multiple times now so I feel like > I must be missing something here. While probe_access() has a size parameter, probe_access_flags() does not. For probe_access_internal(), I currently have a "fault_size" parameter that gets passed to tlb_fill, which is "size" for probe_access() and 0 for probe_access_flags(). I *could* add another "check_size" parameter to probe_access_internal, to be passed on to page_check_range(). It would be "size" for probe_access() and 1 for probe_access_flags(). But what's the point? Always passing 1 to page_check_range() has the same effect. I feel like I'm missing something with your objection. r~
On Fri, 8 May 2020 at 17:57, Richard Henderson <richard.henderson@linaro.org> wrote: > On 5/8/20 9:13 AM, Peter Maydell wrote: > > We've gone round this multiple times now so I feel like > > I must be missing something here. > > While probe_access() has a size parameter, probe_access_flags() does not. > > For probe_access_internal(), I currently have a "fault_size" parameter that > gets passed to tlb_fill, which is "size" for probe_access() and 0 for > probe_access_flags(). > > I *could* add another "check_size" parameter to probe_access_internal, to be > passed on to page_check_range(). It would be "size" for probe_access() and 1 > for probe_access_flags(). But what's the point? Always passing 1 to > page_check_range() has the same effect. > > I feel like I'm missing something with your objection. The thing I was missing was that probe_access_flags() doesn't have a size to pass usefully to probe_access_internal() and so the size is zero in that case, but that tlb_fill() and probe_check_range() want different values for the "just tell me about this address" case. thanks -- PMM
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index 4be78eb9b3..03538e2a38 100644 --- a/accel/tcg/user-exec.c +++ b/accel/tcg/user-exec.c @@ -211,7 +211,7 @@ void *probe_access(CPUArchState *env, target_ulong addr, int size, g_assert_not_reached(); } - if (!guest_addr_valid(addr) || page_check_range(addr, size, flags) < 0) { + if (!guest_addr_valid(addr) || page_check_range(addr, 1, flags) < 0) { CPUState *cpu = env_cpu(env); CPUClass *cc = CPU_GET_CLASS(cpu); cc->tlb_fill(cpu, addr, size, access_type, MMU_USER_IDX, false,
We have validated that addr+size does not cross a page boundary. Therefore we need to validate exactly one page. We can achieve that passing any value 1 <= x <= size to page_check_range. Passing 1 will simplify the next patch. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- accel/tcg/user-exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.20.1