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.
>
> 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
>
