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(-)

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,
-- 
2.20.1

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