On Fri, 6 Oct 2017 09:08:45 +0200
Thomas Huth <th...@redhat.com> wrote:
> On 28.09.2017 22:36, David Hildenbrand wrote:
> > External interrupts are currently all handled like floating external
> > interrupts, they are queued. Let's prepare for a split of floating
> > and local interrupts by turning INTERRUPT_EXT into a mask.
> >
> > While we can have various floating external interrupts of one kind, there
> > is usually only one (or a fixed number) of the local external interrupts.
> >
> > So turn INTERRUPT_EXT into a mask and properly indicate the kind of
> > external interrupt. Floating interrupts will have to moved out of
> > one CPU instance later once we have SMP support.
> >
> > The only floating external interrupts used right now are SERVICE
> > interrupts, so let's use that name. Following patches will clean up
> > SERVICE interrupt injection.
> >
> > This get's rid of the ugly special handling for cpu timer and clock
> > comparator interrupts. And we really only store the parameters as
> > defined by the PoP.
> >
> > Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
> > Signed-off-by: David Hildenbrand <da...@redhat.com>
> > ---
> > target/s390x/cpu.h | 13 ++++++----
> > target/s390x/excp_helper.c | 63
> > +++++++++++++++++++++++-----------------------
> > target/s390x/helper.c | 12 ++-------
> > target/s390x/internal.h | 2 ++
> > target/s390x/interrupt.c | 18 ++++++++++++-
> > 5 files changed, 61 insertions(+), 47 deletions(-)
> [...]
> > diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c
> > index 058e219fe5..b9c30f86d7 100644
> > --- a/target/s390x/interrupt.c
> > +++ b/target/s390x/interrupt.c
> > @@ -71,7 +71,23 @@ void cpu_inject_ext(S390CPU *cpu, uint32_t code,
> > uint32_t param,
> > env->ext_queue[env->ext_index].param = param;
> > env->ext_queue[env->ext_index].param64 = param64;
> >
> > - env->pending_int |= INTERRUPT_EXT;
> > + env->pending_int |= INTERRUPT_EXT_SERVICE;
> > + cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
> > +}
> > +
> > +void cpu_inject_clock_comparator(S390CPU *cpu)
> > +{
> > + CPUS390XState *env = &cpu->env;
> > +
> > + env->pending_int |= INTERRUPT_EXT_CLOCK_COMPARATOR;
> > + cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
> > +}
> > +
> > +void cpu_inject_cpu_timer(S390CPU *cpu)
> > +{
> > + CPUS390XState *env = &cpu->env;
> > +
> > + env->pending_int |= INTERRUPT_EXT_CPU_TIMER;
> > cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
> > }
>
> The last two function look similar enough that you could merge the
> functions, e.g.:
>
> void cpu_inject_ext_pending_bit(S390CPU *cpu, int bit)
> {
> CPUS390XState *env = &cpu->env;
>
> env->pending_int |= bit;
> cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
> }
>
> ?
>
> Apart from that, the patch looks fine to me.
>
> Thomas
FWIW, I'd prefer to keep these as separate functions.
