On 11/8/21 12:40, Christian Borntraeger wrote: > > > 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 <wall...@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.
The data needs to be reset during a clear reset as well. Correct me if I'm wrong here: the 308 resets will invoke the qemu reset, which will eventually invoke the machine reset. The s390_machine_reset code ends with a clear reset request. I believe having the 318 field in the struct accomplishes what we need: it is reset on clear and 308 subcodes and avoids being tampered by SIGP. >> >>> + >>> /* 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 */ >>> >> > -- Regards, Collin Stay safe and stay healthy