| Message ID | 20210124032422.2113565-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Fix truncation of env->hflags | expand |
On Sat, Jan 23, 2021 at 05:24:22PM -1000, Richard Henderson wrote: > Use the cs_base field, because it happens to be the same > size as hflags (and MSR, from which hflags is derived). > > In translate, extract most bits from a local hflags variable. > Mark several cases where code generation is *not* derived from > data stored within the hashed elements of the TranslationBlock. My knowledge of TCG isn't great, so I'm pretty much prepared to accept this is correct on your say so. But that commit message feels like it's following on from a conversation that's not here, nor linked. It'd be great if it explained how said hflags truncation is happening, because it's certainly not obvious to someone with only a fair to middling understanding of TCG. > Cc: David Gibson <david@gibson.dropbear.id.au> > Reported-by: Ivan Warren <ivan@vmfacility.fr> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/ppc/cpu.h | 4 +-- > target/ppc/translate.c | 64 ++++++++++++++++-------------------------- > 2 files changed, 26 insertions(+), 42 deletions(-) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 2609e4082e..4a05e4e544 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -2396,8 +2396,8 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc, > target_ulong *cs_base, uint32_t *flags) > { > *pc = env->nip; > - *cs_base = 0; > - *flags = env->hflags; > + *cs_base = env->hflags; > + *flags = 0; > } > > void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception); > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 0984ce637b..1eb2e1b0c6 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -7879,47 +7879,37 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > { > DisasContext *ctx = container_of(dcbase, DisasContext, base); > CPUPPCState *env = cs->env_ptr; > + target_ulong hflags = ctx->base.tb->cs_base; > int bound; > > ctx->exception = POWERPC_EXCP_NONE; > ctx->spr_cb = env->spr_cb; > - ctx->pr = msr_pr; > + ctx->pr = (hflags >> MSR_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; > + ctx->dr = (hflags >> MSR_DR) & 1; > +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) > + ctx->hv = (hflags >> MSR_HV) & 1; > #endif > 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 >> MSR_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 >> MSR_SF) & 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 >> MSR_FP) & 1; > + ctx->spe_enabled = (hflags >> MSR_SPE) & 1; > + ctx->altivec_enabled = (hflags >> MSR_VR) & 1; > + ctx->vsx_enabled = (hflags >> MSR_VSX) & 1; > + /* FIXME: This needs to be stored in env->hflags_nmsr. */ > if ((env->flags & POWERPC_FLAG_SCV) > && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) { > ctx->scv_enabled = true; > @@ -7927,23 +7917,21 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > 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; > - } > + ctx->tm_enabled = (hflags >> MSR_TM) & 1; > #endif > + /* FIXME: This needs to be stored in env->hflags_nmsr. */ > 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; > - } > - if ((env->flags & POWERPC_FLAG_BE) && msr_be) { > - ctx->singlestep_enabled |= CPU_BRANCH_STEP; > - } > - if ((env->flags & POWERPC_FLAG_DE) && msr_de) { > + > + ctx->singlestep_enabled = ((hflags >> MSR_SE) & 1 ? CPU_SINGLE_STEP : 0) > + | ((hflags >> MSR_BE) & 1 ? CPU_BRANCH_STEP : 0); > + > + if ((hflags >> MSR_DE) & 1) { > ctx->singlestep_enabled = 0; > + /* > + * FIXME: This needs to be stored in env->hflags_nmsr, > + * probably overlapping MSR_SE/MSR_BE like we do for > + * MSR_LE and the ppc 601. > + */ > target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0]; > if (dbcr0 & DBCR0_ICMP) { > ctx->singlestep_enabled |= CPU_SINGLE_STEP; > @@ -7956,10 +7944,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
Richard, Reviewed-by: Ivan Warren <ivan@vmfacility.fr> Thanks a million ! --Ivan On 1/24/2021 4:24 AM, Richard Henderson wrote: > Use the cs_base field, because it happens to be the same > size as hflags (and MSR, from which hflags is derived). > > In translate, extract most bits from a local hflags variable. > Mark several cases where code generation is *not* derived from > data stored within the hashed elements of the TranslationBlock. > > Cc: David Gibson <david@gibson.dropbear.id.au> > Reported-by: Ivan Warren <ivan@vmfacility.fr> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/ppc/cpu.h | 4 +-- > target/ppc/translate.c | 64 ++++++++++++++++-------------------------- > 2 files changed, 26 insertions(+), 42 deletions(-) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 2609e4082e..4a05e4e544 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -2396,8 +2396,8 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc, > target_ulong *cs_base, uint32_t *flags) > { > *pc = env->nip; > - *cs_base = 0; > - *flags = env->hflags; > + *cs_base = env->hflags; > + *flags = 0; > } > > void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception); > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 0984ce637b..1eb2e1b0c6 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -7879,47 +7879,37 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > { > DisasContext *ctx = container_of(dcbase, DisasContext, base); > CPUPPCState *env = cs->env_ptr; > + target_ulong hflags = ctx->base.tb->cs_base; > int bound; > > ctx->exception = POWERPC_EXCP_NONE; > ctx->spr_cb = env->spr_cb; > - ctx->pr = msr_pr; > + ctx->pr = (hflags >> MSR_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; > + ctx->dr = (hflags >> MSR_DR) & 1; > +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) > + ctx->hv = (hflags >> MSR_HV) & 1; > #endif > 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 >> MSR_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 >> MSR_SF) & 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 >> MSR_FP) & 1; > + ctx->spe_enabled = (hflags >> MSR_SPE) & 1; > + ctx->altivec_enabled = (hflags >> MSR_VR) & 1; > + ctx->vsx_enabled = (hflags >> MSR_VSX) & 1; > + /* FIXME: This needs to be stored in env->hflags_nmsr. */ > if ((env->flags & POWERPC_FLAG_SCV) > && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) { > ctx->scv_enabled = true; > @@ -7927,23 +7917,21 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > 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; > - } > + ctx->tm_enabled = (hflags >> MSR_TM) & 1; > #endif > + /* FIXME: This needs to be stored in env->hflags_nmsr. */ > 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; > - } > - if ((env->flags & POWERPC_FLAG_BE) && msr_be) { > - ctx->singlestep_enabled |= CPU_BRANCH_STEP; > - } > - if ((env->flags & POWERPC_FLAG_DE) && msr_de) { > + > + ctx->singlestep_enabled = ((hflags >> MSR_SE) & 1 ? CPU_SINGLE_STEP : 0) > + | ((hflags >> MSR_BE) & 1 ? CPU_BRANCH_STEP : 0); > + > + if ((hflags >> MSR_DE) & 1) { > ctx->singlestep_enabled = 0; > + /* > + * FIXME: This needs to be stored in env->hflags_nmsr, > + * probably overlapping MSR_SE/MSR_BE like we do for > + * MSR_LE and the ppc 601. > + */ > target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0]; > if (dbcr0 & DBCR0_ICMP) { > ctx->singlestep_enabled |= CPU_SINGLE_STEP; > @@ -7956,10 +7944,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);
On 1/23/21 6:46 PM, David Gibson wrote: > On Sat, Jan 23, 2021 at 05:24:22PM -1000, Richard Henderson wrote: >> Use the cs_base field, because it happens to be the same >> size as hflags (and MSR, from which hflags is derived). >> >> In translate, extract most bits from a local hflags variable. >> Mark several cases where code generation is *not* derived from >> data stored within the hashed elements of the TranslationBlock. > > My knowledge of TCG isn't great, so I'm pretty much prepared to accept > this is correct on your say so. > > But that commit message feels like it's following on from a > conversation that's not here, nor linked. It'd be great if it > explained how said hflags truncation is happening, because it's > certainly not obvious to someone with only a fair to middling > understanding of TCG. Mm, fair. How about: The assignment from env->hflags to tb->flags truncates target_ulong to uint32_t. This loses important bits from the top of hflags, which results in incorrect tb selection. Use the cs_base field instead, because it happens to be the same size as hflags (and MSR fom which hflags is derived). In translate, extract most bits from a local hflags variable. All of the checks vs env->flags are redundant with env->msr_mask in that msr bits cannot be set when the feature is not available. Mark several cases where code generation is *not* derived from data stored within hashed elements of the tb. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 1/23/21 6:46 PM, David Gibson wrote: >> On Sat, Jan 23, 2021 at 05:24:22PM -1000, Richard Henderson wrote: >>> Use the cs_base field, because it happens to be the same >>> size as hflags (and MSR, from which hflags is derived). >>> >>> In translate, extract most bits from a local hflags variable. >>> Mark several cases where code generation is *not* derived from >>> data stored within the hashed elements of the TranslationBlock. >> >> My knowledge of TCG isn't great, so I'm pretty much prepared to accept >> this is correct on your say so. >> >> But that commit message feels like it's following on from a >> conversation that's not here, nor linked. It'd be great if it >> explained how said hflags truncation is happening, because it's >> certainly not obvious to someone with only a fair to middling >> understanding of TCG. > > Mm, fair. > > How about: > > The assignment from env->hflags to tb->flags truncates > target_ulong to uint32_t. This loses important bits from > the top of hflags, which results in incorrect tb selection. We are just putting off the day we declare tb->flags to be 64 bit or end up renaming cs_base to tb->cs_base_or_extra_flag_bits_we_could_not_fit_in_flags. The fact that cs_base is expressed in terms of target_ulong worries me if there is ever any hflag state above bit 32 for the ppc32 targets. > > Use the cs_base field instead, because it happens to be the > same size as hflags (and MSR fom which hflags is derived). > > In translate, extract most bits from a local hflags variable. > All of the checks vs env->flags are redundant with env->msr_mask > in that msr bits cannot be set when the feature is not available. > Mark several cases where code generation is *not* derived from > data stored within hashed elements of the tb. > > > r~ -- Alex Bennée
On Sun, Jan 24, 2021 at 09:38:04AM -1000, Richard Henderson wrote: > On 1/23/21 6:46 PM, David Gibson wrote: > > On Sat, Jan 23, 2021 at 05:24:22PM -1000, Richard Henderson wrote: > >> Use the cs_base field, because it happens to be the same > >> size as hflags (and MSR, from which hflags is derived). > >> > >> In translate, extract most bits from a local hflags variable. > >> Mark several cases where code generation is *not* derived from > >> data stored within the hashed elements of the TranslationBlock. > > > > My knowledge of TCG isn't great, so I'm pretty much prepared to accept > > this is correct on your say so. > > > > But that commit message feels like it's following on from a > > conversation that's not here, nor linked. It'd be great if it > > explained how said hflags truncation is happening, because it's > > certainly not obvious to someone with only a fair to middling > > understanding of TCG. > > Mm, fair. > > How about: > > The assignment from env->hflags to tb->flags truncates > target_ulong to uint32_t. This loses important bits from > the top of hflags, which results in incorrect tb selection. > > Use the cs_base field instead, because it happens to be the > same size as hflags (and MSR fom which hflags is derived). > > In translate, extract most bits from a local hflags variable. > All of the checks vs env->flags are redundant with env->msr_mask > in that msr bits cannot be set when the feature is not available. > Mark several cases where code generation is *not* derived from > data stored within hashed elements of the tb. Thanks, I've applied the patch with the updated description. > > > r~ > -- 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
On Sat, Jan 23, 2021 at 05:24:22PM -1000, Richard Henderson wrote: > Use the cs_base field, because it happens to be the same > size as hflags (and MSR, from which hflags is derived). > > In translate, extract most bits from a local hflags variable. > Mark several cases where code generation is *not* derived from > data stored within the hashed elements of the TranslationBlock. > > Cc: David Gibson <david@gibson.dropbear.id.au> > Reported-by: Ivan Warren <ivan@vmfacility.fr> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> Well, I don't know why, but somehow this patch is breaking one of the acceptance tests: (043/134) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_ppc64_e500: INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout reached\nOriginal status: ERROR\n{'name': '043-tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_ppc64_e500', 'logdir': '/home/dwg/src/qemu/build/normal/tests/results/job-2021-02-10T15.04... (90.26 s) From that timeout, I'm guessing something about this is causing the boot to wedge. So, I've removed this from my tree for now, I'll need a fixed version to proceed with. > --- > target/ppc/cpu.h | 4 +-- > target/ppc/translate.c | 64 ++++++++++++++++-------------------------- > 2 files changed, 26 insertions(+), 42 deletions(-) > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 2609e4082e..4a05e4e544 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -2396,8 +2396,8 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc, > target_ulong *cs_base, uint32_t *flags) > { > *pc = env->nip; > - *cs_base = 0; > - *flags = env->hflags; > + *cs_base = env->hflags; > + *flags = 0; > } > > void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception); > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 0984ce637b..1eb2e1b0c6 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -7879,47 +7879,37 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > { > DisasContext *ctx = container_of(dcbase, DisasContext, base); > CPUPPCState *env = cs->env_ptr; > + target_ulong hflags = ctx->base.tb->cs_base; > int bound; > > ctx->exception = POWERPC_EXCP_NONE; > ctx->spr_cb = env->spr_cb; > - ctx->pr = msr_pr; > + ctx->pr = (hflags >> MSR_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; > + ctx->dr = (hflags >> MSR_DR) & 1; > +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) > + ctx->hv = (hflags >> MSR_HV) & 1; > #endif > 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 >> MSR_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 >> MSR_SF) & 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 >> MSR_FP) & 1; > + ctx->spe_enabled = (hflags >> MSR_SPE) & 1; > + ctx->altivec_enabled = (hflags >> MSR_VR) & 1; > + ctx->vsx_enabled = (hflags >> MSR_VSX) & 1; > + /* FIXME: This needs to be stored in env->hflags_nmsr. */ > if ((env->flags & POWERPC_FLAG_SCV) > && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) { > ctx->scv_enabled = true; > @@ -7927,23 +7917,21 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) > 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; > - } > + ctx->tm_enabled = (hflags >> MSR_TM) & 1; > #endif > + /* FIXME: This needs to be stored in env->hflags_nmsr. */ > 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; > - } > - if ((env->flags & POWERPC_FLAG_BE) && msr_be) { > - ctx->singlestep_enabled |= CPU_BRANCH_STEP; > - } > - if ((env->flags & POWERPC_FLAG_DE) && msr_de) { > + > + ctx->singlestep_enabled = ((hflags >> MSR_SE) & 1 ? CPU_SINGLE_STEP : 0) > + | ((hflags >> MSR_BE) & 1 ? CPU_BRANCH_STEP : 0); > + > + if ((hflags >> MSR_DE) & 1) { > ctx->singlestep_enabled = 0; > + /* > + * FIXME: This needs to be stored in env->hflags_nmsr, > + * probably overlapping MSR_SE/MSR_BE like we do for > + * MSR_LE and the ppc 601. > + */ > target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0]; > if (dbcr0 & DBCR0_ICMP) { > ctx->singlestep_enabled |= CPU_SINGLE_STEP; > @@ -7956,10 +7944,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
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h index 2609e4082e..4a05e4e544 100644 --- a/target/ppc/cpu.h +++ b/target/ppc/cpu.h @@ -2396,8 +2396,8 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc, target_ulong *cs_base, uint32_t *flags) { *pc = env->nip; - *cs_base = 0; - *flags = env->hflags; + *cs_base = env->hflags; + *flags = 0; } void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception); diff --git a/target/ppc/translate.c b/target/ppc/translate.c index 0984ce637b..1eb2e1b0c6 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -7879,47 +7879,37 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) { DisasContext *ctx = container_of(dcbase, DisasContext, base); CPUPPCState *env = cs->env_ptr; + target_ulong hflags = ctx->base.tb->cs_base; int bound; ctx->exception = POWERPC_EXCP_NONE; ctx->spr_cb = env->spr_cb; - ctx->pr = msr_pr; + ctx->pr = (hflags >> MSR_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; + ctx->dr = (hflags >> MSR_DR) & 1; +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) + ctx->hv = (hflags >> MSR_HV) & 1; #endif 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 >> MSR_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 >> MSR_SF) & 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 >> MSR_FP) & 1; + ctx->spe_enabled = (hflags >> MSR_SPE) & 1; + ctx->altivec_enabled = (hflags >> MSR_VR) & 1; + ctx->vsx_enabled = (hflags >> MSR_VSX) & 1; + /* FIXME: This needs to be stored in env->hflags_nmsr. */ if ((env->flags & POWERPC_FLAG_SCV) && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) { ctx->scv_enabled = true; @@ -7927,23 +7917,21 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs) 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; - } + ctx->tm_enabled = (hflags >> MSR_TM) & 1; #endif + /* FIXME: This needs to be stored in env->hflags_nmsr. */ 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; - } - if ((env->flags & POWERPC_FLAG_BE) && msr_be) { - ctx->singlestep_enabled |= CPU_BRANCH_STEP; - } - if ((env->flags & POWERPC_FLAG_DE) && msr_de) { + + ctx->singlestep_enabled = ((hflags >> MSR_SE) & 1 ? CPU_SINGLE_STEP : 0) + | ((hflags >> MSR_BE) & 1 ? CPU_BRANCH_STEP : 0); + + if ((hflags >> MSR_DE) & 1) { ctx->singlestep_enabled = 0; + /* + * FIXME: This needs to be stored in env->hflags_nmsr, + * probably overlapping MSR_SE/MSR_BE like we do for + * MSR_LE and the ppc 601. + */ target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0]; if (dbcr0 & DBCR0_ICMP) { ctx->singlestep_enabled |= CPU_SINGLE_STEP; @@ -7956,10 +7944,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);
Use the cs_base field, because it happens to be the same size as hflags (and MSR, from which hflags is derived). In translate, extract most bits from a local hflags variable. Mark several cases where code generation is *not* derived from data stored within the hashed elements of the TranslationBlock. Cc: David Gibson <david@gibson.dropbear.id.au> Reported-by: Ivan Warren <ivan@vmfacility.fr> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/ppc/cpu.h | 4 +-- target/ppc/translate.c | 64 ++++++++++++++++-------------------------- 2 files changed, 26 insertions(+), 42 deletions(-) -- 2.25.1