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

[Fabiano Rosas <farosas@linux.ibm.com>]

Cédric Le Goater <clg@kaod.org> writes:

> 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.

Ok, I'm sending another series with just these 4 + the bounds check
Richard mentioned.

>
> 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.

Agreed.

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

The issue here is that we would probably be switching one arbitrary
identifier for another. I don't think we have a lightweight canonical
way of identifying a CPU or group of CPUs. But maybe having these
conditionals on a specific CPU should be considered a hack to begin
with.

>
> 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.

Ack all of this. I'm working on it.

Thank you for the inputs.