Cédric Le Goater <c...@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.
