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

[Harsh Prateek Bora]

On 9/25/25 20:59, Miles Glenn wrote:

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.

Oh I see, in that case, let's keep it as-is.

Thanks
Harsh

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