[Qemu-devel] [PATCH] target/ppc: fix mtmsr instruction for icount

[Qemu-devel] [PATCH] target/ppc: fix mtmsr instruction for icount

[Pavel Dovgalyuk]

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

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 */

Re: [Qemu-devel] [PATCH] target/ppc: fix mtmsr instruction for icount

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH] target/ppc: fix mtmsr instruction for icount

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2666 bytes --]

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 */
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] target/ppc: fix mtmsr instruction for icount

[Pavel Dovgalyuk]

> 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

Re: [Qemu-devel] [PATCH] target/ppc: fix mtmsr instruction for icount

['David Gibson']

[-- Attachment #1: Type: text/plain, Size: 1161 bytes --]

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] target/ppc: fix mtmsr instruction for icount

[Pavel Dovgalyuk]

> 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

Re: [Qemu-devel] [PATCH] target/ppc: fix mtmsr instruction for icount

[Mark Cave-Ayland]

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.