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);
+
pcc->mmu_model = POWERPC_MMU_SOFT_4xx;
pcc->excp_model = POWERPC_EXCP_40x;
pcc->bus_model = PPC_FLAGS_INPUT_405;
--
2.33.1
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; -- 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 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; > -- 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
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.
--
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
© 2016 - 2026 Red Hat, Inc.