| Message ID | 20190308185932.4954-1-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/hppa: Check for page crossings in use_goto_tb | expand |
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
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~
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,
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(-) -- 2.17.2