diff mbox series

[v5,02/10] target/ppc: Disconnect hflags from MSR

Message ID 20210323184340.619757-3-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
Copying flags directly from msr has drawbacks: (1) msr bits
mean different things per cpu, (2) msr has 64 bits on 64 cpus
while tb->flags has only 32 bits.

Create a enum to define these bits.  Document the origin of each bit
and validate those bits that must match MSR.  This fixes the
truncation of env->hflags to tb->flags, because we no longer
have hflags bits set above bit 31.

Most of the code in ppc_tr_init_disas_context is moved over to
hreg_compute_hflags.  Some of it is simple extractions from msr,
some requires examining other cpu flags.  Anything that is moved
becomes a simple extract from hflags in ppc_tr_init_disas_context.

Several existing bugs are left in ppc_tr_init_disas_context, where
additional changes are required -- to be addressed in future patches.

Remove a broken #if 0 block.

Reported-by: Ivan Warren <ivan@vmfacility.fr>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/ppc/cpu.h         | 25 ++++++++++++++++
 target/ppc/helper_regs.c | 65 +++++++++++++++++++++++++++++++++-------
 target/ppc/translate.c   | 55 ++++++++++------------------------
 3 files changed, 95 insertions(+), 50 deletions(-)

-- 
2.25.1

Comments

David Gibson March 24, 2021, 12:03 a.m. UTC | #1
On Tue, Mar 23, 2021 at 12:43:32PM -0600, Richard Henderson wrote:
> Copying flags directly from msr has drawbacks: (1) msr bits

> mean different things per cpu, (2) msr has 64 bits on 64 cpus

> while tb->flags has only 32 bits.

> 

> Create a enum to define these bits.  Document the origin of each bit

> and validate those bits that must match MSR.  This fixes the

> truncation of env->hflags to tb->flags, because we no longer

> have hflags bits set above bit 31.

> 

> Most of the code in ppc_tr_init_disas_context is moved over to

> hreg_compute_hflags.  Some of it is simple extractions from msr,

> some requires examining other cpu flags.  Anything that is moved

> becomes a simple extract from hflags in ppc_tr_init_disas_context.

> 

> Several existing bugs are left in ppc_tr_init_disas_context, where

> additional changes are required -- to be addressed in future patches.

> 

> Remove a broken #if 0 block.

> 

> Reported-by: Ivan Warren <ivan@vmfacility.fr>

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


Applied to ppc-for-6.0.

> ---

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

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

>  target/ppc/translate.c   | 55 ++++++++++------------------------

>  3 files changed, 95 insertions(+), 50 deletions(-)

> 

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

> index fd13489dce..fe6c3f815d 100644

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

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

> @@ -585,6 +585,31 @@ enum {

>      POWERPC_FLAG_HID0_LE  = 0x00400000,

>  };

>  

> +/*

> + * Bits for env->hflags.

> + *

> + * Most of these bits overlap with corresponding bits in MSR,

> + * but some come from other sources.  Those that do come from

> + * the MSR are validated in hreg_compute_hflags.

> + */

> +enum {

> +    HFLAGS_LE = 0,   /* MSR_LE -- comes from elsewhere on 601 */

> +    HFLAGS_HV = 1,   /* computed from MSR_HV and other state */

> +    HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */

> +    HFLAGS_DR = 4,   /* MSR_DR */

> +    HFLAGS_IR = 5,   /* MSR_IR */

> +    HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */

> +    HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP */

> +    HFLAGS_TM = 8,   /* computed from MSR_TM */

> +    HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */

> +    HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */

> +    HFLAGS_FP = 13,  /* MSR_FP */

> +    HFLAGS_PR = 14,  /* MSR_PR */

> +    HFLAGS_SA = 22,  /* MSR_SA */

> +    HFLAGS_AP = 23,  /* MSR_AP */

> +    HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */

> +};

> +

>  /*****************************************************************************/

>  /* Floating point status and control register                                */

>  #define FPSCR_DRN2   34 /* Decimal Floating-Point rounding control           */

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

> index a87e354ca2..df9673b90f 100644

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

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

> @@ -18,6 +18,7 @@

>   */

>  

>  #include "qemu/osdep.h"

> +#include "cpu.h"

>  #include "qemu/main-loop.h"

>  #include "exec/exec-all.h"

>  #include "sysemu/kvm.h"

> @@ -87,24 +88,66 @@ void hreg_compute_mem_idx(CPUPPCState *env)

>  

>  void hreg_compute_hflags(CPUPPCState *env)

>  {

> -    target_ulong hflags_mask;

> +    target_ulong msr = env->msr;

> +    uint32_t ppc_flags = env->flags;

> +    uint32_t hflags = 0;

> +    uint32_t msr_mask;

>  

> -    /* We 'forget' FE0 & FE1: we'll never generate imprecise exceptions */

> -    hflags_mask = (1 << MSR_VR) | (1 << MSR_AP) | (1 << MSR_SA) |

> -        (1 << MSR_PR) | (1 << MSR_FP) | (1 << MSR_SE) | (1 << MSR_BE) |

> -        (1 << MSR_LE) | (1 << MSR_VSX) | (1 << MSR_IR) | (1 << MSR_DR);

> -    hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;

> -    hreg_compute_mem_idx(env);

> -    env->hflags = env->msr & hflags_mask;

> +    /* Some bits come straight across from MSR. */

> +    QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE);

> +    QEMU_BUILD_BUG_ON(MSR_PR != HFLAGS_PR);

> +    QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);

> +    QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);

> +    QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);

> +    QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);

> +    QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);

> +    msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |

> +                (1 << MSR_DR) | (1 << MSR_IR) |

> +                (1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));

>  

> -    if (env->flags & POWERPC_FLAG_HID0_LE) {

> +    if (ppc_flags & POWERPC_FLAG_HID0_LE) {

>          /*

>           * Note that MSR_LE is not set in env->msr_mask for this cpu,

> -         * and so will never be set in msr or hflags at this point.

> +         * and so will never be set in msr.

>           */

>          uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);

> -        env->hflags |= le << MSR_LE;

> +        hflags |= le << MSR_LE;

>      }

> +

> +    if (ppc_flags & POWERPC_FLAG_BE) {

> +        QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);

> +        msr_mask |= 1 << MSR_BE;

> +    }

> +    if (ppc_flags & POWERPC_FLAG_SE) {

> +        QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);

> +        msr_mask |= 1 << MSR_SE;

> +    }

> +

> +    if (msr_is_64bit(env, msr)) {

> +        hflags |= 1 << HFLAGS_64;

> +    }

> +    if ((ppc_flags & POWERPC_FLAG_SPE) && (msr & (1 << MSR_SPE))) {

> +        hflags |= 1 << HFLAGS_SPE;

> +    }

> +    if (ppc_flags & POWERPC_FLAG_VRE) {

> +        QEMU_BUILD_BUG_ON(MSR_VR != HFLAGS_VR);

> +        msr_mask |= 1 << MSR_VR;

> +    }

> +    if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {

> +        hflags |= 1 << HFLAGS_VSX;

> +    }

> +    if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {

> +        hflags |= 1 << HFLAGS_TM;

> +    }

> +

> +#ifndef CONFIG_USER_ONLY

> +    if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {

> +        hflags |= 1 << HFLAGS_HV;

> +    }

> +#endif

> +

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

> +    hreg_compute_mem_idx(env);

>  }

>  

>  void cpu_interrupt_exittb(CPUState *cs)

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

> index 0984ce637b..a9325a12e5 100644

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

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

> @@ -7879,67 +7879,48 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)

>  {

>      DisasContext *ctx = container_of(dcbase, DisasContext, base);

>      CPUPPCState *env = cs->env_ptr;

> +    uint32_t hflags = ctx->base.tb->flags;

>      int bound;

>  

>      ctx->exception = POWERPC_EXCP_NONE;

>      ctx->spr_cb = env->spr_cb;

> -    ctx->pr = msr_pr;

> +    ctx->pr = (hflags >> HFLAGS_PR) & 1;

>      ctx->mem_idx = env->dmmu_idx;

> -    ctx->dr = msr_dr;

> -#if !defined(CONFIG_USER_ONLY)

> -    ctx->hv = msr_hv || !env->has_hv_mode;

> -#endif

> +    ctx->dr = (hflags >> HFLAGS_DR) & 1;

> +    ctx->hv = (hflags >> HFLAGS_HV) & 1;

>      ctx->insns_flags = env->insns_flags;

>      ctx->insns_flags2 = env->insns_flags2;

>      ctx->access_type = -1;

>      ctx->need_access_type = !mmu_is_64bit(env->mmu_model);

> -    ctx->le_mode = !!(env->hflags & (1 << MSR_LE));

> +    ctx->le_mode = (hflags >> HFLAGS_LE) & 1;

>      ctx->default_tcg_memop_mask = ctx->le_mode ? MO_LE : MO_BE;

>      ctx->flags = env->flags;

>  #if defined(TARGET_PPC64)

> -    ctx->sf_mode = msr_is_64bit(env, env->msr);

> +    ctx->sf_mode = (hflags >> HFLAGS_64) & 1;

>      ctx->has_cfar = !!(env->flags & POWERPC_FLAG_CFAR);

>  #endif

>      ctx->lazy_tlb_flush = env->mmu_model == POWERPC_MMU_32B

>          || env->mmu_model == POWERPC_MMU_601

>          || env->mmu_model & POWERPC_MMU_64;

>  

> -    ctx->fpu_enabled = !!msr_fp;

> -    if ((env->flags & POWERPC_FLAG_SPE) && msr_spe) {

> -        ctx->spe_enabled = !!msr_spe;

> -    } else {

> -        ctx->spe_enabled = false;

> -    }

> -    if ((env->flags & POWERPC_FLAG_VRE) && msr_vr) {

> -        ctx->altivec_enabled = !!msr_vr;

> -    } else {

> -        ctx->altivec_enabled = false;

> -    }

> -    if ((env->flags & POWERPC_FLAG_VSX) && msr_vsx) {

> -        ctx->vsx_enabled = !!msr_vsx;

> -    } else {

> -        ctx->vsx_enabled = false;

> -    }

> +    ctx->fpu_enabled = (hflags >> HFLAGS_FP) & 1;

> +    ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;

> +    ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;

> +    ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;

>      if ((env->flags & POWERPC_FLAG_SCV)

>          && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {

>          ctx->scv_enabled = true;

>      } else {

>          ctx->scv_enabled = false;

>      }

> -#if defined(TARGET_PPC64)

> -    if ((env->flags & POWERPC_FLAG_TM) && msr_tm) {

> -        ctx->tm_enabled = !!msr_tm;

> -    } else {

> -        ctx->tm_enabled = false;

> -    }

> -#endif

> +    ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;

>      ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);

> -    if ((env->flags & POWERPC_FLAG_SE) && msr_se) {

> -        ctx->singlestep_enabled = CPU_SINGLE_STEP;

> -    } else {

> -        ctx->singlestep_enabled = 0;

> +

> +    ctx->singlestep_enabled = 0;

> +    if ((hflags >> HFLAGS_SE) & 1) {

> +        ctx->singlestep_enabled |= CPU_SINGLE_STEP;

>      }

> -    if ((env->flags & POWERPC_FLAG_BE) && msr_be) {

> +    if ((hflags >> HFLAGS_BE) & 1) {

>          ctx->singlestep_enabled |= CPU_BRANCH_STEP;

>      }

>      if ((env->flags & POWERPC_FLAG_DE) && msr_de) {

> @@ -7956,10 +7937,6 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)

>      if (unlikely(ctx->base.singlestep_enabled)) {

>          ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;

>      }

> -#if defined(DO_SINGLE_STEP) && 0

> -    /* Single step trace mode */

> -    msr_se = 1;

> -#endif

>  

>      bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;

>      ctx->base.max_insns = MIN(ctx->base.max_insns, bound);


-- 
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
Greg Kurz March 29, 2021, 1:05 p.m. UTC | #2
On Wed, 24 Mar 2021 11:03:02 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

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

> > Copying flags directly from msr has drawbacks: (1) msr bits

> > mean different things per cpu, (2) msr has 64 bits on 64 cpus

> > while tb->flags has only 32 bits.

> > 

> > Create a enum to define these bits.  Document the origin of each bit

> > and validate those bits that must match MSR.  This fixes the

> > truncation of env->hflags to tb->flags, because we no longer

> > have hflags bits set above bit 31.

> > 

> > Most of the code in ppc_tr_init_disas_context is moved over to

> > hreg_compute_hflags.  Some of it is simple extractions from msr,

> > some requires examining other cpu flags.  Anything that is moved

> > becomes a simple extract from hflags in ppc_tr_init_disas_context.

> > 

> > Several existing bugs are left in ppc_tr_init_disas_context, where

> > additional changes are required -- to be addressed in future patches.

> > 

> > Remove a broken #if 0 block.

> > 

> > Reported-by: Ivan Warren <ivan@vmfacility.fr>

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

> 

> Applied to ppc-for-6.0.

> 


FYI I can consistently reproduce locally on my laptop an error I'm also
seeing in CI.

$ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg
...
  TEST    threadcount on ppc64abi32
qemu: uncaught target signal 11 (Segmentation fault) - core dumped

Bisect led to this commit in ppc-for-6.0 

> > ---

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

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

> >  target/ppc/translate.c   | 55 ++++++++++------------------------

> >  3 files changed, 95 insertions(+), 50 deletions(-)

> > 

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

> > index fd13489dce..fe6c3f815d 100644

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

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

> > @@ -585,6 +585,31 @@ enum {

> >      POWERPC_FLAG_HID0_LE  = 0x00400000,$ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg


> >  };

> >  

> > +/*

> > + * Bits for env->hflags.

> > + *

> > + * Most of these bits overlap with corresponding bits in MSR,

> > + * but some come from other sources.  Those that do come from

> > + * the MSR are validated in hreg_compute_hflags.

> > + */

> > +enum {

> > +    HFLAGS_LE = 0,   /* MSR_LE -- comes from elsewhere on 601 */

> > +    HFLAGS_HV = 1,   /* computed from MSR_HV and other state */

> > +    HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */

> > +    HFLAGS_DR = 4,   /* MSR_DR */

> > +    HFLAGS_IR = 5,   /* MSR_IR */

> > +    HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */

> > +    HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP */

> > +    HFLAGS_TM = 8,   /* computed from MSR_TM */

> > +    HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */

> > +    HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */

> > +    HFLAGS_FP = 13,  /* MSR_FP */

> > +    HFLAGS_PR = 14,  /* MSR_PR */

> > +    HFLAGS_SA = 22,  /* MSR_SA */

> > +    HFLAGS_AP = 23,  /* MSR_AP */

> > +    HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */

> > +};

> > +

> >  /*****************************************************************************/

> >  /* Floating point status and control register                                */

> >  #define FPSCR_DRN2   34 /* Decimal Floating-Point rounding control           */

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

> > index a87e354ca2..df9673b90f 100644

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

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

> > @@ -18,6 +18,7 @@

> >   */

> >  

> >  #include "qemu/osdep.h"

> > +#include "cpu.h"

> >  #include "qemu/main-loop.h"

> >  #include "exec/exec-all.h"

> >  #include "sysemu/kvm.h"

> > @@ -87,24 +88,66 @@ void hreg_compute_mem_idx(CPUPPCState *env)

> >  

> >  void hreg_compute_hflags(CPUPPCState *env)

> >  {

> > -    target_ulong hflags_mask;

> > +    target_ulong msr = env->msr;

> > +    uint32_t ppc_flags = env->flags;

> > +    uint32_t hflags = 0;

> > +    uint32_t msr_mask;

> >  $ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg


> > -    /* We 'forget' FE0 & FE1: we'll never generate imprecise exceptions */

> > -    hflags_mask = (1 << MSR_VR) | (1 << MSR_AP) | (1 << MSR_SA) |

> > -        (1 << MSR_PR) | (1 << MSR_FP) | (1 << MSR_SE) | (1 << MSR_BE) |

> > -        (1 << MSR_LE) | (1 << MSR_VSX) | (1 << MSR_IR) | (1 << MSR_DR);

> > -    hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;

> > -    hreg_compute_mem_idx(env);

> > -    env->hflags = env->msr & hflags_mask;

> > +    /* Some bits come straight across from MSR. */

> > +    QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE);

> > +    QEMU_BUILD_BUG_ON(MSR_PR != HFLAGS_PR);

> > +    QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);

> > +    QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);

> > +    QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);

> > +    QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);

> > +    QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);

> > +    msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |

> > +                (1 << MSR_DR) | (1 << MSR_IR) |

> > +                (1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));

> >  

> > -    if (env->flags & POWERPC_FLAG_HID0_LE) {

> > +    if (ppc_flags & POWERPC_FLAG_HID0_LE) {

> >          /*

> >           * Note that MSR_LE is not set in env->msr_mask for this cpu,

> > -         * and so will never be set in msr or hflags at this point.

> > +         * and so will never be set in msr.

> >           */

> >          uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);

> > -        env->hflags |= le << MSR_LE;

> > +        hflags |= le << MSR_LE;

> >      }

> > +

> > +    if (ppc_flags & POWERPC_FLAG_BE) {

> > +        QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);

> > +        msr_mask |= 1 << MSR_BE;

> > +    }

> > +    if (ppc_flags & POWERPC_FLAG_SE) {

> > +        QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);

> > +        msr_mask |= 1 << MSR_SE;

> > +    }

> > +

> > +    if (msr_is_64bit(env, msr)) {

> > +        hflags |= 1 << HFLAGS_64;

> > +    }

> > +    if ((ppc_flags & POWERPC_FLAG_SPE) && (msr & (1 << MSR_SPE))) {

> > +        hflags |= 1 << HFLAGS_SPE;

> > +    }

> > +    if (ppc_flags & POWERPC_FLAG_VRE) {

> > +        QEMU_BUILD_BUG_ON(MSR_VR != HFLAGS_VR);

> > +        msr_mask |= 1 << MSR_VR;

> > +    }

> > +    if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {

> > +        hflags |= 1 << HFLAGS_VSX;

> > +    }

> > +    if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {

> > +        hflags |= 1 << HFLAGS_TM;

> > +    }

> > +

> > +#ifndef CONFIG_USER_ONLY

> > +    if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {

> > +        hflags |= 1 << HFLAGS_HV;

> > +    }

> > +#endif

> > +

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

> > +    hreg_compute_mem_idx(env);

> >  }

> >  

> >  void cpu_interrupt_exittb(CPUState *cs)

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

> > index 0984ce637b..a9325a12e5 100644

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

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

> > @@ -7879,67 +7879,48 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)

> >  {

> >      DisasContext *ctx = container_of(dcbase, DisasContext, base);

> >      CPUPPCState *env = cs->env_ptr;

> > +    uint32_t hflags = ctx->base.tb->flags;

> >      int bound;

> >  

> >      ctx->exception = POWERPC_EXCP_NONE;

> >      ctx->spr_cb = env->spr_cb;

> > -    ctx->pr = msr_pr;

> > +    ctx->pr = (hflags >> HFLAGS_PR) & 1;

> >      ctx->mem_idx = env->dmmu_idx;

> > -    ctx->dr = msr_dr;

> > -#if !defined(CONFIG_USER_ONLY)

> > -    ctx->hv = msr_hv || !env->has_hv_mode;

> > -#endif

> > +    ctx->dr = (hflags >> HFLAGS_DR) & 1;

> > +    ctx->hv = (hflags >> HFLAGS_HV) & 1;

> >      ctx->insns_flags = env->insns_flags;

> >      ctx->insns_flags2 = env->insns_flags2;

> >      ctx->access_type = -1;

> >      ctx->need_access_type = !mmu_is_64bit(env->mmu_model);

> > -    ctx->le_mode = !!(env->hflags & (1 << MSR_LE));

> > +    ctx->le_mode = (hflags >> HFLAGS_LE) & 1;

> >      ctx->default_tcg_memop_mask = ctx->le_mode ? MO_LE : MO_BE;

> >      ctx->flags = env->flags;

> >  #if defined(TARGET_PPC64)

> > -    ctx->sf_mode = msr_is_64bit(env, env->msr);

> > +    ctx->sf_mode = (hflags >> HFLAGS_64) & 1;

> >      ctx->has_cfar = !!(env->flags & POWERPC_FLAG_CFAR);

> >  #endif

> >      ctx->lazy_tlb_flush = env->mmu_model == POWERPC_MMU_32B

> >          || env->mmu_model == POWERPC_MMU_601

> >          || env->mmu_model & POWERPC_MMU_64;

> >  

> > -    ctx->fpu_enabled = !!msr_fp;

> > -    if ((env->flags & POWERPC_FLAG_SPE) && msr_spe) {

> > -        ctx->spe_enabled = !!msr_spe;

> > -    } else {

> > -        ctx->spe_enabled = false;

> > -    }

> > -    if ((env->flags & POWERPC_FLAG_VRE) && msr_vr) {

> > -        ctx->altivec_enabled = !!msr_vr;

> > -    } else {

> > -        ctx->altivec_enabled = false;

> > -    }

> > -    if ((env->flags & POWERPC_FLAG_VSX) && msr_vsx) {

> > -        ctx->vsx_enabled = !!msr_vsx;

> > -    } else {

> > -        ctx->vsx_enabled = false;

> > -    }

> > +    ctx->fpu_enabled = (hflags >> HFLAGS_FP) & 1;

> > +    ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;

> > +    ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;

> > +    ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;

> >      if ((env->flags & POWERPC_FLAG_SCV)

> >          && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {

> >          ctx->scv_enabled = true;

> >      } else {

> >          ctx->scv_enabled = false;

> >      }

> > -#if defined(TARGET_PPC64)

> > -    if ((env->flags & POWERPC_FLAG_TM) && msr_tm) {

> > -        ctx->tm_enabled = !!msr_tm;

> > -    } else {

> > -        ctx->tm_enabled = false;

> > -    }

> > -#endif

> > +    ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;

> >      ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);

> > -    if ((env->flags & POWERPC_FLAG_SE) && msr_se) {

> > -        ctx->singlestep_enabled = CPU_SINGLE_STEP;

> > -    } else {

> > -        ctx->singlestep_enabled = 0;

> > +

> > +    ctx->singlestep_enabled = 0;

> > +    if ((hflags >> HFLAGS_SE) & 1) {

> > +        ctx->singlestep_enabled |= CPU_SINGLE_STEP;

> >      }

> > -    if ((env->flags & POWERPC_FLAG_BE) && msr_be) {

> > +    if ((hflags >> HFLAGS_BE) & 1) {

> >          ctx->singlestep_enabled |= CPU_BRANCH_STEP;

> >      }

> >      if ((env->flags & POWERPC_FLAG_DE) && msr_de) {

> > @@ -7956,10 +7937,6 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)

> >      if (unlikely(ctx->base.singlestep_enabled)) {

> >          ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;

> >      }

> > -#if defined(DO_SINGLE_STEP) && 0

> > -    /* Single step trace mode */

> > -    msr_se = 1;

> > -#endif

> >  

> >      bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;

> >      ctx->base.max_insns = MIN(ctx->base.max_insns, bound);

>
Richard Henderson March 29, 2021, 4:26 p.m. UTC | #3
On 3/29/21 7:05 AM, Greg Kurz wrote:
> On Wed, 24 Mar 2021 11:03:02 +1100

> David Gibson <david@gibson.dropbear.id.au> wrote:

> 

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

>>> Copying flags directly from msr has drawbacks: (1) msr bits

>>> mean different things per cpu, (2) msr has 64 bits on 64 cpus

>>> while tb->flags has only 32 bits.

>>>

>>> Create a enum to define these bits.  Document the origin of each bit

>>> and validate those bits that must match MSR.  This fixes the

>>> truncation of env->hflags to tb->flags, because we no longer

>>> have hflags bits set above bit 31.

>>>

>>> Most of the code in ppc_tr_init_disas_context is moved over to

>>> hreg_compute_hflags.  Some of it is simple extractions from msr,

>>> some requires examining other cpu flags.  Anything that is moved

>>> becomes a simple extract from hflags in ppc_tr_init_disas_context.

>>>

>>> Several existing bugs are left in ppc_tr_init_disas_context, where

>>> additional changes are required -- to be addressed in future patches.

>>>

>>> Remove a broken #if 0 block.

>>>

>>> Reported-by: Ivan Warren <ivan@vmfacility.fr>

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

>>

>> Applied to ppc-for-6.0.

>>

> 

> FYI I can consistently reproduce locally on my laptop an error I'm also

> seeing in CI.

> 

> $ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg

> ...

>    TEST    threadcount on ppc64abi32

> qemu: uncaught target signal 11 (Segmentation fault) - core dumped

> 

> Bisect led to this commit in ppc-for-6.0


Any more details?  Because this works for me.


r~
David Gibson March 30, 2021, 4:54 a.m. UTC | #4
On Mon, Mar 29, 2021 at 10:26:02AM -0600, Richard Henderson wrote:
> On 3/29/21 7:05 AM, Greg Kurz wrote:

> > On Wed, 24 Mar 2021 11:03:02 +1100

> > David Gibson <david@gibson.dropbear.id.au> wrote:

> > 

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

> > > > Copying flags directly from msr has drawbacks: (1) msr bits

> > > > mean different things per cpu, (2) msr has 64 bits on 64 cpus

> > > > while tb->flags has only 32 bits.

> > > > 

> > > > Create a enum to define these bits.  Document the origin of each bit

> > > > and validate those bits that must match MSR.  This fixes the

> > > > truncation of env->hflags to tb->flags, because we no longer

> > > > have hflags bits set above bit 31.

> > > > 

> > > > Most of the code in ppc_tr_init_disas_context is moved over to

> > > > hreg_compute_hflags.  Some of it is simple extractions from msr,

> > > > some requires examining other cpu flags.  Anything that is moved

> > > > becomes a simple extract from hflags in ppc_tr_init_disas_context.

> > > > 

> > > > Several existing bugs are left in ppc_tr_init_disas_context, where

> > > > additional changes are required -- to be addressed in future patches.

> > > > 

> > > > Remove a broken #if 0 block.

> > > > 

> > > > Reported-by: Ivan Warren <ivan@vmfacility.fr>

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

> > > 

> > > Applied to ppc-for-6.0.

> > > 

> > 

> > FYI I can consistently reproduce locally on my laptop an error I'm also

> > seeing in CI.

> > 

> > $ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg

> > ...

> >    TEST    threadcount on ppc64abi32

> > qemu: uncaught target signal 11 (Segmentation fault) - core dumped

> > 

> > Bisect led to this commit in ppc-for-6.0

> 

> Any more details?  Because this works for me.


Yeah, I haven't gotten this to fail locally either.

But wait... it gets even weirder.  With gitlab, I can get similar
looking failures with

  A) Just the non-hflags patches from my tree
     https://gitlab.com/dgibson/qemu/-/pipelines/278542370

  B) Just the hflags patches from my / Richard's tree
     https://gitlab.com/dgibson/qemu/-/pipelines/278497244

But I haven't managed to get the same failures with (C) their common
ancestor (i.e. the upstream master at the time I split made the (A)
and (B) branches).
    https://gitlab.com/dgibson/qemu/-/pipelines/278497233

With (A) and (B) the build-user and build-user-static tests don't
always fail, but they generally seem to fail within 2-4 attempts.
I've made a bunch of retries on (C) and haven't managed to get either
build-user or build-user-static to fail.

-- 
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 30, 2021, 3:01 p.m. UTC | #5
On 3/29/21 10:54 PM, David Gibson wrote:
>    B) Just the hflags patches from my / Richard's tree

>       https://gitlab.com/dgibson/qemu/-/pipelines/278497244


Look closer at this one -- it's an s390x test that's failing:

make: *** [/builds/dgibson/qemu/tests/Makefile.include:63: 
run-tcg-tests-s390x-linux-user] Error 2


r~
David Gibson March 31, 2021, 12:09 a.m. UTC | #6
On Tue, Mar 30, 2021 at 08:01:13AM -0700, Richard Henderson wrote:
> On 3/29/21 10:54 PM, David Gibson wrote:

> >    B) Just the hflags patches from my / Richard's tree

> >       https://gitlab.com/dgibson/qemu/-/pipelines/278497244

> 

> Look closer at this one -- it's an s390x test that's failing:

> 

> make: *** [/builds/dgibson/qemu/tests/Makefile.include:63:

> run-tcg-tests-s390x-linux-user] Error 2


Goldangit!  Good point.  Now I'm even more baffled as to how I wasn't
able to reproduce on master, but was on different variants of my tree.
A whole heap of bad luck, I guess.

-- 
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
Greg Kurz March 31, 2021, 4:04 a.m. UTC | #7
On Wed, 31 Mar 2021 11:09:06 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Mar 30, 2021 at 08:01:13AM -0700, Richard Henderson wrote:

> > On 3/29/21 10:54 PM, David Gibson wrote:

> > >    B) Just the hflags patches from my / Richard's tree

> > >       https://gitlab.com/dgibson/qemu/-/pipelines/278497244

> > 

> > Look closer at this one -- it's an s390x test that's failing:

> > 


I've been seeing errors with s390x as well in CI but I couldn't
reproduce locally... and of course, now it seems I cannot
reproduce locally with ppc64abi32 either :-\

> > make: *** [/builds/dgibson/qemu/tests/Makefile.include:63:

> > run-tcg-tests-s390x-linux-user] Error 2

> 

> Goldangit!  Good point.  Now I'm even more baffled as to how I wasn't

> able to reproduce on master, but was on different variants of my tree.

> A whole heap of bad luck, I guess.

>
David Gibson March 31, 2021, 4:47 a.m. UTC | #8
On Wed, Mar 31, 2021 at 06:04:27AM +0200, Greg Kurz wrote:
> On Wed, 31 Mar 2021 11:09:06 +1100

> David Gibson <david@gibson.dropbear.id.au> wrote:

> 

> > On Tue, Mar 30, 2021 at 08:01:13AM -0700, Richard Henderson wrote:

> > > On 3/29/21 10:54 PM, David Gibson wrote:

> > > >    B) Just the hflags patches from my / Richard's tree

> > > >       https://gitlab.com/dgibson/qemu/-/pipelines/278497244

> > > 

> > > Look closer at this one -- it's an s390x test that's failing:

> > > 

> 

> I've been seeing errors with s390x as well in CI but I couldn't

> reproduce locally... and of course, now it seems I cannot

> reproduce locally with ppc64abi32 either :-\


Huh.  Well supporting the idea that the issues I've seen on gitlab
were just bad luck, I've now gotten a clean check with the hflags
patches... bug only on my ppc-for-6.1 branch.

The ppc64 bug that Greg was seeing still makes me nervous, as does the
failures which we saw at one point which showed that new hflags assert
explicitly failing.

Since the hflags stuff is of moderate complexity and is a bug fix,
it's not a regression fix.  So, I'm going to postpone that until
ppc-for-6.1, and move ahead with this PR without it.

Richard - the remaining possible problem with the hflags stuff seems
to manifest with the assert failing in the last patch.  However, I'm
guess that's just exposing some more subtle problem introduced by an
earlier patch.  Any chance you could re-order the series to insert the
assert near the beginning, which might give us a better way of
bisecting if this shows up again.

Greg, if this shows up again for you locally, can you please try to
track it down.

-- 
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 31, 2021, 6:31 a.m. UTC | #9
On 3/30/21 10:47 PM, David Gibson wrote:
> Richard - the remaining possible problem with the hflags stuff seems

> to manifest with the assert failing in the last patch.  However, I'm

> guess that's just exposing some more subtle problem introduced by an

> earlier patch.  Any chance you could re-order the series to insert the

> assert near the beginning, which might give us a better way of

> bisecting if this shows up again.


No, we begin in a very sorry state.  Every one of the patches reduces the 
number of bugs with hflags.  If we put the assert anywhere but the end, it 
*will* fail.

If you can get the assert to trigger again, we can work out where the next 
problem is hiding.


r~
Greg Kurz March 31, 2021, 7:30 a.m. UTC | #10
On Wed, 31 Mar 2021 15:47:26 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Mar 31, 2021 at 06:04:27AM +0200, Greg Kurz wrote:

> > On Wed, 31 Mar 2021 11:09:06 +1100

> > David Gibson <david@gibson.dropbear.id.au> wrote:

> > 

> > > On Tue, Mar 30, 2021 at 08:01:13AM -0700, Richard Henderson wrote:

> > > > On 3/29/21 10:54 PM, David Gibson wrote:

> > > > >    B) Just the hflags patches from my / Richard's tree

> > > > >       https://gitlab.com/dgibson/qemu/-/pipelines/278497244

> > > > 

> > > > Look closer at this one -- it's an s390x test that's failing:

> > > > 

> > 

> > I've been seeing errors with s390x as well in CI but I couldn't

> > reproduce locally... and of course, now it seems I cannot

> > reproduce locally with ppc64abi32 either :-\

> 

> Huh.  Well supporting the idea that the issues I've seen on gitlab

> were just bad luck, I've now gotten a clean check with the hflags

> patches... bug only on my ppc-for-6.1 branch.

> 

> The ppc64 bug that Greg was seeing still makes me nervous, as does the

> failures which we saw at one point which showed that new hflags assert

> explicitly failing.

> 

> Since the hflags stuff is of moderate complexity and is a bug fix,

> it's not a regression fix.  So, I'm going to postpone that until

> ppc-for-6.1, and move ahead with this PR without it.

> 

> Richard - the remaining possible problem with the hflags stuff seems

> to manifest with the assert failing in the last patch.  However, I'm

> guess that's just exposing some more subtle problem introduced by an

> earlier patch.  Any chance you could re-order the series to insert the

> assert near the beginning, which might give us a better way of

> bisecting if this shows up again.

> 

> Greg, if this shows up again for you locally, can you please try to

> track it down.

> 


Will do.

Cheers,

--
Greg
David Gibson April 1, 2021, 3:17 a.m. UTC | #11
On Tue, Mar 30, 2021 at 11:31:25PM -0700, Richard Henderson wrote:
> On 3/30/21 10:47 PM, David Gibson wrote:

> > Richard - the remaining possible problem with the hflags stuff seems

> > to manifest with the assert failing in the last patch.  However, I'm

> > guess that's just exposing some more subtle problem introduced by an

> > earlier patch.  Any chance you could re-order the series to insert the

> > assert near the beginning, which might give us a better way of

> > bisecting if this shows up again.

> 

> No, we begin in a very sorry state.  Every one of the patches reduces the

> number of bugs with hflags.  If we put the assert anywhere but the end, it

> *will* fail.


Heh, ok, understood.

> If you can get the assert to trigger again, we can work out where the next

> problem is hiding.


Ok.  *fingers crossed*

-- 
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 fd13489dce..fe6c3f815d 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -585,6 +585,31 @@  enum {
     POWERPC_FLAG_HID0_LE  = 0x00400000,
 };
 
+/*
+ * Bits for env->hflags.
+ *
+ * Most of these bits overlap with corresponding bits in MSR,
+ * but some come from other sources.  Those that do come from
+ * the MSR are validated in hreg_compute_hflags.
+ */
+enum {
+    HFLAGS_LE = 0,   /* MSR_LE -- comes from elsewhere on 601 */
+    HFLAGS_HV = 1,   /* computed from MSR_HV and other state */
+    HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
+    HFLAGS_DR = 4,   /* MSR_DR */
+    HFLAGS_IR = 5,   /* MSR_IR */
+    HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
+    HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP */
+    HFLAGS_TM = 8,   /* computed from MSR_TM */
+    HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
+    HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
+    HFLAGS_FP = 13,  /* MSR_FP */
+    HFLAGS_PR = 14,  /* MSR_PR */
+    HFLAGS_SA = 22,  /* MSR_SA */
+    HFLAGS_AP = 23,  /* MSR_AP */
+    HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
+};
+
 /*****************************************************************************/
 /* Floating point status and control register                                */
 #define FPSCR_DRN2   34 /* Decimal Floating-Point rounding control           */
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index a87e354ca2..df9673b90f 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -18,6 +18,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "cpu.h"
 #include "qemu/main-loop.h"
 #include "exec/exec-all.h"
 #include "sysemu/kvm.h"
@@ -87,24 +88,66 @@  void hreg_compute_mem_idx(CPUPPCState *env)
 
 void hreg_compute_hflags(CPUPPCState *env)
 {
-    target_ulong hflags_mask;
+    target_ulong msr = env->msr;
+    uint32_t ppc_flags = env->flags;
+    uint32_t hflags = 0;
+    uint32_t msr_mask;
 
-    /* We 'forget' FE0 & FE1: we'll never generate imprecise exceptions */
-    hflags_mask = (1 << MSR_VR) | (1 << MSR_AP) | (1 << MSR_SA) |
-        (1 << MSR_PR) | (1 << MSR_FP) | (1 << MSR_SE) | (1 << MSR_BE) |
-        (1 << MSR_LE) | (1 << MSR_VSX) | (1 << MSR_IR) | (1 << MSR_DR);
-    hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;
-    hreg_compute_mem_idx(env);
-    env->hflags = env->msr & hflags_mask;
+    /* Some bits come straight across from MSR. */
+    QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE);
+    QEMU_BUILD_BUG_ON(MSR_PR != HFLAGS_PR);
+    QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
+    QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
+    QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
+    QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);
+    QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);
+    msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
+                (1 << MSR_DR) | (1 << MSR_IR) |
+                (1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
 
-    if (env->flags & POWERPC_FLAG_HID0_LE) {
+    if (ppc_flags & POWERPC_FLAG_HID0_LE) {
         /*
          * Note that MSR_LE is not set in env->msr_mask for this cpu,
-         * and so will never be set in msr or hflags at this point.
+         * and so will never be set in msr.
          */
         uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);
-        env->hflags |= le << MSR_LE;
+        hflags |= le << MSR_LE;
     }
+
+    if (ppc_flags & POWERPC_FLAG_BE) {
+        QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
+        msr_mask |= 1 << MSR_BE;
+    }
+    if (ppc_flags & POWERPC_FLAG_SE) {
+        QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
+        msr_mask |= 1 << MSR_SE;
+    }
+
+    if (msr_is_64bit(env, msr)) {
+        hflags |= 1 << HFLAGS_64;
+    }
+    if ((ppc_flags & POWERPC_FLAG_SPE) && (msr & (1 << MSR_SPE))) {
+        hflags |= 1 << HFLAGS_SPE;
+    }
+    if (ppc_flags & POWERPC_FLAG_VRE) {
+        QEMU_BUILD_BUG_ON(MSR_VR != HFLAGS_VR);
+        msr_mask |= 1 << MSR_VR;
+    }
+    if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {
+        hflags |= 1 << HFLAGS_VSX;
+    }
+    if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
+        hflags |= 1 << HFLAGS_TM;
+    }
+
+#ifndef CONFIG_USER_ONLY
+    if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
+        hflags |= 1 << HFLAGS_HV;
+    }
+#endif
+
+    env->hflags = hflags | (msr & msr_mask);
+    hreg_compute_mem_idx(env);
 }
 
 void cpu_interrupt_exittb(CPUState *cs)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 0984ce637b..a9325a12e5 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7879,67 +7879,48 @@  static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPUPPCState *env = cs->env_ptr;
+    uint32_t hflags = ctx->base.tb->flags;
     int bound;
 
     ctx->exception = POWERPC_EXCP_NONE;
     ctx->spr_cb = env->spr_cb;
-    ctx->pr = msr_pr;
+    ctx->pr = (hflags >> HFLAGS_PR) & 1;
     ctx->mem_idx = env->dmmu_idx;
-    ctx->dr = msr_dr;
-#if !defined(CONFIG_USER_ONLY)
-    ctx->hv = msr_hv || !env->has_hv_mode;
-#endif
+    ctx->dr = (hflags >> HFLAGS_DR) & 1;
+    ctx->hv = (hflags >> HFLAGS_HV) & 1;
     ctx->insns_flags = env->insns_flags;
     ctx->insns_flags2 = env->insns_flags2;
     ctx->access_type = -1;
     ctx->need_access_type = !mmu_is_64bit(env->mmu_model);
-    ctx->le_mode = !!(env->hflags & (1 << MSR_LE));
+    ctx->le_mode = (hflags >> HFLAGS_LE) & 1;
     ctx->default_tcg_memop_mask = ctx->le_mode ? MO_LE : MO_BE;
     ctx->flags = env->flags;
 #if defined(TARGET_PPC64)
-    ctx->sf_mode = msr_is_64bit(env, env->msr);
+    ctx->sf_mode = (hflags >> HFLAGS_64) & 1;
     ctx->has_cfar = !!(env->flags & POWERPC_FLAG_CFAR);
 #endif
     ctx->lazy_tlb_flush = env->mmu_model == POWERPC_MMU_32B
         || env->mmu_model == POWERPC_MMU_601
         || env->mmu_model & POWERPC_MMU_64;
 
-    ctx->fpu_enabled = !!msr_fp;
-    if ((env->flags & POWERPC_FLAG_SPE) && msr_spe) {
-        ctx->spe_enabled = !!msr_spe;
-    } else {
-        ctx->spe_enabled = false;
-    }
-    if ((env->flags & POWERPC_FLAG_VRE) && msr_vr) {
-        ctx->altivec_enabled = !!msr_vr;
-    } else {
-        ctx->altivec_enabled = false;
-    }
-    if ((env->flags & POWERPC_FLAG_VSX) && msr_vsx) {
-        ctx->vsx_enabled = !!msr_vsx;
-    } else {
-        ctx->vsx_enabled = false;
-    }
+    ctx->fpu_enabled = (hflags >> HFLAGS_FP) & 1;
+    ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;
+    ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
+    ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
     if ((env->flags & POWERPC_FLAG_SCV)
         && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {
         ctx->scv_enabled = true;
     } else {
         ctx->scv_enabled = false;
     }
-#if defined(TARGET_PPC64)
-    if ((env->flags & POWERPC_FLAG_TM) && msr_tm) {
-        ctx->tm_enabled = !!msr_tm;
-    } else {
-        ctx->tm_enabled = false;
-    }
-#endif
+    ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
     ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
-    if ((env->flags & POWERPC_FLAG_SE) && msr_se) {
-        ctx->singlestep_enabled = CPU_SINGLE_STEP;
-    } else {
-        ctx->singlestep_enabled = 0;
+
+    ctx->singlestep_enabled = 0;
+    if ((hflags >> HFLAGS_SE) & 1) {
+        ctx->singlestep_enabled |= CPU_SINGLE_STEP;
     }
-    if ((env->flags & POWERPC_FLAG_BE) && msr_be) {
+    if ((hflags >> HFLAGS_BE) & 1) {
         ctx->singlestep_enabled |= CPU_BRANCH_STEP;
     }
     if ((env->flags & POWERPC_FLAG_DE) && msr_de) {
@@ -7956,10 +7937,6 @@  static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     if (unlikely(ctx->base.singlestep_enabled)) {
         ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;
     }
-#if defined(DO_SINGLE_STEP) && 0
-    /* Single step trace mode */
-    msr_se = 1;
-#endif
 
     bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;
     ctx->base.max_insns = MIN(ctx->base.max_insns, bound);