| Message ID | 20220110181546.4131853-2-farosas@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/ppc: powerpc_excp improvements [40x] (3/n) | expand |
On Mon, Jan 10, 2022 at 03:15:39PM -0300, Fabiano Rosas wrote: > Some bits described in the user manual are missing from msr_mask. Add > them. > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > --- > target/ppc/cpu_init.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > index e30e86fe9d..a50ddaeaae 100644 > --- a/target/ppc/cpu_init.c > +++ b/target/ppc/cpu_init.c > @@ -2535,15 +2535,19 @@ POWERPC_FAMILY(405)(ObjectClass *oc, void *data) > PPC_MEM_SYNC | PPC_MEM_EIEIO | > PPC_40x_TLB | PPC_MEM_TLBIA | PPC_MEM_TLBSYNC | > PPC_4xx_COMMON | PPC_405_MAC | PPC_40x_EXCP; > - pcc->msr_mask = (1ull << MSR_POW) | > + pcc->msr_mask = (1ull << MSR_AP) | > + (1ull << MSR_POW) | If I'm looking at things correctly, the "MSR_POW" bit on 405 is actually called "MSR_WE", which appears related, but not quite identical. I think it would be good to introduce a new define to get it a name matching the user manual. > (1ull << MSR_CE) | > (1ull << MSR_EE) | > (1ull << MSR_PR) | > (1ull << MSR_FP) | > + (1ull << MSR_ME) | > (1ull << MSR_DWE) | > (1ull << MSR_DE) | > + (1ull << MSR_FE1) | > (1ull << MSR_IR) | > (1ull << MSR_DR); > + > pcc->mmu_model = POWERPC_MMU_SOFT_4xx; > pcc->excp_model = POWERPC_EXCP_40x; > pcc->bus_model = PPC_FLAGS_INPUT_405;
On Tue, Jan 11, 2022 at 01:04:14PM +1100, David Gibson wrote: > On Mon, Jan 10, 2022 at 03:15:39PM -0300, Fabiano Rosas wrote: > > Some bits described in the user manual are missing from msr_mask. Add > > them. > > > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > > --- > > target/ppc/cpu_init.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > > index e30e86fe9d..a50ddaeaae 100644 > > --- a/target/ppc/cpu_init.c > > +++ b/target/ppc/cpu_init.c > > @@ -2535,15 +2535,19 @@ POWERPC_FAMILY(405)(ObjectClass *oc, void *data) > > PPC_MEM_SYNC | PPC_MEM_EIEIO | > > PPC_40x_TLB | PPC_MEM_TLBIA | PPC_MEM_TLBSYNC | > > PPC_4xx_COMMON | PPC_405_MAC | PPC_40x_EXCP; > > - pcc->msr_mask = (1ull << MSR_POW) | > > + pcc->msr_mask = (1ull << MSR_AP) | > > + (1ull << MSR_POW) | > > If I'm looking at things correctly, the "MSR_POW" bit on 405 is > actually called "MSR_WE", which appears related, but not quite > identical. I think it would be good to introduce a new define to get > it a name matching the user manual. Also, it looks like this is still missing the MSR[APE] bit (in the same place as the MSR_KEY bit on 603e). > > (1ull << MSR_CE) | > > (1ull << MSR_EE) | > > (1ull << MSR_PR) | > > (1ull << MSR_FP) | > > + (1ull << MSR_ME) | > > (1ull << MSR_DWE) | > > (1ull << MSR_DE) | > > + (1ull << MSR_FE1) | > > (1ull << MSR_IR) | > > (1ull << MSR_DR); > > + > > pcc->mmu_model = POWERPC_MMU_SOFT_4xx; > > pcc->excp_model = POWERPC_EXCP_40x; > > pcc->bus_model = PPC_FLAGS_INPUT_405; >
Fabiano Rosas <farosas@linux.ibm.com> writes: > Some bits described in the user manual are missing from msr_mask. Add > them. > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > --- > target/ppc/cpu_init.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > index e30e86fe9d..a50ddaeaae 100644 > --- a/target/ppc/cpu_init.c > +++ b/target/ppc/cpu_init.c > @@ -2535,15 +2535,19 @@ POWERPC_FAMILY(405)(ObjectClass *oc, void *data) > PPC_MEM_SYNC | PPC_MEM_EIEIO | > PPC_40x_TLB | PPC_MEM_TLBIA | PPC_MEM_TLBSYNC | > PPC_4xx_COMMON | PPC_405_MAC | PPC_40x_EXCP; > - pcc->msr_mask = (1ull << MSR_POW) | > + pcc->msr_mask = (1ull << MSR_AP) | > + (1ull << MSR_POW) | > (1ull << MSR_CE) | > (1ull << MSR_EE) | > (1ull << MSR_PR) | > (1ull << MSR_FP) | > + (1ull << MSR_ME) | > (1ull << MSR_DWE) | > (1ull << MSR_DE) | > + (1ull << MSR_FE1) | > (1ull << MSR_IR) | > (1ull << MSR_DR); This patch brings an unexpected complication: MSR_AP here is not correct, it is defined as: #define MSR_AP 23 /* Access privilege state on 602 */ That is bit 8. While MSR_AP in the 405 is bit 6. So I would need to introduce a new MSR_AP_405 defined as: #define MSR_AP_405 25 /* Auxiliar processor available on 405 */ But 25 is the same as MSR_SPE, so it triggers this code in init_ppc_proc: /* MSR bits & flags consistency checks */ if (env->msr_mask & (1 << 25)) { switch (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) { case POWERPC_FLAG_SPE: case POWERPC_FLAG_VRE: break; default: fprintf(stderr, "PowerPC MSR definition inconsistency\n" "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE\n"); exit(1); } ... The commit that introduced that sanity check is 25ba3a6812 ("Remove synonymous in PowerPC MSR bits definitions..."), which sort of assumes that MSR bits will not have different purposes between any of the (now 47) CPUs, while itself leaving other duplicated bits around. So my idea is to drop this patch and only include the MSR_ME that is of practical effect at patch 6. I think going into the rabbit hole of disambiguating MSR bits falls out of the scope of the exception series.
On Mon, Jan 17, 2022 at 06:12:20PM -0300, Fabiano Rosas wrote: > Fabiano Rosas <farosas@linux.ibm.com> writes: > > > Some bits described in the user manual are missing from msr_mask. Add > > them. > > > > Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> > > --- > > target/ppc/cpu_init.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > > index e30e86fe9d..a50ddaeaae 100644 > > --- a/target/ppc/cpu_init.c > > +++ b/target/ppc/cpu_init.c > > @@ -2535,15 +2535,19 @@ POWERPC_FAMILY(405)(ObjectClass *oc, void *data) > > PPC_MEM_SYNC | PPC_MEM_EIEIO | > > PPC_40x_TLB | PPC_MEM_TLBIA | PPC_MEM_TLBSYNC | > > PPC_4xx_COMMON | PPC_405_MAC | PPC_40x_EXCP; > > - pcc->msr_mask = (1ull << MSR_POW) | > > + pcc->msr_mask = (1ull << MSR_AP) | > > + (1ull << MSR_POW) | > > (1ull << MSR_CE) | > > (1ull << MSR_EE) | > > (1ull << MSR_PR) | > > (1ull << MSR_FP) | > > + (1ull << MSR_ME) | > > (1ull << MSR_DWE) | > > (1ull << MSR_DE) | > > + (1ull << MSR_FE1) | > > (1ull << MSR_IR) | > > (1ull << MSR_DR); > > This patch brings an unexpected complication: > > MSR_AP here is not correct, it is defined as: > > #define MSR_AP 23 /* Access privilege state on 602 */ > > That is bit 8. While MSR_AP in the 405 is bit 6. So I would need to Uh oh... > introduce a new MSR_AP_405 defined as: > > #define MSR_AP_405 25 /* Auxiliar processor available on 405 */ > > But 25 is the same as MSR_SPE, so it triggers this code in > init_ppc_proc: > > /* MSR bits & flags consistency checks */ > if (env->msr_mask & (1 << 25)) { > switch (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) { > case POWERPC_FLAG_SPE: > case POWERPC_FLAG_VRE: > break; > default: > fprintf(stderr, "PowerPC MSR definition inconsistency\n" > "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE\n"); > exit(1); > } > ... Ah. Which suggests this section itself should probably be taken out of the common path and moved to code specific to the cpu families with SPE. > The commit that introduced that sanity check is 25ba3a6812 ("Remove > synonymous in PowerPC MSR bits definitions..."), which sort of assumes > that MSR bits will not have different purposes between any of the (now > 47) CPUs, while itself leaving other duplicated bits around. Ah, oops. Even in 2007 I could have told you that wasn't a safe assumption. Oh well. > So my idea is to drop this patch and only include the MSR_ME that is of > practical effect at patch 6. I think going into the rabbit hole of > disambiguating MSR bits falls out of the scope of the exception series. That seems fair for the time being. I suspect splitting the exception paths will make cleaning up MSR collisions like this easier.
diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c index e30e86fe9d..a50ddaeaae 100644 --- a/target/ppc/cpu_init.c +++ b/target/ppc/cpu_init.c @@ -2535,15 +2535,19 @@ POWERPC_FAMILY(405)(ObjectClass *oc, void *data) PPC_MEM_SYNC | PPC_MEM_EIEIO | PPC_40x_TLB | PPC_MEM_TLBIA | PPC_MEM_TLBSYNC | PPC_4xx_COMMON | PPC_405_MAC | PPC_40x_EXCP; - pcc->msr_mask = (1ull << MSR_POW) | + pcc->msr_mask = (1ull << MSR_AP) | + (1ull << MSR_POW) | (1ull << MSR_CE) | (1ull << MSR_EE) | (1ull << MSR_PR) | (1ull << MSR_FP) | + (1ull << MSR_ME) | (1ull << MSR_DWE) | (1ull << MSR_DE) | + (1ull << MSR_FE1) | (1ull << MSR_IR) | (1ull << MSR_DR); + pcc->mmu_model = POWERPC_MMU_SOFT_4xx; pcc->excp_model = POWERPC_EXCP_40x; pcc->bus_model = PPC_FLAGS_INPUT_405;
Some bits described in the user manual are missing from msr_mask. Add them. Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com> --- target/ppc/cpu_init.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)