Re: [PATCH v15 05/11] s390x/cpu topology: resetting the Topology-Change-Report

[Nina Schoetterl-Glausch <nsg@linux.ibm.com>]

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
>