Re: [PATCH] s390/kvm: fix diag318 propagation and reset functionality

[Cornelia Huck]

On Wed,  4 Nov 2020 13:12:44 -0500
Collin Walling <walling@linux.ibm.com> wrote:

> The Control Program Name Code (CPNC) portion of the diag318
> info must be set within the SIE block of each VCPU in the
> configuration. The handler will iterate through each VCPU
> and dirty the diag318_info reg to be synced with KVM on a
> subsequent sync_regs call.
> 
> Additionally, the diag318 info resets must be handled via
> userspace. As such, QEMU will reset this value for each
> VCPU during a modified clear, load normal, and load clear
> reset event.
> 

Fixes: fabdada9357b ("s390: guest support for diagnose 0x318")

(I guess?)

> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c |  5 +++++
>  target/s390x/cpu.c         | 10 ++++++++++
>  target/s390x/cpu.h         |  1 +
>  target/s390x/kvm.c         | 10 ++++++----
>  4 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 22222c4fd5..a8cebf9bff 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -427,6 +427,9 @@ static void s390_machine_reset(MachineState *machine)
>          qemu_devices_reset();
>          s390_crypto_reset();
>  
> +        CPU_FOREACH(t) {
> +            run_on_cpu(t, s390_do_cpu_set_diag318, RUN_ON_CPU_HOST_ULONG(0));
> +        }
>          /* configure and start the ipl CPU only */
>          run_on_cpu(cs, s390_do_cpu_ipl, RUN_ON_CPU_NULL);
>          break;
> @@ -440,6 +443,7 @@ static void s390_machine_reset(MachineState *machine)
>          s390_pv_prepare_reset(ms);
>          CPU_FOREACH(t) {
>              run_on_cpu(t, s390_do_cpu_full_reset, RUN_ON_CPU_NULL);
> +            run_on_cpu(t, s390_do_cpu_set_diag318, RUN_ON_CPU_HOST_ULONG(0));
>          }
>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>          break;
> @@ -451,6 +455,7 @@ static void s390_machine_reset(MachineState *machine)
>          subsystem_reset();
>          s390_pv_prepare_reset(ms);
>          CPU_FOREACH(t) {
> +            run_on_cpu(t, s390_do_cpu_set_diag318, RUN_ON_CPU_HOST_ULONG(0));
>              if (t == cs) {
>                  continue;
>              }
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 66f5942b53..cb0548b4d8 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -447,6 +447,16 @@ void s390_enable_css_support(S390CPU *cpu)
>          kvm_s390_enable_css_support(cpu);
>      }
>  }
> +
> +void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg)
> +{
> +    CPUS390XState *env = &S390_CPU(cs)->env;
> +    uint64_t diag318_info = arg.host_ulong;

Should you check with can_sync_regs() here? Or in the callers?

Do we need this even without the cpu feature bit set?

> +
> +    env->diag318_info = diag318_info;
> +    cs->kvm_run->s.regs.diag318 = diag318_info;
> +    cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> +}
>  #endif
>  
>  static gchar *s390_gdb_arch_name(CPUState *cs)
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index f875ebf0f4..60d434d5ed 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -769,6 +769,7 @@ int s390_set_memory_limit(uint64_t new_limit, uint64_t 
> *hw_limit);
>  void s390_set_max_pagesize(uint64_t pagesize, Error **errp);
>  void s390_cmma_reset(void);
>  void s390_enable_css_support(S390CPU *cpu);
> +void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
>  int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch_id,
>                                  int vq, bool assign);
>  #ifndef CONFIG_USER_ONLY
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index baa070fdf7..4b2aad009c 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1615,6 +1615,7 @@ static void handle_diag_318(S390CPU *cpu, struct 
> kvm_run *run)
>  {
>      uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
>      uint64_t diag318_info = run->s.regs.gprs[reg];
> +    CPUState *t;
>  
>      /*
>       * DIAG 318 can only be enabled with KVM support. As such, let's
> @@ -1622,13 +1623,14 @@ static void handle_diag_318(S390CPU *cpu, struct 
> kvm_run *run)
>       */
>      if (!s390_has_feat(S390_FEAT_DIAG_318)) {
>          kvm_s390_program_interrupt(cpu, PGM_SPECIFICATION);
> +        return;

Seems like this return already fixes a bug on its own?

>      }
>  
> -    cpu->env.diag318_info = diag318_info;
> -
>      if (can_sync_regs(CPU(cpu), KVM_SYNC_DIAG318)) {
> -        run->s.regs.diag318 = diag318_info;
> -        run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> +        CPU_FOREACH(t) {
> +            run_on_cpu(t, s390_do_cpu_set_diag318,
> +                       RUN_ON_CPU_HOST_ULONG(diag318_info));
> +        }
>      }
>  }
>