| Message ID | 20211108211311.33834-1-walling@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] s390x: kvm: adjust diag318 resets to retain data | expand |
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 */ >
On 11/9/21 08:32, Christian Borntraeger wrote: > > > 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)); >> - } This needs to stay for the move to the clear reset area to work on other diag308 subcodes. The reset will then be done twice for the clear reset but that's fine. Moving the diag318 data into the clear area means we only reset it on a full cpu reset which is only done for diagnose 308 subcode 0 and maybe a QEMU reset, I didn't fully follow the code there. The other subcodes do an initial reset on the calling cpu and normal resets on the others or just normal resets which don't touch the 318 data if we move it into the clear reset area. Or did I miss something fundamental here? >> 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 */ >>
On 11/9/21 05:48, Janosch Frank wrote: > On 11/9/21 08:32, Christian Borntraeger wrote: >> >> >> 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)); >>> - } > > This needs to stay for the move to the clear reset area to work on other > diag308 subcodes. The reset will then be done twice for the clear reset > but that's fine. > > Moving the diag318 data into the clear area means we only reset it on a > full cpu reset which is only done for diagnose 308 subcode 0 and maybe a > QEMU reset, I didn't fully follow the code there. > > The other subcodes do an initial reset on the calling cpu and normal > resets on the others or just normal resets which don't touch the 318 > data if we move it into the clear reset area. >> Or did I miss something fundamental here? > > While the diag318 data will not be cleared during an initial and load normal resets _directly_, the s390_machine_reset function ends with the call below: >>> s390_ipl_clear_reset_request(); >>> } The clear reset request is invoked after each of the reset types at the tail end of the function. Because of this, the diag318 data will be reset during the 308 subcodes by way of the clear reset at the end. >>> 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 */ >>> > >
On 11/9/21 16:58, Collin Walling wrote: > On 11/9/21 05:48, Janosch Frank wrote: >> On 11/9/21 08:32, Christian Borntraeger wrote: >>> >>> >>> 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)); >>>> - } >> >> This needs to stay for the move to the clear reset area to work on other >> diag308 subcodes. The reset will then be done twice for the clear reset >> but that's fine. >> >> Moving the diag318 data into the clear area means we only reset it on a >> full cpu reset which is only done for diagnose 308 subcode 0 and maybe a >> QEMU reset, I didn't fully follow the code there. >> >> The other subcodes do an initial reset on the calling cpu and normal >> resets on the others or just normal resets which don't touch the 318 >> data if we move it into the clear reset area. >>> Or did I miss something fundamental here? >> >> > While the diag318 data will not be cleared during an initial and load > normal resets _directly_, the s390_machine_reset function ends with the > call below: > >>>> s390_ipl_clear_reset_request(); >>>> } > > The clear reset request is invoked after each of the reset types at the > tail end of the function. Because of this, the diag318 data will be > reset during the 308 subcodes by way of the clear reset at the end. This changes a few values in ipl.c and is not in fact a cpu clear reset. QEMU knows a lot of resets. The VM resets here mainly depict diagnose 308 subcodes which set the cpus and the channel subsystem into a specific state for the purpose of IPL (subcode 3 and 4 are how we IPL, subcode 10 is PV IPL). The CPU resets can be triggered by SIGP (normal and initial, NOT clear reset) or are triggered as part of a VM reset mentioned above. > >>>> 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 */ >>>> >> >> > >
On 11/9/21 12:01, Janosch Frank wrote: > On 11/9/21 16:58, Collin Walling wrote: >> On 11/9/21 05:48, Janosch Frank wrote: >>> On 11/9/21 08:32, Christian Borntraeger wrote: >>>> >>>> >>>> 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)); >>>>> - } >>> >>> This needs to stay for the move to the clear reset area to work on other >>> diag308 subcodes. The reset will then be done twice for the clear reset >>> but that's fine. >>> >>> Moving the diag318 data into the clear area means we only reset it on a >>> full cpu reset which is only done for diagnose 308 subcode 0 and maybe a >>> QEMU reset, I didn't fully follow the code there. >>> >>> The other subcodes do an initial reset on the calling cpu and normal >>> resets on the others or just normal resets which don't touch the 318 >>> data if we move it into the clear reset area. >>>> Or did I miss something fundamental here? >>> >>> >> While the diag318 data will not be cleared during an initial and load >> normal resets _directly_, the s390_machine_reset function ends with the >> call below: >> >>>>> s390_ipl_clear_reset_request(); >>>>> } >> >> The clear reset request is invoked after each of the reset types at the >> tail end of the function. Because of this, the diag318 data will be >> reset during the 308 subcodes by way of the clear reset at the end. > > This changes a few values in ipl.c and is not in fact a cpu clear reset. > > QEMU knows a lot of resets. The VM resets here mainly depict diagnose > 308 subcodes which set the cpus and the channel subsystem into a > specific state for the purpose of IPL (subcode 3 and 4 are how we IPL, > subcode 10 is PV IPL). > > The CPU resets can be triggered by SIGP (normal and initial, NOT clear > reset) or are triggered as part of a VM reset mentioned above. > Makes sense to me now. I had the terminology between our s390-specific stuff and the QEMU code jumbled, but it's clear to me what's actually happening here. Thank you for the explanation. I'll send the next version which simply relocates the diag318_info in the CPU state struct. The reset code will remain untouched. >> >>>>> 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 */ >>>>> >>> >>> >> >> >
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 */
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> --- 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(-)