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

[Thomas Huth]

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