Re: [RFC v2 00/12] target/ppc: powerpc_excp improvements

[Cédric Le Goater]

Hello Fabiano,

On 12/20/21 19:18, Fabiano Rosas wrote:
> This changed a lot since v1, basically what remains is the idea that
> we want to have some sort of array of interrupts and some sort of
> separation between processors.
>
> At the end of this series we'll have:
>
> - One file with all interrupt implementations (interrupts.c);
>
> - Separate files for each major group of CPUs (book3s, booke,
>    32bits). Only interrupt code for now, but we could bring pieces of
>    cpu_init into them;
>
> - Four separate interrupt arrays, one for each of the above groups
>    plus KVM.
>
> - powerpc_excp calls into the individual files and from there we
>    dispatch according to what is available in the interrupts array.


This is going in the good direction. I think we need more steps for
the reviewers, for tests and bisectability. First 4 patches are OK
and I hope to merge them ASAP.

The powerpc_excp() routine has grown nearly out of control these last
years and it is becoming difficult to maintain. The goal is to clarify
what it is going on for each CPU or each CPU family. The first step
consists basically in duplicating the code and moving the exceptions
handlers in specific routines.

1. cleanups should come first as usual.

2. isolate large chunks, like Nick did with ppc_excp_apply_ail().
   We could do easily the same for :

   2.1 ILE
   2.2 unimplemeted ones doing a cpu abort:
    
         cpu_abort(cs, ".... "  "is not implemented yet !\n");
   2.3 6x TLBS

   This should reduce considerably powerpc_excp() without changing too
   much the execution path.

3. Cleanup the use of excp_model, like in dcbz_common() and kvm.
   This is not critical but some are shortcuts.

4. Introduce a new powerpc_excp() handler :

   static void powerpc_excp(PowerPCCPU *cpu, int excp)
   {
       switch(env->excp_model) {
       case POWERPC_EXCP_FOO1:
       case POWERPC_EXCP_FOO2:
           powerpc_excp_foo(cpu, excp);
           break;
       case POWERPC_EXCP_BAR:
           powerpc_excp_legacy(cpu, excp);
           break;
       default:
           g_assert_not_reached();
       }
   }

   and start duplicating code cpu per cpu in specific excp handlers, avoiding
   as much as possible the use of excp_model in the powerpc_excp_*() routines.
   That's for the theory.

   I suppose these can be grouped in the following way :

   * 405 CPU
        POWERPC_EXCP_40x,

   * 6xx CPUs
        POWERPC_EXCP_601,
        POWERPC_EXCP_602,
        POWERPC_EXCP_603,
        POWERPC_EXCP_G2,
        POWERPC_EXCP_604,
        
   * 7xx CPUs
        POWERPC_EXCP_7x0,
        POWERPC_EXCP_7x5,
        POWERPC_EXCP_74xx,
        
   * BOOKE CPUs
        POWERPC_EXCP_BOOKE,

   * BOOKS CPUs
        POWERPC_EXCP_970,            /* could be special */
        POWERPC_EXCP_POWER7,
        POWERPC_EXCP_POWER8,
        POWERPC_EXCP_POWER9,
        POWERPC_EXCP_POWER10,
    
   If not possible, then, we will duplicate more and that's not a problem.

   I would keep the routines in the same excp_helper.c file for now; we
   can move the code in different files but I would do it later and with
   other components in mind and not just the exception models. book3s,
   booke, 7xx, 6xx, 405 are the different groups. It fits what you did.
   
5. Once done, get rid of powerpc_excp_legacy()

6. Start looking at refactoring again.

   There might be a common prologue and epilogue. As a consequence we could
   change the args passed to powerpc_excp_*().

   There could be common handlers and that's why an array of exception
   handlers looks good. this is what you are trying to address after patch 5
   but I would prefer to do the above steps before.

Thanks,

C.