Re: [Qemu-devel] [PATCH 41/50] s390x: move stuff out of cpu.h

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Fri,  8 Apr 2016 22:29:01 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/s390x/css.c                              |   4 +-
>  hw/s390x/s390-skeys.c                       |   1 +
>  hw/s390x/s390-virtio-ccw.c                  |   4 +-
>  hw/s390x/virtio-ccw.c                       |   5 +-
>  hw/s390x/virtio-ccw.h                       |   3 +-
>  {hw => include/hw}/s390x/css.h              |  31 +++++++-
>  {target-s390x => include/hw/s390x}/ioinst.h |  16 +---
>  target-s390x/cpu.h                          | 117 ++++++----------------------
>  target-s390x/helper.c                       |   1 +
>  target-s390x/interrupt.c                    |  64 +++++++++++++++
>  target-s390x/ioinst.c                       |   2 +-
>  11 files changed, 131 insertions(+), 117 deletions(-)
>  rename {hw => include/hw}/s390x/css.h (76%)
>  rename {target-s390x => include/hw/s390x}/ioinst.h (87%)
> 

<did not review, but noticed the below:>

> diff --git a/target-s390x/interrupt.c b/target-s390x/interrupt.c
> index bad60a7..a13fd26 100644
> --- a/target-s390x/interrupt.c
> +++ b/target-s390x/interrupt.c
> @@ -10,6 +10,7 @@
>  #include "qemu/osdep.h"
>  #include "cpu.h"
>  #include "sysemu/kvm.h"
> +#include "hw/s390x/ioinst.h"
> 
>  /*
>   * All of the following interrupts are floating, i.e. not per-vcpu.

This used to be true before...

> @@ -17,6 +18,69 @@
>   * non-kvm case.
>   */
>  #if !defined(CONFIG_USER_ONLY)
> +void cpu_inject_ext(S390CPU *cpu, uint32_t code, uint32_t param,
> +                    uint64_t param64)

...but this one can be both floating and per-vcpu. Need to adust the
comment above?

> +{
> +    CPUS390XState *env = &cpu->env;
> +
> +    if (env->ext_index == MAX_EXT_QUEUE - 1) {
> +        /* ugh - can't queue anymore. Let's drop. */
> +        return;
> +    }
> +
> +    env->ext_index++;
> +    assert(env->ext_index < MAX_EXT_QUEUE);
> +
> +    env->ext_queue[env->ext_index].code = code;
> +    env->ext_queue[env->ext_index].param = param;
> +    env->ext_queue[env->ext_index].param64 = param64;
> +
> +    env->pending_int |= INTERRUPT_EXT;
> +    cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
> +}
> +
> +void cpu_inject_io(S390CPU *cpu, uint16_t subchannel_id,
> +                   uint16_t subchannel_number,
> +                   uint32_t io_int_parm, uint32_t io_int_word)
> +{

This one...

> +    CPUS390XState *env = &cpu->env;
> +    int isc = IO_INT_WORD_ISC(io_int_word);
> +
> +    if (env->io_index[isc] == MAX_IO_QUEUE - 1) {
> +        /* ugh - can't queue anymore. Let's drop. */
> +        return;
> +    }
> +
> +    env->io_index[isc]++;
> +    assert(env->io_index[isc] < MAX_IO_QUEUE);
> +
> +    env->io_queue[env->io_index[isc]][isc].id = subchannel_id;
> +    env->io_queue[env->io_index[isc]][isc].nr = subchannel_number;
> +    env->io_queue[env->io_index[isc]][isc].parm = io_int_parm;
> +    env->io_queue[env->io_index[isc]][isc].word = io_int_word;
> +
> +    env->pending_int |= INTERRUPT_IO;
> +    cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
> +}
> +
> +void cpu_inject_crw_mchk(S390CPU *cpu)
> +{

...and this one are always floating and therefore should only be called
from this file anyway (so they can be made static).

> +    CPUS390XState *env = &cpu->env;
> +
> +    if (env->mchk_index == MAX_MCHK_QUEUE - 1) {
> +        /* ugh - can't queue anymore. Let's drop. */
> +        return;
> +    }
> +
> +    env->mchk_index++;
> +    assert(env->mchk_index < MAX_MCHK_QUEUE);
> +
> +    env->mchk_queue[env->mchk_index].type = 1;
> +
> +    env->pending_int |= INTERRUPT_MCHK;
> +    cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
> +}
> +
>  void s390_sclp_extint(uint32_t parm)
>  {
>      if (kvm_enabled()) {