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
