Re: [PATCH v2] s390x: kvm: adjust diag318 resets to retain data

[Christian Borntraeger]

Am 08.11.21 um 22:13 schrieb Collin Walling:
> The CPNC portion of the diag 318 data is erroneously reset during an
> initial CPU reset caused by SIGP. Let's go ahead and relocate the
> diag318_info field within the CPUS390XState struct such that it is
> only zeroed during a clear reset. This way, the CPNC will be retained
> for each VCPU in the configuration after the diag 318 instruction
> has been invoked.
>
> Additionally, the diag 318 data reset is handled via the CPU reset
> code during a clear reset. This means some of the diag 318-specific
> reset code can now be removed.
>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> Fixes: fabdada9357b ("s390: guest support for diagnose 0x318")
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>

It would be good to add at least a comment in the diag308 handlers
where the value of cpnc is resetted during the resets that Janosch
mentioned.
> ---
>
> Changelog:
>
>         v2
>         - handler uses run_on_cpu again
>         - reworded commit message slightly
>         - added fixes and reported-by tags
>
> ---
>   hw/s390x/s390-virtio-ccw.c   |  3 ---
>   target/s390x/cpu-sysemu.c    |  7 -------
>   target/s390x/cpu.h           |  5 ++---
>   target/s390x/kvm/kvm.c       | 14 +++++---------
>   target/s390x/kvm/kvm_s390x.h |  1 -
>   5 files changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 653587ea62..51dcb83b0c 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -489,9 +489,6 @@ static void s390_machine_reset(MachineState *machine)
>           g_assert_not_reached();
>       }
>   
> -    CPU_FOREACH(t) {
> -        run_on_cpu(t, s390_do_cpu_set_diag318, RUN_ON_CPU_HOST_ULONG(0));
> -    }
>       s390_ipl_clear_reset_request();
>   }
>   
> diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
> index 5471e01ee8..6d9f6d4402 100644
> --- a/target/s390x/cpu-sysemu.c
> +++ b/target/s390x/cpu-sysemu.c
> @@ -299,10 +299,3 @@ 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)
> -{
> -    if (kvm_enabled()) {
> -        kvm_s390_set_diag318(cs, arg.host_ulong);
> -    }
> -}
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 3153d053e9..1b94b91d87 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -63,6 +63,8 @@ struct CPUS390XState {
>       uint64_t etoken;       /* etoken */
>       uint64_t etoken_extension; /* etoken extension */
>   
> +    uint64_t diag318_info;
> +
>       /* Fields up to this point are not cleared by initial CPU reset */
>       struct {} start_initial_reset_fields;
>   
> @@ -118,8 +120,6 @@ struct CPUS390XState {
>       uint16_t external_call_addr;
>       DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
>   
> -    uint64_t diag318_info;
> -
>   #if !defined(CONFIG_USER_ONLY)
>       uint64_t tlb_fill_tec;   /* translation exception code during tlb_fill */
>       int tlb_fill_exc;        /* exception number seen during tlb_fill */
> @@ -780,7 +780,6 @@ 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/kvm.c b/target/s390x/kvm/kvm.c
> index 5b1fdb55c4..8970d9ca55 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -1576,16 +1576,13 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct 
> kvm_run *run)
>       return -ENOENT;
>   }
>   
> -void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info)
> +static void set_diag_318(CPUState *cs, run_on_cpu_data arg)
>   {
>       CPUS390XState *env = &S390_CPU(cs)->env;
>   
> -    /* Feat bit is set only if KVM supports sync for diag318 */
> -    if (s390_has_feat(S390_FEAT_DIAG_318)) {
> -        env->diag318_info = diag318_info;
> -        cs->kvm_run->s.regs.diag318 = diag318_info;
> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> -    }
> +    env->diag318_info = arg.host_ulong;
> +    cs->kvm_run->s.regs.diag318 = arg.host_ulong;
> +    cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>   }
>   
>   static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
> @@ -1604,8 +1601,7 @@ static void handle_diag_318(S390CPU *cpu, struct kvm_run 
> *run)
>       }
>   
>       CPU_FOREACH(t) {
> -        run_on_cpu(t, s390_do_cpu_set_diag318,
> -                   RUN_ON_CPU_HOST_ULONG(diag318_info));
> +        run_on_cpu(t, set_diag_318, RUN_ON_CPU_HOST_ULONG(diag318_info));
>       }
>   }
>   
> diff --git a/target/s390x/kvm/kvm_s390x.h b/target/s390x/kvm/kvm_s390x.h
> index 05a5e1e6f4..8c244ee84d 100644
> --- a/target/s390x/kvm/kvm_s390x.h
> +++ b/target/s390x/kvm/kvm_s390x.h
> @@ -44,6 +44,5 @@ void kvm_s390_set_max_pagesize(uint64_t pagesize, Error 
> **errp);
>   void kvm_s390_crypto_reset(void);
>   void kvm_s390_restart_interrupt(S390CPU *cpu);
>   void kvm_s390_stop_interrupt(S390CPU *cpu);
> -void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info);
>   
>   #endif /* KVM_S390X_H */