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

[Christian Borntraeger <borntraeger@de.ibm.com>]

Am 08.11.21 um 18:02 schrieb Janosch Frank:
> On 11/5/21 23:46, Collin Walling wrote:
>> 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>
> 
> Fixes tag?
> 
>> ---
>>   hw/s390x/s390-virtio-ccw.c   |  3 ---
>>   target/s390x/cpu-sysemu.c    |  7 -------
>>   target/s390x/cpu.h           |  5 ++---
>>   target/s390x/kvm/kvm.c       | 19 +++++--------------
>>   target/s390x/kvm/kvm_s390x.h |  1 -
>>   5 files changed, 7 insertions(+), 28 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;
> 
> Before we brought this upstream I had a conversation with the architect because I was confused about this myself. He said: SIGP does not affect 318 data but all 308 subcode resets do (0,1,3,4).
> 
> Hence I'd much rather move this out of the automatic reset areas and clear it by hand for diag308 resets. And then add a big comment with a warning to never move this into the automatic clearing areas.

the automatic cleaning areas are also used for the big hammer system_reset in QEMU which acts like a power cycle. And those fields are initialized always on such events.
  So you cannot avoid clearing this for the big hammer things anyway.
> 
>> +
>>       /* 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..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;
>>       }
>>   }
>> 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 */
>>
>