Re: [PATCH v5 2/9] target/ppc: Add IBM PPE42 family of processors

[Miles Glenn <milesg@linux.ibm.com>]

On Thu, 2025-09-25 at 10:27 +0530, Harsh Prateek Bora wrote:
> Hi Glenn,
> 
> On 9/24/25 20:36, Miles Glenn wrote:
> > > > > @@ -6802,53 +6916,63 @@ static void init_ppc_proc(PowerPCCPU *cpu)
> > > > >    
> > > > >        /* MSR bits & flags consistency checks */
> > > > >        if (env->msr_mask & (1 << 25)) {
> > > > > -        switch (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
> > > > > +        switch (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE |
> > > > > +                              POWERPC_FLAG_PPE42)) {
> > > > >            case POWERPC_FLAG_SPE:
> > > > >            case POWERPC_FLAG_VRE:
> > > > > +        case POWERPC_FLAG_PPE42:
> > > > >                break;
> > > > >            default:
> > > > >                fprintf(stderr, "PowerPC MSR definition inconsistency\n"
> > > > > -                    "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE\n");
> > > > > +                    "Should define POWERPC_FLAG_SPE or POWERPC_FLAG_VRE\n"
> > > > > +                    "or POWERPC_FLAG_PPE42\n");
> > > > >                exit(1);
> > > > >            }
> > > > >        } else if (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
> > > > Hey Glenn,
> > > > 
> > > > Did you miss adding the POWERPC_FLAG_PPE42 flag here  ^  ?
> > > > 
> > > > Thanks,
> > > > Chinmay
> > > No. All PPE42 processors will have bit 1 << 25 set in env->msr_mask, so
> > > it will always fall into the previous condition block and never enter
> > > the 2nd check.
> > > 
> > > Glenn
> > > 
> > Ah, sorry, I should have looked closer!  This is supposed to be
> > checking that if 1 << 25 is not set that we shouldn't be setting the
> > PPE42 flag either.  So, yes, I'll add that in v6.
> 
> While we are at it, can we also replace all hard-coded bit shifts with
> appropriate macros which reflect what these shifts are about. There are
> few more such checks in the patch. May be audit other patches as well
> for such instances.
> 
> regards
> Harsh
> 

Hi Harsh,

Normally I would agree with you, but I think that all of the hard-coded 
bit shifts in this function (init_ppc_proc) are hard-coded because the
MSR bits have multiple meanings depending on the CPU and this function
is called on all PPC CPUs.  So, in this context, I think that using the
hard-coded bit number is appropriate and this is probably why it has
remained as a hard-coded value in this function since 2007.

That being said, if you still feel strongly that these hard-coded
values should be replaced with macros, could you provide suggestions on
what would be appropriate names in this function?

Thanks,

Glenn


> > Thanks,
> > 
> > Glenn