[Qemu-devel] [PATCH] target/hppa: Check for page crossings in use_goto_tb

[Qemu-devel] [PATCH] target/hppa: Check for page crossings in use_goto_tb

[Richard Henderson]

We got away with eliding this check when target/hppa was user-only,
but missed adding this check when adding system support.

Fixes an early crash in the HP-UX 11 installer.

Reported-by: Sven Schnelle <svens@stackframe.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/hppa/translate.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index dc5636fe94..6c815e05c2 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -816,12 +816,10 @@ static bool gen_illegal(DisasContext *ctx)
 
 static bool use_goto_tb(DisasContext *ctx, target_ureg dest)
 {
-    /* Suppress goto_tb in the case of single-steping and IO.  */
-    if ((tb_cflags(ctx->base.tb) & CF_LAST_IO)
-        || ctx->base.singlestep_enabled) {
-        return false;
-    }
-    return true;
+    /* Suppress goto_tb for page crossing, IO, or single-steping.  */
+    return !(((ctx->base.pc_first ^ dest) & TARGET_PAGE_MASK)
+             || (tb_cflags(ctx->base.tb) & CF_LAST_IO)
+             || ctx->base.singlestep_enabled);
 }
 
 /* If the next insn is to be nullified, and it's on the same page,
-- 
2.17.2

Re: [Qemu-devel] [PATCH] target/hppa: Check for page crossings in use_goto_tb

[Peter Maydell]

On Fri, 8 Mar 2019 at 19:00, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We got away with eliding this check when target/hppa was user-only,
> but missed adding this check when adding system support.
>
> Fixes an early crash in the HP-UX 11 installer.
>
> Reported-by: Sven Schnelle <svens@stackframe.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/hppa/translate.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index dc5636fe94..6c815e05c2 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -816,12 +816,10 @@ static bool gen_illegal(DisasContext *ctx)
>
>  static bool use_goto_tb(DisasContext *ctx, target_ureg dest)
>  {
> -    /* Suppress goto_tb in the case of single-steping and IO.  */
> -    if ((tb_cflags(ctx->base.tb) & CF_LAST_IO)
> -        || ctx->base.singlestep_enabled) {
> -        return false;
> -    }
> -    return true;
> +    /* Suppress goto_tb for page crossing, IO, or single-steping.  */

"stepping"

> +    return !(((ctx->base.pc_first ^ dest) & TARGET_PAGE_MASK)
> +             || (tb_cflags(ctx->base.tb) & CF_LAST_IO)
> +             || ctx->base.singlestep_enabled);
>  }

I note that (a) this isn't the way every other port phrases
the "same page" check -- they generally use something like
 (ctx->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)
and (b) the other ports generally keep that check inside an
ifndef CONFIG_USER_ONLY.

>
>  /* If the next insn is to be nullified, and it's on the same page,
> --
> 2.17.2

thanks
-- PMM

Re: [Qemu-devel] [PATCH] target/hppa: Check for page crossings in use_goto_tb

[Richard Henderson]

On 3/8/19 11:04 AM, Peter Maydell wrote:
> On Fri, 8 Mar 2019 at 19:00, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> We got away with eliding this check when target/hppa was user-only,
>> but missed adding this check when adding system support.
>>
>> Fixes an early crash in the HP-UX 11 installer.
>>
>> Reported-by: Sven Schnelle <svens@stackframe.org>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/hppa/translate.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
>> index dc5636fe94..6c815e05c2 100644
>> --- a/target/hppa/translate.c
>> +++ b/target/hppa/translate.c
>> @@ -816,12 +816,10 @@ static bool gen_illegal(DisasContext *ctx)
>>
>>  static bool use_goto_tb(DisasContext *ctx, target_ureg dest)
>>  {
>> -    /* Suppress goto_tb in the case of single-steping and IO.  */
>> -    if ((tb_cflags(ctx->base.tb) & CF_LAST_IO)
>> -        || ctx->base.singlestep_enabled) {
>> -        return false;
>> -    }
>> -    return true;
>> +    /* Suppress goto_tb for page crossing, IO, or single-steping.  */
> 
> "stepping"

Oops.

>> +    return !(((ctx->base.pc_first ^ dest) & TARGET_PAGE_MASK)
>> +             || (tb_cflags(ctx->base.tb) & CF_LAST_IO)
>> +             || ctx->base.singlestep_enabled);
>>  }
> 
> I note that (a) this isn't the way every other port phrases
> the "same page" check -- they generally use something like
>  (ctx->base.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)

This should be the same result.

ctx->base.pc_first was initialized from tb->pc.  I find the xor expression more
compact and usually fits on a line, where repeating TARGET_PAGE_MASK doesn't.

> and (b) the other ports generally keep that check inside an
> ifndef CONFIG_USER_ONLY.

I've wondered about that.  It certainly works for normal executables, but I
wonder if there are jit-like cases that fail when eliding that check.

Here, I think I was just a bit lazy.

Thoughts?


r~