Re: [RFC PATCH 5/5] target/ppc: powerpc_excp: Move interrupt raising code to QOM

[Fabiano Rosas]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Tue, Jun 01, 2021 at 06:46:49PM -0300, Fabiano Rosas wrote:
>> +typedef void (*ppc_intr_fn_t)(PowerPCCPU *cpu, PPCInterrupt *intr,
>> +                              int excp_model, ppc_intr_args *regs,
>> +                              bool *ignore);
>
> Hmm.  Using this signature kind of enforces that we dispatch based on
> which exception *then* then the exception model.  I think that's
> backwards: since what vectors exist and make sense depends on the
> exception model, I think we should ideally be splitting on model
> first, then exception type.
>
> Now, a lot of the existing code is exception-then-model and changing
> that is a long term project, but I don't think we should lock
> ourselves further into doing it the backwards way.

Ok, so assuming one C file per exception model, I see three options:

 i) exception --> model (current):

 Interrupt code separate from models. One implementation for each
 interrupt that takes the model as argument. Models opt-in which
 interrupts they want (according to the ISA).

 ii) model --> exception:

 Interrupt code inside each model file. The model implements only the
 interrupts which exist (according to ISA). There would be duplication
 since several models would implement the same system reset, machine
 check, program, etc.

 iii) model --> exception w/ generic interrupts:

 Generic interrupt code separate from models. One implementation for
 each generic interrupt. Models opt-in which interrupts they want
 (according to the ISA). Models override generic implementation with
 model-specific ones.

Option (i) leads to the most code reuse; (ii) makes each model and its
interrupts into one comprehensive unit; (iii) avoids duplication of the
generic code.

>> +
>> +struct ppc_intr_args {
>> +    target_ulong nip;
>> +    target_ulong msr;
>> +    target_ulong new_nip;
>> +    target_ulong new_msr;
>> +    int sprn_srr0;
>> +    int sprn_srr1;
>> +    int sprn_asrr0;
>> +    int sprn_asrr1;
>> +    int lev;
>> +};
>> +
>> +struct PPCInterrupt {
>
> Having an info/dispatch structure for each vector makes sense..
>
>> +    Object parent;
>
> ..but making it a QOM object really seems like overkill.  In fact
> making it a QOM object at least somewhat exposes the internal
> structure to the user via QMP, which I really don't think we want to
> do.

I'm using QOM code mainly to facilitate the id->function mapping. I'll
remove the QOM layer and implement my own.

>> +
>> +    int id;
>> +    const char *name;
>> +    target_ulong addr;
>> +    ppc_intr_fn_t setup_regs;
>> +};