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

[Pierre Morel <pmorel@linux.ibm.com>]

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


-- 
Pierre Morel
IBM Lab Boeblingen