Re: [PATCH 1/8] target/ppc: 405: Add missing MSR bits to msr_mask

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3434 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]