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

[David Gibson]

On Mon, Jun 07, 2021 at 01:54:15PM -0300, Fabiano Rosas wrote:
> 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;

Technically, yes, but my experience with this approach is that because
many of the interrupts have similar, but not quite identical behaviour
in different models, the "common" interrupt path is so littered with
special cases and per-model tests that they become very hard to read.
It also makes things fragile, because it's difficult to refactor or
update things for one model without risking breaking some other model
for an obscure CPU variant you barely know and aren't thinking about
at the time.

Basically the reason I think things look at mess at the moment, is
that historically we've tried to share the implementation for things
that aren't quite similar enough for that to really work well.

> (ii) makes each model and its
> interrupts into one comprehensive unit; (iii) avoids duplication of the
> generic code.

Note that with model-then-exception it's still possible and encouraged
for the model specific code to call back to a common "library"
implementation for things that really do have the same logic between
models.  For simple things like system reset the per-model versions
could well just be stubs calling back to generic code.

But for the complex MMU interrupts, which have heaps of variants and
different cases, I think model-then-exception will make following the
logic much easier.

> 
> >> +
> >> +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;
> >> +};
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature