| Message ID | 20191203132813.2734-3-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Increase architectural compliance | expand |
On Tue, 3 Dec 2019 08:28:11 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach > for the initial reset, and that was also called for the clear > reset. To be architecture compliant, we also need to clear local > interrupts on a normal reset. > > Because of this and the upcoming protvirt support we need to add > ioctls for the missing clear and normal resets. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Acked-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/cpu.c | 16 +++++++++++++-- > target/s390x/kvm-stub.c | 10 +++++++++- > target/s390x/kvm.c | 42 ++++++++++++++++++++++++++++++++-------- > target/s390x/kvm_s390x.h | 4 +++- > 4 files changed, 60 insertions(+), 12 deletions(-) > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index 829ce6ad54..4973365d6c 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type) > } > > /* Reset state inside the kernel that we cannot access yet from QEMU. */ For the last iteration, I asked about the 'yet' here... > - if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) { > - kvm_s390_reset_vcpu(cpu); > + if (kvm_enabled()) { > + switch (type) { > + case S390_CPU_RESET_CLEAR: > + kvm_s390_reset_vcpu_clear(cpu); > + break; > + case S390_CPU_RESET_INITIAL: > + kvm_s390_reset_vcpu_initial(cpu); > + break; > + case S390_CPU_RESET_NORMAL: > + kvm_s390_reset_vcpu_normal(cpu); > + break; > + default: > + g_assert_not_reached(); > + } > } > } > (...) > @@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs) > return 0; > } > > -void kvm_s390_reset_vcpu(S390CPU *cpu) > +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type) > { > CPUState *cs = CPU(cpu); > > - /* The initial reset call is needed here to reset in-kernel > - * vcpu data that we can't access directly from QEMU > - * (i.e. with older kernels which don't support sync_regs/ONE_REG). > - * Before this ioctl cpu_synchronize_state() is called in common kvm > - * code (kvm-all) */ > - if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) { > - error_report("Initial CPU reset failed on CPU %i", cs->cpu_index); > + /* > + * The reset call is needed here to reset in-kernel vcpu data that > + * we can't access directly from QEMU (i.e. with older kernels > + * which don't support sync_regs/ONE_REG). Before this ioctl ...and this reference to 'older kernels' here. Are the comments still correct/relevant? > + * cpu_synchronize_state() is called in common kvm code > + * (kvm-all). > + */ > + if (kvm_vcpu_ioctl(cs, type)) { > + error_report("CPU reset failed on CPU %i type %lx", > + cs->cpu_index, type); > + } > +}
On 12/3/19 6:44 PM, Cornelia Huck wrote: > On Tue, 3 Dec 2019 08:28:11 -0500 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach >> for the initial reset, and that was also called for the clear >> reset. To be architecture compliant, we also need to clear local >> interrupts on a normal reset. >> >> Because of this and the upcoming protvirt support we need to add >> ioctls for the missing clear and normal resets. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> Acked-by: David Hildenbrand <david@redhat.com> >> --- >> target/s390x/cpu.c | 16 +++++++++++++-- >> target/s390x/kvm-stub.c | 10 +++++++++- >> target/s390x/kvm.c | 42 ++++++++++++++++++++++++++++++++-------- >> target/s390x/kvm_s390x.h | 4 +++- >> 4 files changed, 60 insertions(+), 12 deletions(-) >> >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c >> index 829ce6ad54..4973365d6c 100644 >> --- a/target/s390x/cpu.c >> +++ b/target/s390x/cpu.c >> @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type) >> } >> >> /* Reset state inside the kernel that we cannot access yet from QEMU. */ > > For the last iteration, I asked about the 'yet' here... I have not written those comments, I merely refuse to delete them :) We still reset some state in the kernel, I'm not sure how much of that is already exposed via ioctls to QEMU, so I won't remove the comment. > >> - if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) { >> - kvm_s390_reset_vcpu(cpu); >> + if (kvm_enabled()) { >> + switch (type) { >> + case S390_CPU_RESET_CLEAR: >> + kvm_s390_reset_vcpu_clear(cpu); >> + break; >> + case S390_CPU_RESET_INITIAL: >> + kvm_s390_reset_vcpu_initial(cpu); >> + break; >> + case S390_CPU_RESET_NORMAL: >> + kvm_s390_reset_vcpu_normal(cpu); >> + break; >> + default: >> + g_assert_not_reached(); >> + } >> } >> } >> > > (...) > >> @@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs) >> return 0; >> } >> >> -void kvm_s390_reset_vcpu(S390CPU *cpu) >> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type) >> { >> CPUState *cs = CPU(cpu); >> >> - /* The initial reset call is needed here to reset in-kernel >> - * vcpu data that we can't access directly from QEMU >> - * (i.e. with older kernels which don't support sync_regs/ONE_REG). >> - * Before this ioctl cpu_synchronize_state() is called in common kvm >> - * code (kvm-all) */ >> - if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) { >> - error_report("Initial CPU reset failed on CPU %i", cs->cpu_index); >> + /* >> + * The reset call is needed here to reset in-kernel vcpu data that >> + * we can't access directly from QEMU (i.e. with older kernels >> + * which don't support sync_regs/ONE_REG). Before this ioctl > > ...and this reference to 'older kernels' here. > > Are the comments still correct/relevant? See above > >> + * cpu_synchronize_state() is called in common kvm code >> + * (kvm-all). >> + */ >> + if (kvm_vcpu_ioctl(cs, type)) { >> + error_report("CPU reset failed on CPU %i type %lx", >> + cs->cpu_index, type); >> + } >> +} >
On Wed, 4 Dec 2019 10:00:45 +0100 Janosch Frank <frankja@linux.ibm.com> wrote: > On 12/3/19 6:44 PM, Cornelia Huck wrote: > > On Tue, 3 Dec 2019 08:28:11 -0500 > > Janosch Frank <frankja@linux.ibm.com> wrote: > > > >> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach > >> for the initial reset, and that was also called for the clear > >> reset. To be architecture compliant, we also need to clear local > >> interrupts on a normal reset. > >> > >> Because of this and the upcoming protvirt support we need to add > >> ioctls for the missing clear and normal resets. > >> > >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > >> Reviewed-by: Thomas Huth <thuth@redhat.com> > >> Acked-by: David Hildenbrand <david@redhat.com> > >> --- > >> target/s390x/cpu.c | 16 +++++++++++++-- > >> target/s390x/kvm-stub.c | 10 +++++++++- > >> target/s390x/kvm.c | 42 ++++++++++++++++++++++++++++++++-------- > >> target/s390x/kvm_s390x.h | 4 +++- > >> 4 files changed, 60 insertions(+), 12 deletions(-) > >> > >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > >> index 829ce6ad54..4973365d6c 100644 > >> --- a/target/s390x/cpu.c > >> +++ b/target/s390x/cpu.c > >> @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type) > >> } > >> > >> /* Reset state inside the kernel that we cannot access yet from QEMU. */ > > > > For the last iteration, I asked about the 'yet' here... > > I have not written those comments, I merely refuse to delete them :) > We still reset some state in the kernel, I'm not sure how much of that > is already exposed via ioctls to QEMU, so I won't remove the comment. > > > > >> - if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) { > >> - kvm_s390_reset_vcpu(cpu); > >> + if (kvm_enabled()) { > >> + switch (type) { > >> + case S390_CPU_RESET_CLEAR: > >> + kvm_s390_reset_vcpu_clear(cpu); > >> + break; > >> + case S390_CPU_RESET_INITIAL: > >> + kvm_s390_reset_vcpu_initial(cpu); > >> + break; > >> + case S390_CPU_RESET_NORMAL: > >> + kvm_s390_reset_vcpu_normal(cpu); > >> + break; > >> + default: > >> + g_assert_not_reached(); > >> + } > >> } > >> } > >> > > > > (...) > > > >> @@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs) > >> return 0; > >> } > >> > >> -void kvm_s390_reset_vcpu(S390CPU *cpu) > >> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type) > >> { > >> CPUState *cs = CPU(cpu); > >> > >> - /* The initial reset call is needed here to reset in-kernel > >> - * vcpu data that we can't access directly from QEMU > >> - * (i.e. with older kernels which don't support sync_regs/ONE_REG). > >> - * Before this ioctl cpu_synchronize_state() is called in common kvm > >> - * code (kvm-all) */ > >> - if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) { > >> - error_report("Initial CPU reset failed on CPU %i", cs->cpu_index); > >> + /* > >> + * The reset call is needed here to reset in-kernel vcpu data that > >> + * we can't access directly from QEMU (i.e. with older kernels > >> + * which don't support sync_regs/ONE_REG). Before this ioctl > > > > ...and this reference to 'older kernels' here. > > > > Are the comments still correct/relevant? > > See above Fair enough, let's keep them for now and revisit this later. > > > > >> + * cpu_synchronize_state() is called in common kvm code > >> + * (kvm-all). > >> + */ > >> + if (kvm_vcpu_ioctl(cs, type)) { > >> + error_report("CPU reset failed on CPU %i type %lx", > >> + cs->cpu_index, type); > >> + } > >> +} > > > >
On Tue, 3 Dec 2019 08:28:11 -0500 Janosch Frank <frankja@linux.ibm.com> wrote: > Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach > for the initial reset, and that was also called for the clear > reset. To be architecture compliant, we also need to clear local > interrupts on a normal reset. Do we also need to do something like that for tcg? David? > > Because of this and the upcoming protvirt support we need to add > ioctls for the missing clear and normal resets. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Acked-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/cpu.c | 16 +++++++++++++-- > target/s390x/kvm-stub.c | 10 +++++++++- > target/s390x/kvm.c | 42 ++++++++++++++++++++++++++++++++-------- > target/s390x/kvm_s390x.h | 4 +++- > 4 files changed, 60 insertions(+), 12 deletions(-)
On 04.12.19 15:59, Cornelia Huck wrote: > On Tue, 3 Dec 2019 08:28:11 -0500 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach >> for the initial reset, and that was also called for the clear >> reset. To be architecture compliant, we also need to clear local >> interrupts on a normal reset. > > Do we also need to do something like that for tcg? David? > So, we have /* Fields up to this point are not cleared by initial CPU reset */ struct {} start_initial_reset_fields; [...] int pending_int uint16_t external_call_addr; DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS); [...] /* Fields up to this point are cleared by a CPU reset */ struct {} end_reset_fields; This means, local interrupts will be cleared by everything that zeroes "start_initial_reset_fields->end_reset_fields" So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to manually set them to zero. (we really should wrap TCG-only fields by ifdefs)
On 04.12.19 16:07, David Hildenbrand wrote: > On 04.12.19 15:59, Cornelia Huck wrote: >> On Tue, 3 Dec 2019 08:28:11 -0500 >> Janosch Frank <frankja@linux.ibm.com> wrote: >> >>> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach >>> for the initial reset, and that was also called for the clear >>> reset. To be architecture compliant, we also need to clear local >>> interrupts on a normal reset. >> >> Do we also need to do something like that for tcg? David? >> > > So, we have > > /* Fields up to this point are not cleared by initial CPU reset */ > struct {} start_initial_reset_fields; > [...] > int pending_int > uint16_t external_call_addr; > DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS); > [...] > /* Fields up to this point are cleared by a CPU reset */ > struct {} end_reset_fields; > > This means, local interrupts will be cleared by everything that zeroes > "start_initial_reset_fields->end_reset_fields" > > So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not > wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to > manually set them to zero. Sorry, by S390_CPU_RESET_INITIAL and S390_CPU_RESET_CLEAR. > > (we really should wrap TCG-only fields by ifdefs) >
On Wed, 4 Dec 2019 16:08:48 +0100 David Hildenbrand <david@redhat.com> wrote: > On 04.12.19 16:07, David Hildenbrand wrote: > > On 04.12.19 15:59, Cornelia Huck wrote: > >> On Tue, 3 Dec 2019 08:28:11 -0500 > >> Janosch Frank <frankja@linux.ibm.com> wrote: > >> > >>> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach > >>> for the initial reset, and that was also called for the clear > >>> reset. To be architecture compliant, we also need to clear local > >>> interrupts on a normal reset. > >> > >> Do we also need to do something like that for tcg? David? > >> > > > > So, we have > > > > /* Fields up to this point are not cleared by initial CPU reset */ > > struct {} start_initial_reset_fields; > > [...] > > int pending_int > > uint16_t external_call_addr; > > DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS); > > [...] > > /* Fields up to this point are cleared by a CPU reset */ > > struct {} end_reset_fields; > > > > This means, local interrupts will be cleared by everything that zeroes > > "start_initial_reset_fields->end_reset_fields" > > > > So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not > > wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to > > manually set them to zero. > > Sorry, by S390_CPU_RESET_INITIAL and S390_CPU_RESET_CLEAR. Ok. Will you cook up a patch, or should I do it? > > > > > (we really should wrap TCG-only fields by ifdefs) They probably do not get into the way, but making it obvious that they a tcg-only is something we really should do. > > > >
On 04.12.19 16:33, Cornelia Huck wrote: > On Wed, 4 Dec 2019 16:08:48 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> On 04.12.19 16:07, David Hildenbrand wrote: >>> On 04.12.19 15:59, Cornelia Huck wrote: >>>> On Tue, 3 Dec 2019 08:28:11 -0500 >>>> Janosch Frank <frankja@linux.ibm.com> wrote: >>>> >>>>> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach >>>>> for the initial reset, and that was also called for the clear >>>>> reset. To be architecture compliant, we also need to clear local >>>>> interrupts on a normal reset. >>>> >>>> Do we also need to do something like that for tcg? David? >>>> >>> >>> So, we have >>> >>> /* Fields up to this point are not cleared by initial CPU reset */ >>> struct {} start_initial_reset_fields; >>> [...] >>> int pending_int >>> uint16_t external_call_addr; >>> DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS); >>> [...] >>> /* Fields up to this point are cleared by a CPU reset */ >>> struct {} end_reset_fields; >>> >>> This means, local interrupts will be cleared by everything that zeroes >>> "start_initial_reset_fields->end_reset_fields" >>> >>> So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not >>> wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to >>> manually set them to zero. >> >> Sorry, by S390_CPU_RESET_INITIAL and S390_CPU_RESET_CLEAR. > > Ok. Will you cook up a patch, or should I do it? > If you want to have some TCG fun, feel free. Otherwise, let me know :)
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index 829ce6ad54..4973365d6c 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type) } /* Reset state inside the kernel that we cannot access yet from QEMU. */ - if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) { - kvm_s390_reset_vcpu(cpu); + if (kvm_enabled()) { + switch (type) { + case S390_CPU_RESET_CLEAR: + kvm_s390_reset_vcpu_clear(cpu); + break; + case S390_CPU_RESET_INITIAL: + kvm_s390_reset_vcpu_initial(cpu); + break; + case S390_CPU_RESET_NORMAL: + kvm_s390_reset_vcpu_normal(cpu); + break; + default: + g_assert_not_reached(); + } } } diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c index 5152e2bdf1..c4cd497f85 100644 --- a/target/s390x/kvm-stub.c +++ b/target/s390x/kvm-stub.c @@ -83,7 +83,15 @@ void kvm_s390_cmma_reset(void) { } -void kvm_s390_reset_vcpu(S390CPU *cpu) +void kvm_s390_reset_vcpu_initial(S390CPU *cpu) +{ +} + +void kvm_s390_reset_vcpu_clear(S390CPU *cpu) +{ +} + +void kvm_s390_reset_vcpu_normal(S390CPU *cpu) { } diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index ad6e38c876..f633472980 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -151,6 +151,7 @@ static int cap_s390_irq; static int cap_ri; static int cap_gs; static int cap_hpage_1m; +static int cap_vcpu_resets; static int active_cmma; @@ -342,6 +343,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s) cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF); cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP); cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ); + cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS); if (!kvm_check_extension(s, KVM_CAP_S390_GMAP) || !kvm_check_extension(s, KVM_CAP_S390_COW)) { @@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs) return 0; } -void kvm_s390_reset_vcpu(S390CPU *cpu) +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type) { CPUState *cs = CPU(cpu); - /* The initial reset call is needed here to reset in-kernel - * vcpu data that we can't access directly from QEMU - * (i.e. with older kernels which don't support sync_regs/ONE_REG). - * Before this ioctl cpu_synchronize_state() is called in common kvm - * code (kvm-all) */ - if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) { - error_report("Initial CPU reset failed on CPU %i", cs->cpu_index); + /* + * The reset call is needed here to reset in-kernel vcpu data that + * we can't access directly from QEMU (i.e. with older kernels + * which don't support sync_regs/ONE_REG). Before this ioctl + * cpu_synchronize_state() is called in common kvm code + * (kvm-all). + */ + if (kvm_vcpu_ioctl(cs, type)) { + error_report("CPU reset failed on CPU %i type %lx", + cs->cpu_index, type); + } +} + +void kvm_s390_reset_vcpu_initial(S390CPU *cpu) +{ + kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET); +} + +void kvm_s390_reset_vcpu_clear(S390CPU *cpu) +{ + if (cap_vcpu_resets) { + kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET); + } else { + kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET); + } +} + +void kvm_s390_reset_vcpu_normal(S390CPU *cpu) +{ + if (cap_vcpu_resets) { + kvm_s390_reset_vcpu(cpu, KVM_S390_NORMAL_RESET); } } diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h index caf985955b..0b21789796 100644 --- a/target/s390x/kvm_s390x.h +++ b/target/s390x/kvm_s390x.h @@ -34,7 +34,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch, int vq, bool assign); int kvm_s390_cmma_active(void); void kvm_s390_cmma_reset(void); -void kvm_s390_reset_vcpu(S390CPU *cpu); +void kvm_s390_reset_vcpu_clear(S390CPU *cpu); +void kvm_s390_reset_vcpu_normal(S390CPU *cpu); +void kvm_s390_reset_vcpu_initial(S390CPU *cpu); int kvm_s390_set_mem_limit(uint64_t new_limit, uint64_t *hw_limit); void kvm_s390_set_max_pagesize(uint64_t pagesize, Error **errp); void kvm_s390_crypto_reset(void);