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@xxxxxxxxxxxxx>
---
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
--
Pierre Morel
IBM Lab Boeblingen
