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

[Christian Borntraeger]

Am 05.11.21 um 23:46 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 by the kernel.
>
> Additionally, the diag 318 data reset is handled via the CPU reset
> code. The set_diag318 code can be merged into the handler function
> and the helper functions can consequently be removed.
>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
[..]
> 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;
> -

The move makes perfect sense.


>   #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..ed9c477b6f 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -1576,18 +1576,6 @@ static int handle_sw_breakpoint(S390CPU *cpu, struct 
> kvm_run *run)
>       return -ENOENT;
>   }
>   
> -void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info)
> -{
> -    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;
> -    }
> -}
> -
>   static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
>   {
>       uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
> @@ -1604,8 +1592,11 @@ 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));
> +        CPUS390XState *env = &S390_CPU(t)->env;
> +
> +        env->diag318_info = diag318_info;
> +        t->kvm_run->s.regs.diag318 = diag318_info;
> +        t->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;

I am not sure if this part works fine. What happens if
another CPU is currently in SIE (not stopped).
Then this change will be not visible in that CPU and in
fact this change will be overwritten when the CPU exits to QEMU.