Re: [Qemu-devel] [PATCH v2 01/30] s390x/tcg: turn INTERRUPT_EXT into a mask

[Cornelia Huck]

On Fri, 6 Oct 2017 09:08:45 +0200
Thomas Huth <address@hidden> 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 <address@hidden>
> > Signed-off-by: David Hildenbrand <address@hidden>
> > ---
> >  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.