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

[Nina Schoetterl-Glausch]

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
>