| Message ID | 20181030093031.10131.17221.stgit@pasha-VirtualBox |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: fix mtmsr instruction for icount | expand |
On 10/30/18 9:30 AM, Pavel Dovgalyuk wrote: > This patch fixes processing of mtmsr instructions in icount mode. > In this mode writing to interrupt/peripheral state is controlled > by can_do_io flag. This flag must be set explicitly before helper > function invocation. > > Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > --- > target/ppc/translate.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Tue, Oct 30, 2018 at 12:30:31PM +0300, Pavel Dovgalyuk wrote: > This patch fixes processing of mtmsr instructions in icount mode. > In this mode writing to interrupt/peripheral state is controlled > by can_do_io flag. This flag must be set explicitly before helper > function invocation. > > Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> Applied to ppc-for-3.1, thanks. > --- > target/ppc/translate.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 4e59dd5..987ce6e 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -4257,11 +4257,17 @@ static void gen_mtmsrd(DisasContext *ctx) > * if we enter power saving mode, we will exit the loop > * directly from ppc_store_msr > */ > + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { > + gen_io_start(); > + } > gen_update_nip(ctx, ctx->base.pc_next); > gen_helper_store_msr(cpu_env, cpu_gpr[rS(ctx->opcode)]); > /* Must stop the translation as machine state (may have) changed */ > /* Note that mtmsr is not always defined as context-synchronizing */ > gen_stop_exception(ctx); > + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { > + gen_io_end(); > + } > } > #endif /* !defined(CONFIG_USER_ONLY) */ > } > @@ -4286,6 +4292,9 @@ static void gen_mtmsr(DisasContext *ctx) > * if we enter power saving mode, we will exit the loop > * directly from ppc_store_msr > */ > + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { > + gen_io_start(); > + } > gen_update_nip(ctx, ctx->base.pc_next); > #if defined(TARGET_PPC64) > tcg_gen_deposit_tl(msr, cpu_msr, cpu_gpr[rS(ctx->opcode)], 0, 32); > @@ -4293,6 +4302,9 @@ static void gen_mtmsr(DisasContext *ctx) > tcg_gen_mov_tl(msr, cpu_gpr[rS(ctx->opcode)]); > #endif > gen_helper_store_msr(cpu_env, msr); > + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { > + gen_io_end(); > + } > tcg_temp_free(msr); > /* Must stop the translation as machine state (may have) changed */ > /* Note that mtmsr is not always defined as context-synchronizing */ >
> From: David Gibson [mailto:david@gibson.dropbear.id.au] > On Tue, Oct 30, 2018 at 12:30:31PM +0300, Pavel Dovgalyuk wrote: > > This patch fixes processing of mtmsr instructions in icount mode. > > In this mode writing to interrupt/peripheral state is controlled > > by can_do_io flag. This flag must be set explicitly before helper > > function invocation. > > > > Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru> > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > > Applied to ppc-for-3.1, thanks. Thanks. What about this one https://patchew.org/QEMU/20181030122134.11055.15711.stgit@pasha-VirtualBox/ There is a mess with the subject, but the code is ok :) Pavel Dovgalyuk
On Tue, Nov 06, 2018 at 09:10:45AM +0300, Pavel Dovgalyuk wrote: > > From: David Gibson [mailto:david@gibson.dropbear.id.au] > > On Tue, Oct 30, 2018 at 12:30:31PM +0300, Pavel Dovgalyuk wrote: > > > This patch fixes processing of mtmsr instructions in icount mode. > > > In this mode writing to interrupt/peripheral state is controlled > > > by can_do_io flag. This flag must be set explicitly before helper > > > function invocation. > > > > > > Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru> > > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > > > > Applied to ppc-for-3.1, thanks. > > Thanks. What about this one > https://patchew.org/QEMU/20181030122134.11055.15711.stgit@pasha-VirtualBox/ > There is a mess with the subject, but the code is ok :) I've been procrastinating on that because I don't understand icount well enough to review it easily, and no-one has replied with Reviewed-by or Tested-by.
> From: Richard Henderson [mailto:richard.henderson@linaro.org] > On 10/30/18 9:30 AM, Pavel Dovgalyuk wrote: > > This patch fixes processing of mtmsr instructions in icount mode. > > In this mode writing to interrupt/peripheral state is controlled > > by can_do_io flag. This flag must be set explicitly before helper > > function invocation. > > > > Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru> > > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> > > --- > > target/ppc/translate.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Richard, can you check the another similar patch? https://patchew.org/QEMU/20181030122134.11055.15711.stgit@pasha-VirtualBox/ Pavel Dovgalyuk
On 06/11/2018 12:20, 'David Gibson' wrote: > On Tue, Nov 06, 2018 at 09:10:45AM +0300, Pavel Dovgalyuk wrote: >>> From: David Gibson [mailto:david@gibson.dropbear.id.au] >>> On Tue, Oct 30, 2018 at 12:30:31PM +0300, Pavel Dovgalyuk wrote: >>>> This patch fixes processing of mtmsr instructions in icount mode. >>>> In this mode writing to interrupt/peripheral state is controlled >>>> by can_do_io flag. This flag must be set explicitly before helper >>>> function invocation. >>>> >>>> Signed-off-by: Maria Klimushenkova <maria.klimushenkova@ispras.ru> >>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru> >>> >>> Applied to ppc-for-3.1, thanks. >> >> Thanks. What about this one >> https://patchew.org/QEMU/20181030122134.11055.15711.stgit@pasha-VirtualBox/ >> There is a mess with the subject, but the code is ok :) > > I've been procrastinating on that because I don't understand icount > well enough to review it easily, and no-one has replied with > Reviewed-by or Tested-by. I've just sent a Tested-by tag for this - with icount enabled, QEMU will assert if an instruction that alters interrupt state doesn't set can_do_io beforehand. With this patch on top of ppc-for-3.1 then I can boot my OpenBIOS test images without QEMU asserting in icount mode. Since it should have no effect without icount enabled, it should be safe. It might also be worth changing the title of the patch to "target/ppc: fix rfid instruction for icount" so it matches its companion patch. ATB, Mark.
diff --git a/target/ppc/translate.c b/target/ppc/translate.c index 4e59dd5..987ce6e 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -4257,11 +4257,17 @@ static void gen_mtmsrd(DisasContext *ctx) * if we enter power saving mode, we will exit the loop * directly from ppc_store_msr */ + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { + gen_io_start(); + } gen_update_nip(ctx, ctx->base.pc_next); gen_helper_store_msr(cpu_env, cpu_gpr[rS(ctx->opcode)]); /* Must stop the translation as machine state (may have) changed */ /* Note that mtmsr is not always defined as context-synchronizing */ gen_stop_exception(ctx); + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { + gen_io_end(); + } } #endif /* !defined(CONFIG_USER_ONLY) */ } @@ -4286,6 +4292,9 @@ static void gen_mtmsr(DisasContext *ctx) * if we enter power saving mode, we will exit the loop * directly from ppc_store_msr */ + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { + gen_io_start(); + } gen_update_nip(ctx, ctx->base.pc_next); #if defined(TARGET_PPC64) tcg_gen_deposit_tl(msr, cpu_msr, cpu_gpr[rS(ctx->opcode)], 0, 32); @@ -4293,6 +4302,9 @@ static void gen_mtmsr(DisasContext *ctx) tcg_gen_mov_tl(msr, cpu_gpr[rS(ctx->opcode)]); #endif gen_helper_store_msr(cpu_env, msr); + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) { + gen_io_end(); + } tcg_temp_free(msr); /* Must stop the translation as machine state (may have) changed */ /* Note that mtmsr is not always defined as context-synchronizing */