| Message ID | 20230201132051.126868-6-pmorel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: CPU Topology | expand |
On 01/02/2023 14.20, Pierre Morel wrote: > 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> > --- ... > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c > index a80a1ebf22..cf63f3dd01 100644 > --- a/hw/s390x/cpu-topology.c > +++ b/hw/s390x/cpu-topology.c > @@ -85,6 +85,18 @@ static void s390_topology_init(MachineState *ms) > QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next); > } > > +/** > + * s390_topology_reset: > + * > + * Generic reset for CPU topology, calls s390_topology_reset() > + * s390_topology_reset() to reset the kernel Modified Topology Duplicated s390_topology_reset() in the comment. > + * change record. > + */ > +void s390_topology_reset(void) > +{ > + s390_cpu_topology_reset(); > +} With the nit fixed: Reviewed-by: Thomas Huth <thuth@redhat.com>
On 2/6/23 12:05, Thomas Huth wrote: > On 01/02/2023 14.20, Pierre Morel wrote: >> 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> >> --- > ... >> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c >> index a80a1ebf22..cf63f3dd01 100644 >> --- a/hw/s390x/cpu-topology.c >> +++ b/hw/s390x/cpu-topology.c >> @@ -85,6 +85,18 @@ static void s390_topology_init(MachineState *ms) >> QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next); >> } >> +/** >> + * s390_topology_reset: >> + * >> + * Generic reset for CPU topology, calls s390_topology_reset() >> + * s390_topology_reset() to reset the kernel Modified Topology > > Duplicated s390_topology_reset() in the comment. right, thx. > >> + * change record. >> + */ >> +void s390_topology_reset(void) >> +{ >> + s390_cpu_topology_reset(); >> +} > > With the nit fixed: > Reviewed-by: Thomas Huth <thuth@redhat.com> > Thanks! Regards, Pierre
On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote: > 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> > --- > include/hw/s390x/cpu-topology.h | 1 + > target/s390x/cpu.h | 1 + > target/s390x/kvm/kvm_s390x.h | 1 + > hw/s390x/cpu-topology.c | 12 ++++++++++++ > hw/s390x/s390-virtio-ccw.c | 3 +++ > target/s390x/cpu-sysemu.c | 13 +++++++++++++ > target/s390x/kvm/kvm.c | 17 +++++++++++++++++ > 7 files changed, 48 insertions(+) > > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h > index 1ae7e7c5e3..60e0b9fbfa 100644 > --- a/include/hw/s390x/cpu-topology.h > +++ b/include/hw/s390x/cpu-topology.h > @@ -66,5 +66,6 @@ static inline void s390_topology_set_cpu(MachineState *ms, > > extern S390Topology s390_topology; > int s390_socket_nb(S390CPU *cpu); > +void s390_topology_reset(void); > > #endif > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index e1f6925856..848314d2a9 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -641,6 +641,7 @@ typedef struct SysIBTl_cpu { > QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16); > > void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar); > +void s390_cpu_topology_reset(void); How about you call this s390_cpu_topology_reset_modified, so it's symmetric with the function you define in the next patch. You could also drop the "cpu" from the name. Or maybe even better, you only define a function for setting the modified state, but make it take a bool argument. This way you also get rid of some code duplication and it wouldn't harm readability IMO. > > /* 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 a80a1ebf22..cf63f3dd01 100644 > --- a/hw/s390x/cpu-topology.c > +++ b/hw/s390x/cpu-topology.c > @@ -85,6 +85,18 @@ static void s390_topology_init(MachineState *ms) > QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next); > } > > +/** > + * s390_topology_reset: > + * > + * Generic reset for CPU topology, calls s390_topology_reset() > + * s390_topology_reset() to reset the kernel Modified Topology > + * change record. > + */ > +void s390_topology_reset(void) > +{ I'm wondering if you shouldn't move the reset changes you do in the next patch into this one. I don't see what they have to do with PTF emulation. > + s390_cpu_topology_reset(); > +} > + > /** > * 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 9bc51a83f4..30fdfe41fa 100644 > --- a/hw/s390x/s390-virtio-ccw.c > +++ b/hw/s390x/s390-virtio-ccw.c > @@ -122,6 +122,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 948e4bd3e0..e27864c5f5 100644 > --- a/target/s390x/cpu-sysemu.c > +++ b/target/s390x/cpu-sysemu.c > @@ -306,3 +306,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_reset(void) > +{ > + int ret; > + > + if (kvm_enabled()) { > + ret = kvm_s390_topology_set_mtcr(0); > + 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 5ea358cbb0..bc953151ce 100644 > --- a/target/s390x/kvm/kvm.c > +++ b/target/s390x/kvm/kvm.c > @@ -2592,6 +2592,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) > { > }
On 2/6/23 18:52, Nina Schoetterl-Glausch wrote: > On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote: >> 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> >> --- >> include/hw/s390x/cpu-topology.h | 1 + >> target/s390x/cpu.h | 1 + >> target/s390x/kvm/kvm_s390x.h | 1 + >> hw/s390x/cpu-topology.c | 12 ++++++++++++ >> hw/s390x/s390-virtio-ccw.c | 3 +++ >> target/s390x/cpu-sysemu.c | 13 +++++++++++++ >> target/s390x/kvm/kvm.c | 17 +++++++++++++++++ >> 7 files changed, 48 insertions(+) >> >> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h >> index 1ae7e7c5e3..60e0b9fbfa 100644 >> --- a/include/hw/s390x/cpu-topology.h >> +++ b/include/hw/s390x/cpu-topology.h >> @@ -66,5 +66,6 @@ static inline void s390_topology_set_cpu(MachineState *ms, >> >> extern S390Topology s390_topology; >> int s390_socket_nb(S390CPU *cpu); >> +void s390_topology_reset(void); >> >> #endif >> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >> index e1f6925856..848314d2a9 100644 >> --- a/target/s390x/cpu.h >> +++ b/target/s390x/cpu.h >> @@ -641,6 +641,7 @@ typedef struct SysIBTl_cpu { >> QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16); >> >> void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar); >> +void s390_cpu_topology_reset(void); > > How about you call this s390_cpu_topology_reset_modified, so it's symmetric > with the function you define in the next patch. You could also drop the "cpu" > from the name. I am not sure about this, Thomas already gave his R-B on this patch so I prefer to stay on the original name, unless he says it is a good idea to change. Also in cpu-sysemu.c most of the function are tagged with _cpu_ > > Or maybe even better, you only define a function for setting the modified state, > but make it take a bool argument. This way you also get rid of some code duplication > and it wouldn't harm readability IMO. There is already a single function kvm_s390_topology_set_mtcr(attr) to set the "modified state" > >> >> /* 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 a80a1ebf22..cf63f3dd01 100644 >> --- a/hw/s390x/cpu-topology.c >> +++ b/hw/s390x/cpu-topology.c >> @@ -85,6 +85,18 @@ static void s390_topology_init(MachineState *ms) >> QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next); >> } >> >> +/** >> + * s390_topology_reset: >> + * >> + * Generic reset for CPU topology, calls s390_topology_reset() >> + * s390_topology_reset() to reset the kernel Modified Topology >> + * change record. >> + */ >> +void s390_topology_reset(void) >> +{ > > I'm wondering if you shouldn't move the reset changes you do in the next patch > into this one. I don't see what they have to do with PTF emulation. Here in this patch we do not intercept PTF and we have only an horizontal polarity. So we do not need to reset the polarity for all the vCPUs, we only need it when we have vertical polarity. Regards, Pierre
On Tue, 2023-02-07 at 10:24 +0100, Pierre Morel wrote: > > On 2/6/23 18:52, Nina Schoetterl-Glausch wrote: > > On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote: > > > 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> > > > --- > > > include/hw/s390x/cpu-topology.h | 1 + > > > target/s390x/cpu.h | 1 + > > > target/s390x/kvm/kvm_s390x.h | 1 + > > > hw/s390x/cpu-topology.c | 12 ++++++++++++ > > > hw/s390x/s390-virtio-ccw.c | 3 +++ > > > target/s390x/cpu-sysemu.c | 13 +++++++++++++ > > > target/s390x/kvm/kvm.c | 17 +++++++++++++++++ > > > 7 files changed, 48 insertions(+) > > > > > > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h > > > index 1ae7e7c5e3..60e0b9fbfa 100644 > > > --- a/include/hw/s390x/cpu-topology.h > > > +++ b/include/hw/s390x/cpu-topology.h > > > @@ -66,5 +66,6 @@ static inline void s390_topology_set_cpu(MachineState *ms, > > > > > > extern S390Topology s390_topology; > > > int s390_socket_nb(S390CPU *cpu); > > > +void s390_topology_reset(void); > > > > > > #endif > > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > > > index e1f6925856..848314d2a9 100644 > > > --- a/target/s390x/cpu.h > > > +++ b/target/s390x/cpu.h > > > @@ -641,6 +641,7 @@ typedef struct SysIBTl_cpu { > > > QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16); > > > > > > void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar); > > > +void s390_cpu_topology_reset(void); > > > > How about you call this s390_cpu_topology_reset_modified, so it's symmetric > > with the function you define in the next patch. You could also drop the "cpu" > > from the name. > > I am not sure about this, Thomas already gave his R-B on this patch so I > prefer to stay on the original name, unless he says it is a good idea to > change. > Also in cpu-sysemu.c most of the function are tagged with _cpu_ IMO, renaming a function would be a small enough change to keep an R-b. > > > > > Or maybe even better, you only define a function for setting the modified state, > > but make it take a bool argument. This way you also get rid of some code duplication > > and it wouldn't harm readability IMO. > > There is already a single function kvm_s390_topology_set_mtcr(attr) to > set the "modified state" Yes, but that is for KVM only and doesn't do error handling. So you need at least one function on top of that. What I'm suggesting is to only have one function instead of two because it gets rid of some code. > > > > > > > > > /* 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 a80a1ebf22..cf63f3dd01 100644 > > > --- a/hw/s390x/cpu-topology.c > > > +++ b/hw/s390x/cpu-topology.c > > > @@ -85,6 +85,18 @@ static void s390_topology_init(MachineState *ms) > > > QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next); > > > } > > > > > > +/** > > > + * s390_topology_reset: > > > + * > > > + * Generic reset for CPU topology, calls s390_topology_reset() > > > + * s390_topology_reset() to reset the kernel Modified Topology > > > + * change record. > > > + */ > > > +void s390_topology_reset(void) > > > +{ > > > > I'm wondering if you shouldn't move the reset changes you do in the next patch > > into this one. I don't see what they have to do with PTF emulation. > > Here in this patch we do not intercept PTF and we have only an > horizontal polarity. > So we do not need to reset the polarity for all the vCPUs, we only need > it when we have vertical polarity. Well, with the PTF patch you don't get vertical polarity either, because you only enable the topology with patch 7. And since it's about resetting, it fits better in this patch IMO. > > Regards, > Pierre >
On 2/7/23 11:50, Nina Schoetterl-Glausch wrote: > On Tue, 2023-02-07 at 10:24 +0100, Pierre Morel wrote: >> >> On 2/6/23 18:52, Nina Schoetterl-Glausch wrote: >>> On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote: >>>> 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> >>>> --- >>>> include/hw/s390x/cpu-topology.h | 1 + >>>> target/s390x/cpu.h | 1 + >>>> target/s390x/kvm/kvm_s390x.h | 1 + >>>> hw/s390x/cpu-topology.c | 12 ++++++++++++ >>>> hw/s390x/s390-virtio-ccw.c | 3 +++ >>>> target/s390x/cpu-sysemu.c | 13 +++++++++++++ >>>> target/s390x/kvm/kvm.c | 17 +++++++++++++++++ >>>> 7 files changed, 48 insertions(+) >>>> >>>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h >>>> index 1ae7e7c5e3..60e0b9fbfa 100644 >>>> --- a/include/hw/s390x/cpu-topology.h >>>> +++ b/include/hw/s390x/cpu-topology.h >>>> @@ -66,5 +66,6 @@ static inline void s390_topology_set_cpu(MachineState *ms, >>>> >>>> extern S390Topology s390_topology; >>>> int s390_socket_nb(S390CPU *cpu); >>>> +void s390_topology_reset(void); >>>> >>>> #endif >>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>>> index e1f6925856..848314d2a9 100644 >>>> --- a/target/s390x/cpu.h >>>> +++ b/target/s390x/cpu.h >>>> @@ -641,6 +641,7 @@ typedef struct SysIBTl_cpu { >>>> QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16); >>>> >>>> void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar); >>>> +void s390_cpu_topology_reset(void); >>> >>> How about you call this s390_cpu_topology_reset_modified, so it's symmetric >>> with the function you define in the next patch. You could also drop the "cpu" >>> from the name. >> >> I am not sure about this, Thomas already gave his R-B on this patch so I >> prefer to stay on the original name, unless he says it is a good idea to >> change. >> Also in cpu-sysemu.c most of the function are tagged with _cpu_ > > IMO, renaming a function would be a small enough change to keep an R-b. >> >>> >>> Or maybe even better, you only define a function for setting the modified state, >>> but make it take a bool argument. This way you also get rid of some code duplication >>> and it wouldn't harm readability IMO. >> >> There is already a single function kvm_s390_topology_set_mtcr(attr) to >> set the "modified state" > > Yes, but that is for KVM only and doesn't do error handling. > So you need at least one function on top of that. What I'm suggesting is to > only have one function instead of two because it gets rid of some code. OK this is right. I rename void s390_cpu_topology_reset(void); to void s390_cpu_topology_set_mtcr(int value); and then: - s390_cpu_topology_reset(); + s390_cpu_topology_set_mtcr(0); >> >>> >>>> >>>> /* 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 a80a1ebf22..cf63f3dd01 100644 >>>> --- a/hw/s390x/cpu-topology.c >>>> +++ b/hw/s390x/cpu-topology.c >>>> @@ -85,6 +85,18 @@ static void s390_topology_init(MachineState *ms) >>>> QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next); >>>> } >>>> >>>> +/** >>>> + * s390_topology_reset: >>>> + * >>>> + * Generic reset for CPU topology, calls s390_topology_reset() >>>> + * s390_topology_reset() to reset the kernel Modified Topology >>>> + * change record. >>>> + */ >>>> +void s390_topology_reset(void) >>>> +{ >>> >>> I'm wondering if you shouldn't move the reset changes you do in the next patch >>> into this one. I don't see what they have to do with PTF emulation. >> >> Here in this patch we do not intercept PTF and we have only an >> horizontal polarity. >> So we do not need to reset the polarity for all the vCPUs, we only need >> it when we have vertical polarity. > > Well, with the PTF patch you don't get vertical polarity either, because you > only enable the topology with patch 7. > And since it's about resetting, it fits better in this patch IMO. Not in my opinion, suppose the next patch never get included it has no sense. However if there is a majority in favor of this change I will do it. Regards, Pierre
On Tue, 2023-02-07 at 13:19 +0100, Pierre Morel wrote: > > On 2/7/23 11:50, Nina Schoetterl-Glausch wrote: > > On Tue, 2023-02-07 at 10:24 +0100, Pierre Morel wrote: > > > > > > On 2/6/23 18:52, Nina Schoetterl-Glausch wrote: > > > > On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote: > > > > > 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> > > > > > --- > > > > > include/hw/s390x/cpu-topology.h | 1 + > > > > > target/s390x/cpu.h | 1 + > > > > > target/s390x/kvm/kvm_s390x.h | 1 + > > > > > hw/s390x/cpu-topology.c | 12 ++++++++++++ > > > > > hw/s390x/s390-virtio-ccw.c | 3 +++ > > > > > target/s390x/cpu-sysemu.c | 13 +++++++++++++ > > > > > target/s390x/kvm/kvm.c | 17 +++++++++++++++++ > > > > > 7 files changed, 48 insertions(+) > > > > > > > > > > diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h > > > > > index 1ae7e7c5e3..60e0b9fbfa 100644 > > > > > --- a/include/hw/s390x/cpu-topology.h > > > > > +++ b/include/hw/s390x/cpu-topology.h > > > > > @@ -66,5 +66,6 @@ static inline void s390_topology_set_cpu(MachineState *ms, > > > > > > > > > > extern S390Topology s390_topology; > > > > > int s390_socket_nb(S390CPU *cpu); > > > > > +void s390_topology_reset(void); > > > > > > > > > > #endif > > > > > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > > > > > index e1f6925856..848314d2a9 100644 > > > > > --- a/target/s390x/cpu.h > > > > > +++ b/target/s390x/cpu.h > > > > > @@ -641,6 +641,7 @@ typedef struct SysIBTl_cpu { > > > > > QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16); > > > > > > > > > > void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar); > > > > > +void s390_cpu_topology_reset(void); > > > > > > > > How about you call this s390_cpu_topology_reset_modified, so it's symmetric > > > > with the function you define in the next patch. You could also drop the "cpu" > > > > from the name. > > > > > > I am not sure about this, Thomas already gave his R-B on this patch so I > > > prefer to stay on the original name, unless he says it is a good idea to > > > change. > > > Also in cpu-sysemu.c most of the function are tagged with _cpu_ > > > > IMO, renaming a function would be a small enough change to keep an R-b. > > > > > > > > > > > Or maybe even better, you only define a function for setting the modified state, > > > > but make it take a bool argument. This way you also get rid of some code duplication > > > > and it wouldn't harm readability IMO. > > > > > > There is already a single function kvm_s390_topology_set_mtcr(attr) to > > > set the "modified state" > > > > Yes, but that is for KVM only and doesn't do error handling. > > So you need at least one function on top of that. What I'm suggesting is to > > only have one function instead of two because it gets rid of some code. > > OK this is right. > I rename > void s390_cpu_topology_reset(void); > to > void s390_cpu_topology_set_mtcr(int value); I don't find mtcr very descriptive and a bit of a SIE/KVM name, it might not fit a possible future tcg implementation. I'd just call it s390_cpu_topology_set_changed/modified, and have it take a bool, because I cannot imagine other int values to make sense. > > and then: > > - s390_cpu_topology_reset(); > + s390_cpu_topology_set_mtcr(0); > > > > > > > > > > > > > > > > > > > /* 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 a80a1ebf22..cf63f3dd01 100644 > > > > > --- a/hw/s390x/cpu-topology.c > > > > > +++ b/hw/s390x/cpu-topology.c > > > > > @@ -85,6 +85,18 @@ static void s390_topology_init(MachineState *ms) > > > > > QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next); > > > > > } > > > > > > > > > > +/** > > > > > + * s390_topology_reset: > > > > > + * > > > > > + * Generic reset for CPU topology, calls s390_topology_reset() > > > > > + * s390_topology_reset() to reset the kernel Modified Topology > > > > > + * change record. > > > > > + */ > > > > > +void s390_topology_reset(void) > > > > > +{ > > > > > > > > I'm wondering if you shouldn't move the reset changes you do in the next patch > > > > into this one. I don't see what they have to do with PTF emulation. > > > > > > Here in this patch we do not intercept PTF and we have only an > > > horizontal polarity. > > > So we do not need to reset the polarity for all the vCPUs, we only need > > > it when we have vertical polarity. > > > > Well, with the PTF patch you don't get vertical polarity either, because you > > only enable the topology with patch 7. > > And since it's about resetting, it fits better in this patch IMO. > > Not in my opinion, suppose the next patch never get included it has no > sense. Well, yes, but then the series would be broken, since the facility requires PTF to work. > However if there is a majority in favor of this change I will do it. > > Regards, > Pierre >
On 2/7/23 14:37, Nina Schoetterl-Glausch wrote: > On Tue, 2023-02-07 at 13:19 +0100, Pierre Morel wrote: >> >> On 2/7/23 11:50, Nina Schoetterl-Glausch wrote: >>> On Tue, 2023-02-07 at 10:24 +0100, Pierre Morel wrote: >>>> >>>> On 2/6/23 18:52, Nina Schoetterl-Glausch wrote: >>>>> On Wed, 2023-02-01 at 14:20 +0100, Pierre Morel wrote: >>>>>> 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> >>>>>> --- >>>>>> include/hw/s390x/cpu-topology.h | 1 + >>>>>> target/s390x/cpu.h | 1 + >>>>>> target/s390x/kvm/kvm_s390x.h | 1 + >>>>>> hw/s390x/cpu-topology.c | 12 ++++++++++++ >>>>>> hw/s390x/s390-virtio-ccw.c | 3 +++ >>>>>> target/s390x/cpu-sysemu.c | 13 +++++++++++++ >>>>>> target/s390x/kvm/kvm.c | 17 +++++++++++++++++ >>>>>> 7 files changed, 48 insertions(+) >>>>>> >>>>>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h >>>>>> index 1ae7e7c5e3..60e0b9fbfa 100644 >>>>>> --- a/include/hw/s390x/cpu-topology.h >>>>>> +++ b/include/hw/s390x/cpu-topology.h >>>>>> @@ -66,5 +66,6 @@ static inline void s390_topology_set_cpu(MachineState *ms, >>>>>> >>>>>> extern S390Topology s390_topology; >>>>>> int s390_socket_nb(S390CPU *cpu); >>>>>> +void s390_topology_reset(void); >>>>>> >>>>>> #endif >>>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h >>>>>> index e1f6925856..848314d2a9 100644 >>>>>> --- a/target/s390x/cpu.h >>>>>> +++ b/target/s390x/cpu.h >>>>>> @@ -641,6 +641,7 @@ typedef struct SysIBTl_cpu { >>>>>> QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16); >>>>>> >>>>>> void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar); >>>>>> +void s390_cpu_topology_reset(void); >>>>> >>>>> How about you call this s390_cpu_topology_reset_modified, so it's symmetric >>>>> with the function you define in the next patch. You could also drop the "cpu" >>>>> from the name. >>>> >>>> I am not sure about this, Thomas already gave his R-B on this patch so I >>>> prefer to stay on the original name, unless he says it is a good idea to >>>> change. >>>> Also in cpu-sysemu.c most of the function are tagged with _cpu_ >>> >>> IMO, renaming a function would be a small enough change to keep an R-b. >>>> >>>>> >>>>> Or maybe even better, you only define a function for setting the modified state, >>>>> but make it take a bool argument. This way you also get rid of some code duplication >>>>> and it wouldn't harm readability IMO. >>>> >>>> There is already a single function kvm_s390_topology_set_mtcr(attr) to >>>> set the "modified state" >>> >>> Yes, but that is for KVM only and doesn't do error handling. >>> So you need at least one function on top of that. What I'm suggesting is to >>> only have one function instead of two because it gets rid of some code. >> >> OK this is right. >> I rename >> void s390_cpu_topology_reset(void); >> to >> void s390_cpu_topology_set_mtcr(int value); > > I don't find mtcr very descriptive and a bit of a SIE/KVM name, it might not > fit a possible future tcg implementation. > I'd just call it s390_cpu_topology_set_changed/modified, and have it take a bool, > because I cannot imagine other int values to make sense. OK > >> >> and then: >> >> - s390_cpu_topology_reset(); >> + s390_cpu_topology_set_mtcr(0); >> >> >>>> >>>>> >>>>>> >>>>>> /* 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 a80a1ebf22..cf63f3dd01 100644 >>>>>> --- a/hw/s390x/cpu-topology.c >>>>>> +++ b/hw/s390x/cpu-topology.c >>>>>> @@ -85,6 +85,18 @@ static void s390_topology_init(MachineState *ms) >>>>>> QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next); >>>>>> } >>>>>> >>>>>> +/** >>>>>> + * s390_topology_reset: >>>>>> + * >>>>>> + * Generic reset for CPU topology, calls s390_topology_reset() >>>>>> + * s390_topology_reset() to reset the kernel Modified Topology >>>>>> + * change record. >>>>>> + */ >>>>>> +void s390_topology_reset(void) >>>>>> +{ >>>>> >>>>> I'm wondering if you shouldn't move the reset changes you do in the next patch >>>>> into this one. I don't see what they have to do with PTF emulation. >>>> >>>> Here in this patch we do not intercept PTF and we have only an >>>> horizontal polarity. >>>> So we do not need to reset the polarity for all the vCPUs, we only need >>>> it when we have vertical polarity. >>> >>> Well, with the PTF patch you don't get vertical polarity either, because you >>> only enable the topology with patch 7. >>> And since it's about resetting, it fits better in this patch IMO. >> >> Not in my opinion, suppose the next patch never get included it has no >> sense. > > Well, yes, but then the series would be broken, since the facility requires PTF to work. Yes, same if the activation of the facility is not included. Regards, Pierre
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h index 1ae7e7c5e3..60e0b9fbfa 100644 --- a/include/hw/s390x/cpu-topology.h +++ b/include/hw/s390x/cpu-topology.h @@ -66,5 +66,6 @@ static inline void s390_topology_set_cpu(MachineState *ms, extern S390Topology s390_topology; int s390_socket_nb(S390CPU *cpu); +void s390_topology_reset(void); #endif diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index e1f6925856..848314d2a9 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -641,6 +641,7 @@ typedef struct SysIBTl_cpu { QEMU_BUILD_BUG_ON(sizeof(SysIBTl_cpu) != 16); void insert_stsi_15_1_x(S390CPU *cpu, int sel2, __u64 addr, uint8_t ar); +void s390_cpu_topology_reset(void); /* 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 a80a1ebf22..cf63f3dd01 100644 --- a/hw/s390x/cpu-topology.c +++ b/hw/s390x/cpu-topology.c @@ -85,6 +85,18 @@ static void s390_topology_init(MachineState *ms) QTAILQ_INSERT_HEAD(&s390_topology.list, entry, next); } +/** + * s390_topology_reset: + * + * Generic reset for CPU topology, calls s390_topology_reset() + * s390_topology_reset() to reset the kernel Modified Topology + * change record. + */ +void s390_topology_reset(void) +{ + s390_cpu_topology_reset(); +} + /** * 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 9bc51a83f4..30fdfe41fa 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -122,6 +122,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 948e4bd3e0..e27864c5f5 100644 --- a/target/s390x/cpu-sysemu.c +++ b/target/s390x/cpu-sysemu.c @@ -306,3 +306,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_reset(void) +{ + int ret; + + if (kvm_enabled()) { + ret = kvm_s390_topology_set_mtcr(0); + 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 5ea358cbb0..bc953151ce 100644 --- a/target/s390x/kvm/kvm.c +++ b/target/s390x/kvm/kvm.c @@ -2592,6 +2592,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) { }
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> --- include/hw/s390x/cpu-topology.h | 1 + target/s390x/cpu.h | 1 + target/s390x/kvm/kvm_s390x.h | 1 + hw/s390x/cpu-topology.c | 12 ++++++++++++ hw/s390x/s390-virtio-ccw.c | 3 +++ target/s390x/cpu-sysemu.c | 13 +++++++++++++ target/s390x/kvm/kvm.c | 17 +++++++++++++++++ 7 files changed, 48 insertions(+)