| Message ID | 20231018130716.286638-7-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/25] qapi: machine.json: change docs regarding CPU topology | expand |
On Wed, 18 Oct 2023 at 06:09, Thomas Huth <thuth@redhat.com> wrote: > > From: Pierre Morel <pmorel@linux.ibm.com> > > During a subsystem reset the Topology-Change-Report is cleared > by the machine. > Let's ask KVM to clear the Modified Topology Change Report (MTCR) > bit of the SCA in the case of a subsystem reset. > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > Message-ID: <20231016183925.2384704-7-nsg@linux.ibm.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > include/hw/s390x/cpu-topology.h | 1 + > target/s390x/cpu.h | 1 + > target/s390x/kvm/kvm_s390x.h | 1 + > hw/s390x/cpu-topology.c | 11 +++++++++++ > hw/s390x/s390-virtio-ccw.c | 3 +++ > target/s390x/cpu-sysemu.c | 13 +++++++++++++ > target/s390x/kvm/kvm.c | 17 +++++++++++++++++ > 7 files changed, 47 insertions(+) > > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h > index f95d26d37c..e33e7c66df 100644 > --- a/include/hw/s390x/cpu-topology.h > +++ b/include/hw/s390x/cpu-topology.h > @@ -56,6 +56,7 @@ static inline void s390_topology_setup_cpu(MachineState *ms, > #endif > > extern S390Topology s390_topology; > +void s390_topology_reset(void); Please take a look at the following CI failure: /usr/bin/ld: libqemu-s390x-softmmu.fa.p/hw_s390x_s390-virtio-ccw.c.o: in function `subsystem_reset': /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/build/../hw/s390x/s390-virtio-ccw.c:128: undefined reference to `s390_topology_reset' https://gitlab.com/qemu-project/qemu/-/jobs/5330218593 > > static inline int s390_std_socket(int n, CpuTopology *smp) > { > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index 09bff39fe4..40c5cedd0e 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -654,6 +654,7 @@ typedef struct SysIBCPUListEntry { > QEMU_BUILD_BUG_ON(sizeof(SysIBCPUListEntry) != 16); > > void insert_stsi_15_1_x(S390CPU *cpu, int sel2, uint64_t addr, uint8_t ar, uintptr_t ra); > +void s390_cpu_topology_set_changed(bool changed); > > /* MMU defines */ > #define ASCE_ORIGIN (~0xfffULL) /* segment table origin */ > diff --git a/target/s390x/kvm/kvm_s390x.h b/target/s390x/kvm/kvm_s390x.h > index f9785564d0..649dae5948 100644 > --- a/target/s390x/kvm/kvm_s390x.h > +++ b/target/s390x/kvm/kvm_s390x.h > @@ -47,5 +47,6 @@ 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); > +int kvm_s390_topology_set_mtcr(uint64_t attr); > > #endif /* KVM_S390X_H */ > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c > index 13168341b6..7ec9319272 100644 > --- a/hw/s390x/cpu-topology.c > +++ b/hw/s390x/cpu-topology.c > @@ -90,6 +90,17 @@ static void s390_topology_init(MachineState *ms) > smp->books * smp->drawers); > } > > +/** > + * s390_topology_reset: > + * > + * Generic reset for CPU topology, calls s390_topology_reset() > + * to reset the kernel Modified Topology Change Record. > + */ > +void s390_topology_reset(void) > +{ > + s390_cpu_topology_set_changed(false); > +} > + > /** > * s390_topology_cpu_default: > * @cpu: pointer to a S390CPU > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c > index 7fe2bce20c..6012165d41 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -124,6 +124,9 @@ static void subsystem_reset(void) > device_cold_reset(dev); > } > } > + if (s390_has_topology()) { > + s390_topology_reset(); > + } > } > > static int virtio_ccw_hcall_notify(const uint64_t *args) > diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c > index 8112561e5e..1cd30c1d84 100644 > --- a/target/s390x/cpu-sysemu.c > +++ b/target/s390x/cpu-sysemu.c > @@ -307,3 +307,16 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg) > kvm_s390_set_diag318(cs, arg.host_ulong); > } > } > + > +void s390_cpu_topology_set_changed(bool changed) > +{ > + int ret; > + > + if (kvm_enabled()) { > + ret = kvm_s390_topology_set_mtcr(changed); > + if (ret) { > + error_report("Failed to set Modified Topology Change Report: %s", > + strerror(-ret)); > + } > + } > +} > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c > index 53d6300809..d6bda3a2a8 100644 > --- a/target/s390x/kvm/kvm.c > +++ b/target/s390x/kvm/kvm.c > @@ -2664,6 +2664,23 @@ int kvm_s390_get_zpci_op(void) > return cap_zpci_op; > } > > +int kvm_s390_topology_set_mtcr(uint64_t attr) > +{ > + struct kvm_device_attr attribute = { > + .group = KVM_S390_VM_CPU_TOPOLOGY, > + .attr = attr, > + }; > + > + if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) { > + return 0; > + } > + if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) { > + return -ENOTSUP; > + } > + > + return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute); > +} > + > void kvm_arch_accel_class_init(ObjectClass *oc) > { > } > -- > 2.41.0 > >
On Thu, 2023-10-19 at 09:35 -0700, Stefan Hajnoczi wrote: > On Wed, 18 Oct 2023 at 06:09, Thomas Huth <thuth@redhat.com> wrote: > > > > From: Pierre Morel <pmorel@linux.ibm.com> > > > > During a subsystem reset the Topology-Change-Report is cleared > > by the machine. > > Let's ask KVM to clear the Modified Topology Change Report (MTCR) > > bit of the SCA in the case of a subsystem reset. > > > > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> > > Message-ID: <20231016183925.2384704-7-nsg@linux.ibm.com> > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > include/hw/s390x/cpu-topology.h | 1 + > > target/s390x/cpu.h | 1 + > > target/s390x/kvm/kvm_s390x.h | 1 + > > hw/s390x/cpu-topology.c | 11 +++++++++++ > > hw/s390x/s390-virtio-ccw.c | 3 +++ > > target/s390x/cpu-sysemu.c | 13 +++++++++++++ > > target/s390x/kvm/kvm.c | 17 +++++++++++++++++ > > 7 files changed, 47 insertions(+) > > > > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h > > index f95d26d37c..e33e7c66df 100644 > > --- a/include/hw/s390x/cpu-topology.h > > +++ b/include/hw/s390x/cpu-topology.h > > @@ -56,6 +56,7 @@ static inline void s390_topology_setup_cpu(MachineState *ms, > > #endif > > > > extern S390Topology s390_topology; > > +void s390_topology_reset(void); > > Please take a look at the following CI failure: > > /usr/bin/ld: libqemu-s390x-softmmu.fa.p/hw_s390x_s390-virtio-ccw.c.o: > in function `subsystem_reset': > /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/build/../hw/s390x/s390-virtio-ccw.c:128: > undefined reference to `s390_topology_reset' > > https://gitlab.com/qemu-project/qemu/-/jobs/5330218593 I can replicate this with --disable-kvm, tho I don't think that's what the CI does. Fix looks something like this (copy pasted): --- a/include/hw/s390x/cpu-topology.h +++ b/include/hw/s390x/cpu-topology.h @@ -45,6 +45,7 @@ typedef QTAILQ_HEAD(, S390TopologyEntry) S390TopologyList; #ifdef CONFIG_KVM bool s390_has_topology(void); void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error **errp); +void s390_topology_reset(void); #else static inline bool s390_has_topology(void) { @@ -53,10 +54,14 @@ static inline bool s390_has_topology(void) static inline void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error **errp) {} +static inline void s390_topology_reset(void) +{ + /* Unreachable, CPU topology not implemented for TCG */ + assert(false); +} #endif extern S390Topology s390_topology; -void s390_topology_reset(void); static inline int s390_std_socket(int n, CpuTopology *smp) {
On 19/10/2023 19.55, Nina Schoetterl-Glausch wrote: > On Thu, 2023-10-19 at 09:35 -0700, Stefan Hajnoczi wrote: >> On Wed, 18 Oct 2023 at 06:09, Thomas Huth <thuth@redhat.com> wrote: >>> >>> From: Pierre Morel <pmorel@linux.ibm.com> >>> >>> During a subsystem reset the Topology-Change-Report is cleared >>> by the machine. >>> Let's ask KVM to clear the Modified Topology Change Report (MTCR) >>> bit of the SCA in the case of a subsystem reset. >>> >>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com> >>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >>> Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com> >>> Message-ID: <20231016183925.2384704-7-nsg@linux.ibm.com> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> include/hw/s390x/cpu-topology.h | 1 + >>> target/s390x/cpu.h | 1 + >>> target/s390x/kvm/kvm_s390x.h | 1 + >>> hw/s390x/cpu-topology.c | 11 +++++++++++ >>> hw/s390x/s390-virtio-ccw.c | 3 +++ >>> target/s390x/cpu-sysemu.c | 13 +++++++++++++ >>> target/s390x/kvm/kvm.c | 17 +++++++++++++++++ >>> 7 files changed, 47 insertions(+) >>> >>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h >>> index f95d26d37c..e33e7c66df 100644 >>> --- a/include/hw/s390x/cpu-topology.h >>> +++ b/include/hw/s390x/cpu-topology.h >>> @@ -56,6 +56,7 @@ static inline void s390_topology_setup_cpu(MachineState *ms, >>> #endif >>> >>> extern S390Topology s390_topology; >>> +void s390_topology_reset(void); >> >> Please take a look at the following CI failure: >> >> /usr/bin/ld: libqemu-s390x-softmmu.fa.p/hw_s390x_s390-virtio-ccw.c.o: >> in function `subsystem_reset': >> /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/build/../hw/s390x/s390-virtio-ccw.c:128: >> undefined reference to `s390_topology_reset' >> >> https://gitlab.com/qemu-project/qemu/-/jobs/5330218593 > > I can replicate this with --disable-kvm, tho I don't think that's what the CI does. I think that was the wrong CI job that Stefan linked. It rather seemed to happen here: https://gitlab.com/qemu-project/qemu/-/jobs/5329820093#L5564 That job uses --enable-debug which turns off optimization, i.e. that was likely causing some code to be included that normally gets optimized away. > Fix looks something like this (copy pasted): > > --- a/include/hw/s390x/cpu-topology.h > +++ b/include/hw/s390x/cpu-topology.h > @@ -45,6 +45,7 @@ typedef QTAILQ_HEAD(, S390TopologyEntry) S390TopologyList; > #ifdef CONFIG_KVM > bool s390_has_topology(void); > void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error **errp); > +void s390_topology_reset(void); > #else > static inline bool s390_has_topology(void) > { > @@ -53,10 +54,14 @@ static inline bool s390_has_topology(void) > static inline void s390_topology_setup_cpu(MachineState *ms, > S390CPU *cpu, > Error **errp) {} > +static inline void s390_topology_reset(void) > +{ > + /* Unreachable, CPU topology not implemented for TCG */ > + assert(false); > +} > #endif > > extern S390Topology s390_topology; > -void s390_topology_reset(void); > > static inline int s390_std_socket(int n, CpuTopology *smp) > { Thanks, that seems to fix the issue with --enable-debug, too. I'll squash that into the related patch (also fixing the indentation in s390_has_topology()) and respin the pull request. Thomas
On Thu, 2023-10-19 at 21:32 +0200, Thomas Huth wrote: [...] > Thanks, that seems to fix the issue with --enable-debug, too. > I'll squash that into the related patch (also fixing the indentation in > s390_has_topology()) and respin the pull request. > > Thomas > Thanks!
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h index f95d26d37c..e33e7c66df 100644 --- a/include/hw/s390x/cpu-topology.h +++ b/include/hw/s390x/cpu-topology.h @@ -56,6 +56,7 @@ static inline void s390_topology_setup_cpu(MachineState *ms, #endif extern S390Topology s390_topology; +void s390_topology_reset(void); static inline int s390_std_socket(int n, CpuTopology *smp) { diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index 09bff39fe4..40c5cedd0e 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -654,6 +654,7 @@ typedef struct SysIBCPUListEntry { QEMU_BUILD_BUG_ON(sizeof(SysIBCPUListEntry) != 16); void insert_stsi_15_1_x(S390CPU *cpu, int sel2, uint64_t addr, uint8_t ar, uintptr_t ra); +void s390_cpu_topology_set_changed(bool changed); /* MMU defines */ #define ASCE_ORIGIN (~0xfffULL) /* segment table origin */ diff --git a/target/s390x/kvm/kvm_s390x.h b/target/s390x/kvm/kvm_s390x.h index f9785564d0..649dae5948 100644 --- a/target/s390x/kvm/kvm_s390x.h +++ b/target/s390x/kvm/kvm_s390x.h @@ -47,5 +47,6 @@ 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); +int kvm_s390_topology_set_mtcr(uint64_t attr); #endif /* KVM_S390X_H */ diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c index 13168341b6..7ec9319272 100644 --- a/hw/s390x/cpu-topology.c +++ b/hw/s390x/cpu-topology.c @@ -90,6 +90,17 @@ static void s390_topology_init(MachineState *ms) smp->books * smp->drawers); } +/** + * s390_topology_reset: + * + * Generic reset for CPU topology, calls s390_topology_reset() + * to reset the kernel Modified Topology Change Record. + */ +void s390_topology_reset(void) +{ + s390_cpu_topology_set_changed(false); +} + /** * s390_topology_cpu_default: * @cpu: pointer to a S390CPU diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index 7fe2bce20c..6012165d41 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -124,6 +124,9 @@ static void subsystem_reset(void) device_cold_reset(dev); } } + if (s390_has_topology()) { + s390_topology_reset(); + } } static int virtio_ccw_hcall_notify(const uint64_t *args) diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c index 8112561e5e..1cd30c1d84 100644 --- a/target/s390x/cpu-sysemu.c +++ b/target/s390x/cpu-sysemu.c @@ -307,3 +307,16 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg) kvm_s390_set_diag318(cs, arg.host_ulong); } } + +void s390_cpu_topology_set_changed(bool changed) +{ + int ret; + + if (kvm_enabled()) { + ret = kvm_s390_topology_set_mtcr(changed); + if (ret) { + error_report("Failed to set Modified Topology Change Report: %s", + strerror(-ret)); + } + } +} diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c index 53d6300809..d6bda3a2a8 100644 --- a/target/s390x/kvm/kvm.c +++ b/target/s390x/kvm/kvm.c @@ -2664,6 +2664,23 @@ int kvm_s390_get_zpci_op(void) return cap_zpci_op; } +int kvm_s390_topology_set_mtcr(uint64_t attr) +{ + struct kvm_device_attr attribute = { + .group = KVM_S390_VM_CPU_TOPOLOGY, + .attr = attr, + }; + + if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) { + return 0; + } + if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) { + return -ENOTSUP; + } + + return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute); +} + void kvm_arch_accel_class_init(ObjectClass *oc) { }