diff mbox series

[v5,10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG

Message ID 20210323184340.619757-11-richard.henderson@linaro.org
State Superseded
Headers show
Series target/ppc: Fix truncation of env->hflags | expand

Commit Message

Richard Henderson March 23, 2021, 6:43 p.m. UTC
Verify that hflags was updated correctly whenever we change
cpu state that is used by hflags.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/ppc/cpu.h         |  5 +++++
 target/ppc/helper_regs.c | 29 +++++++++++++++++++++++++++--
 2 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.25.1

Comments

David Gibson March 24, 2021, 12:12 a.m. UTC | #1
On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:
> Verify that hflags was updated correctly whenever we change

> cpu state that is used by hflags.

> 

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


Applied to ppc-for-6.0, thanks.

> ---

>  target/ppc/cpu.h         |  5 +++++

>  target/ppc/helper_regs.c | 29 +++++++++++++++++++++++++++--

>  2 files changed, 32 insertions(+), 2 deletions(-)

> 

> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h

> index 3d021f61f3..69fc9a2831 100644

> --- a/target/ppc/cpu.h

> +++ b/target/ppc/cpu.h

> @@ -2425,6 +2425,10 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer);

>   */

>  #define is_book3s_arch2x(ctx) (!!((ctx)->insns_flags & PPC_SEGMENT_64B))

>  

> +#ifdef CONFIG_DEBUG_TCG

> +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,

> +                          target_ulong *cs_base, uint32_t *flags);

> +#else

>  static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,

>                                          target_ulong *cs_base, uint32_t *flags)

>  {

> @@ -2432,6 +2436,7 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,

>      *cs_base = 0;

>      *flags = env->hflags;

>  }

> +#endif

>  

>  void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);

>  void QEMU_NORETURN raise_exception_ra(CPUPPCState *env, uint32_t exception,

> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c

> index 5411a67e9a..3723872aa6 100644

> --- a/target/ppc/helper_regs.c

> +++ b/target/ppc/helper_regs.c

> @@ -43,7 +43,7 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)

>      env->tgpr[3] = tmp;

>  }

>  

> -void hreg_compute_hflags(CPUPPCState *env)

> +static uint32_t hreg_compute_hflags_value(CPUPPCState *env)

>  {

>      target_ulong msr = env->msr;

>      uint32_t ppc_flags = env->flags;

> @@ -155,9 +155,34 @@ void hreg_compute_hflags(CPUPPCState *env)

>      hflags |= dmmu_idx << HFLAGS_DMMU_IDX;

>  #endif

>  

> -    env->hflags = hflags | (msr & msr_mask);

> +    return hflags | (msr & msr_mask);

>  }

>  

> +void hreg_compute_hflags(CPUPPCState *env)

> +{

> +    env->hflags = hreg_compute_hflags_value(env);

> +}

> +

> +#ifdef CONFIG_DEBUG_TCG

> +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,

> +                          target_ulong *cs_base, uint32_t *flags)

> +{

> +    uint32_t hflags_current = env->hflags;

> +    uint32_t hflags_rebuilt;

> +

> +    *pc = env->nip;

> +    *cs_base = 0;

> +    *flags = hflags_current;

> +

> +    hflags_rebuilt = hreg_compute_hflags_value(env);

> +    if (unlikely(hflags_current != hflags_rebuilt)) {

> +        cpu_abort(env_cpu(env),

> +                  "TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",

> +                  hflags_current, hflags_rebuilt);

> +    }

> +}

> +#endif

> +

>  void cpu_interrupt_exittb(CPUState *cs)

>  {

>      if (!kvm_enabled()) {


-- 
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
David Gibson March 25, 2021, 8:47 a.m. UTC | #2
On Wed, Mar 24, 2021 at 11:12:20AM +1100, David Gibson wrote:
> On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:

> > Verify that hflags was updated correctly whenever we change

> > cpu state that is used by hflags.

> > 

> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> 

> Applied to ppc-for-6.0, thanks.


Alas, this appears to cause a failure in the 'build-user' test on
gitlab CI :(.  I haven't managed to reproduce it locally, so I'm not
sure what's going on.

> 

> > ---

> >  target/ppc/cpu.h         |  5 +++++

> >  target/ppc/helper_regs.c | 29 +++++++++++++++++++++++++++--

> >  2 files changed, 32 insertions(+), 2 deletions(-)

> > 

> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h

> > index 3d021f61f3..69fc9a2831 100644

> > --- a/target/ppc/cpu.h

> > +++ b/target/ppc/cpu.h

> > @@ -2425,6 +2425,10 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer);

> >   */

> >  #define is_book3s_arch2x(ctx) (!!((ctx)->insns_flags & PPC_SEGMENT_64B))

> >  

> > +#ifdef CONFIG_DEBUG_TCG

> > +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,

> > +                          target_ulong *cs_base, uint32_t *flags);

> > +#else

> >  static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,

> >                                          target_ulong *cs_base, uint32_t *flags)

> >  {

> > @@ -2432,6 +2436,7 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,

> >      *cs_base = 0;

> >      *flags = env->hflags;

> >  }

> > +#endif

> >  

> >  void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);

> >  void QEMU_NORETURN raise_exception_ra(CPUPPCState *env, uint32_t exception,

> > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c

> > index 5411a67e9a..3723872aa6 100644

> > --- a/target/ppc/helper_regs.c

> > +++ b/target/ppc/helper_regs.c

> > @@ -43,7 +43,7 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)

> >      env->tgpr[3] = tmp;

> >  }

> >  

> > -void hreg_compute_hflags(CPUPPCState *env)

> > +static uint32_t hreg_compute_hflags_value(CPUPPCState *env)

> >  {

> >      target_ulong msr = env->msr;

> >      uint32_t ppc_flags = env->flags;

> > @@ -155,9 +155,34 @@ void hreg_compute_hflags(CPUPPCState *env)

> >      hflags |= dmmu_idx << HFLAGS_DMMU_IDX;

> >  #endif

> >  

> > -    env->hflags = hflags | (msr & msr_mask);

> > +    return hflags | (msr & msr_mask);

> >  }

> >  

> > +void hreg_compute_hflags(CPUPPCState *env)

> > +{

> > +    env->hflags = hreg_compute_hflags_value(env);

> > +}

> > +

> > +#ifdef CONFIG_DEBUG_TCG

> > +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,

> > +                          target_ulong *cs_base, uint32_t *flags)

> > +{

> > +    uint32_t hflags_current = env->hflags;

> > +    uint32_t hflags_rebuilt;

> > +

> > +    *pc = env->nip;

> > +    *cs_base = 0;

> > +    *flags = hflags_current;

> > +

> > +    hflags_rebuilt = hreg_compute_hflags_value(env);

> > +    if (unlikely(hflags_current != hflags_rebuilt)) {

> > +        cpu_abort(env_cpu(env),

> > +                  "TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",

> > +                  hflags_current, hflags_rebuilt);

> > +    }

> > +}

> > +#endif

> > +

> >  void cpu_interrupt_exittb(CPUState *cs)

> >  {

> >      if (!kvm_enabled()) {

> 




-- 
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
Richard Henderson March 26, 2021, 12:41 p.m. UTC | #3
On 3/25/21 2:47 AM, David Gibson wrote:
> On Wed, Mar 24, 2021 at 11:12:20AM +1100, David Gibson wrote:

>> On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:

>>> Verify that hflags was updated correctly whenever we change

>>> cpu state that is used by hflags.

>>>

>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>>

>> Applied to ppc-for-6.0, thanks.

> 

> Alas, this appears to cause a failure in the 'build-user' test on

> gitlab CI :(.  I haven't managed to reproduce it locally, so I'm not

> sure what's going on.


I guess you mean e.g.

https://gitlab.com/dgibson/qemu/-/jobs/1126364147

?  I'll have a look.


r~
Richard Henderson March 27, 2021, 12:46 p.m. UTC | #4
On 3/26/21 6:41 AM, Richard Henderson wrote:
> On 3/25/21 2:47 AM, David Gibson wrote:

>> On Wed, Mar 24, 2021 at 11:12:20AM +1100, David Gibson wrote:

>>> On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:

>>>> Verify that hflags was updated correctly whenever we change

>>>> cpu state that is used by hflags.

>>>>

>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

>>>

>>> Applied to ppc-for-6.0, thanks.

>>

>> Alas, this appears to cause a failure in the 'build-user' test on

>> gitlab CI :(.  I haven't managed to reproduce it locally, so I'm not

>> sure what's going on.

> 

> I guess you mean e.g.

> 

> https://gitlab.com/dgibson/qemu/-/jobs/1126364147

> 

> ?  I'll have a look.


I haven't been able to repo locally, or on gitlab:

https://gitlab.com/rth7680/qemu/-/pipelines/277073704

Have you tried simply re-running that job?


r~
David Gibson March 28, 2021, 1:09 p.m. UTC | #5
On Sat, Mar 27, 2021 at 06:46:15AM -0600, Richard Henderson wrote:
> On 3/26/21 6:41 AM, Richard Henderson wrote:

> > On 3/25/21 2:47 AM, David Gibson wrote:

> > > On Wed, Mar 24, 2021 at 11:12:20AM +1100, David Gibson wrote:

> > > > On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:

> > > > > Verify that hflags was updated correctly whenever we change

> > > > > cpu state that is used by hflags.

> > > > > 

> > > > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> > > > 

> > > > Applied to ppc-for-6.0, thanks.

> > > 

> > > Alas, this appears to cause a failure in the 'build-user' test on

> > > gitlab CI :(.  I haven't managed to reproduce it locally, so I'm not

> > > sure what's going on.

> > 

> > I guess you mean e.g.

> > 

> > https://gitlab.com/dgibson/qemu/-/jobs/1126364147


Yes, sorry I meant to give you a link.

> > 

> > ?  I'll have a look.

> 

> I haven't been able to repo locally, or on gitlab:

> 

> https://gitlab.com/rth7680/qemu/-/pipelines/277073704


Huh..

> Have you tried simply re-running that job?


Several times :/

-- 
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
diff mbox series

Patch

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 3d021f61f3..69fc9a2831 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2425,6 +2425,10 @@  void cpu_write_xer(CPUPPCState *env, target_ulong xer);
  */
 #define is_book3s_arch2x(ctx) (!!((ctx)->insns_flags & PPC_SEGMENT_64B))
 
+#ifdef CONFIG_DEBUG_TCG
+void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
+                          target_ulong *cs_base, uint32_t *flags);
+#else
 static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
                                         target_ulong *cs_base, uint32_t *flags)
 {
@@ -2432,6 +2436,7 @@  static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
     *cs_base = 0;
     *flags = env->hflags;
 }
+#endif
 
 void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);
 void QEMU_NORETURN raise_exception_ra(CPUPPCState *env, uint32_t exception,
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 5411a67e9a..3723872aa6 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -43,7 +43,7 @@  void hreg_swap_gpr_tgpr(CPUPPCState *env)
     env->tgpr[3] = tmp;
 }
 
-void hreg_compute_hflags(CPUPPCState *env)
+static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
 {
     target_ulong msr = env->msr;
     uint32_t ppc_flags = env->flags;
@@ -155,9 +155,34 @@  void hreg_compute_hflags(CPUPPCState *env)
     hflags |= dmmu_idx << HFLAGS_DMMU_IDX;
 #endif
 
-    env->hflags = hflags | (msr & msr_mask);
+    return hflags | (msr & msr_mask);
 }
 
+void hreg_compute_hflags(CPUPPCState *env)
+{
+    env->hflags = hreg_compute_hflags_value(env);
+}
+
+#ifdef CONFIG_DEBUG_TCG
+void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
+                          target_ulong *cs_base, uint32_t *flags)
+{
+    uint32_t hflags_current = env->hflags;
+    uint32_t hflags_rebuilt;
+
+    *pc = env->nip;
+    *cs_base = 0;
+    *flags = hflags_current;
+
+    hflags_rebuilt = hreg_compute_hflags_value(env);
+    if (unlikely(hflags_current != hflags_rebuilt)) {
+        cpu_abort(env_cpu(env),
+                  "TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",
+                  hflags_current, hflags_rebuilt);
+    }
+}
+#endif
+
 void cpu_interrupt_exittb(CPUState *cs)
 {
     if (!kvm_enabled()) {