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

Richard Henderson posted 10 patches 4 years, 10 months ago
[PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG
Posted by Richard Henderson 4 years, 10 months ago
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(-)

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()) {
-- 
2.25.1


Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG
Posted by David Gibson 4 years, 10 months ago
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
Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG
Posted by David Gibson 4 years, 10 months ago
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
Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG
Posted by Richard Henderson 4 years, 10 months ago
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~

Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG
Posted by Richard Henderson 4 years, 10 months ago
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~