Re: [PULL 06/25] s390x/cpu topology: resetting the Topology-Change-Report

[Thomas Huth <thuth@redhat.com>]

On 19/10/2023 19.55, Nina Schoetterl-Glausch wrote:
> On Thu, 2023-10-19 at 09:35 -0700, Stefan Hajnoczi wrote:
>> On Wed, 18 Oct 2023 at 06:09, Thomas Huth <thuth@redhat.com> wrote:
>>>
>>> From: Pierre Morel <pmorel@linux.ibm.com>
>>>
>>> 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>
>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>> Co-developed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>> Message-ID: <20231016183925.2384704-7-nsg@linux.ibm.com>
>>> Signed-off-by: Thomas Huth <thuth@redhat.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         | 11 +++++++++++
>>>   hw/s390x/s390-virtio-ccw.c      |  3 +++
>>>   target/s390x/cpu-sysemu.c       | 13 +++++++++++++
>>>   target/s390x/kvm/kvm.c          | 17 +++++++++++++++++
>>>   7 files changed, 47 insertions(+)
>>>
>>> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
>>> index f95d26d37c..e33e7c66df 100644
>>> --- a/include/hw/s390x/cpu-topology.h
>>> +++ b/include/hw/s390x/cpu-topology.h
>>> @@ -56,6 +56,7 @@ static inline void s390_topology_setup_cpu(MachineState *ms,
>>>   #endif
>>>
>>>   extern S390Topology s390_topology;
>>> +void s390_topology_reset(void);
>>
>> Please take a look at the following CI failure:
>>
>> /usr/bin/ld: libqemu-s390x-softmmu.fa.p/hw_s390x_s390-virtio-ccw.c.o:
>> in function `subsystem_reset':
>> /home/gitlab-runner/builds/E8PpwMky/0/qemu-project/qemu/build/../hw/s390x/s390-virtio-ccw.c:128:
>> undefined reference to `s390_topology_reset'
>>
>> https://gitlab.com/qemu-project/qemu/-/jobs/5330218593
> 
> I can replicate this with --disable-kvm, tho I don't think that's what the CI does.

I think that was the wrong CI job that Stefan linked. It rather seemed to 
happen here:

  https://gitlab.com/qemu-project/qemu/-/jobs/5329820093#L5564

That job uses --enable-debug which turns off optimization, i.e. that was 
likely causing some code to be included that normally gets optimized away.

> Fix looks something like this (copy pasted):
> 
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -45,6 +45,7 @@ typedef QTAILQ_HEAD(, S390TopologyEntry) S390TopologyList;
>   #ifdef CONFIG_KVM
>   bool s390_has_topology(void);
>   void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error **errp);
> +void s390_topology_reset(void);
>   #else
>   static inline bool s390_has_topology(void)
>   {
> @@ -53,10 +54,14 @@ static inline bool s390_has_topology(void)
>   static inline void s390_topology_setup_cpu(MachineState *ms,
>                                              S390CPU *cpu,
>                                              Error **errp) {}
> +static inline void s390_topology_reset(void)
> +{
> +    /* Unreachable, CPU topology not implemented for TCG */
> +    assert(false);
> +}
>   #endif
> 
>   extern S390Topology s390_topology;
> -void s390_topology_reset(void);
> 
>   static inline int s390_std_socket(int n, CpuTopology *smp)
>   {

Thanks, that seems to fix the issue with --enable-debug, too.
I'll squash that into the related patch (also fixing the indentation in 
s390_has_topology()) and respin the pull request.

  Thomas