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

Thomas Huth posted 25 patches 1 year, 1 month ago
Maintainers: Nina Schoetterl-Glausch <nsg@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Michael Roth <michael.roth@amd.com>, Cleber Rosa <crosa@redhat.com>, Wainer dos Santos Moschetta <wainersm@redhat.com>, Beraldo Leal <bleal@redhat.com>, Juan Quintela <quintela@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Leonardo Bras <leobras@redhat.com>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PULL 06/25] s390x/cpu topology: resetting the Topology-Change-Report
Posted by Thomas Huth 1 year, 1 month ago
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);
 
 static inline int s390_std_socket(int n, CpuTopology *smp)
 {
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 09bff39fe4..40c5cedd0e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -654,6 +654,7 @@ typedef struct SysIBCPUListEntry {
 QEMU_BUILD_BUG_ON(sizeof(SysIBCPUListEntry) != 16);
 
 void insert_stsi_15_1_x(S390CPU *cpu, int sel2, uint64_t addr, uint8_t ar, uintptr_t ra);
+void s390_cpu_topology_set_changed(bool changed);
 
 /* 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 13168341b6..7ec9319272 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -90,6 +90,17 @@ static void s390_topology_init(MachineState *ms)
                                             smp->books * smp->drawers);
 }
 
+/**
+ * s390_topology_reset:
+ *
+ * Generic reset for CPU topology, calls s390_topology_reset()
+ * to reset the kernel Modified Topology Change Record.
+ */
+void s390_topology_reset(void)
+{
+    s390_cpu_topology_set_changed(false);
+}
+
 /**
  * s390_topology_cpu_default:
  * @cpu: pointer to a S390CPU
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 7fe2bce20c..6012165d41 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -124,6 +124,9 @@ static void subsystem_reset(void)
             device_cold_reset(dev);
         }
     }
+    if (s390_has_topology()) {
+        s390_topology_reset();
+    }
 }
 
 static int virtio_ccw_hcall_notify(const uint64_t *args)
diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
index 8112561e5e..1cd30c1d84 100644
--- a/target/s390x/cpu-sysemu.c
+++ b/target/s390x/cpu-sysemu.c
@@ -307,3 +307,16 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg)
         kvm_s390_set_diag318(cs, arg.host_ulong);
     }
 }
+
+void s390_cpu_topology_set_changed(bool changed)
+{
+    int ret;
+
+    if (kvm_enabled()) {
+        ret = kvm_s390_topology_set_mtcr(changed);
+        if (ret) {
+            error_report("Failed to set Modified Topology Change Report: %s",
+                         strerror(-ret));
+        }
+    }
+}
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 53d6300809..d6bda3a2a8 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2664,6 +2664,23 @@ int kvm_s390_get_zpci_op(void)
     return cap_zpci_op;
 }
 
+int kvm_s390_topology_set_mtcr(uint64_t attr)
+{
+    struct kvm_device_attr attribute = {
+        .group = KVM_S390_VM_CPU_TOPOLOGY,
+        .attr  = attr,
+    };
+
+    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
+        return 0;
+    }
+    if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) {
+        return -ENOTSUP;
+    }
+
+    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute);
+}
+
 void kvm_arch_accel_class_init(ObjectClass *oc)
 {
 }
-- 
2.41.0
Re: [PULL 06/25] s390x/cpu topology: resetting the Topology-Change-Report
Posted by Stefan Hajnoczi 1 year, 1 month ago
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

>
>  static inline int s390_std_socket(int n, CpuTopology *smp)
>  {
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 09bff39fe4..40c5cedd0e 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -654,6 +654,7 @@ typedef struct SysIBCPUListEntry {
>  QEMU_BUILD_BUG_ON(sizeof(SysIBCPUListEntry) != 16);
>
>  void insert_stsi_15_1_x(S390CPU *cpu, int sel2, uint64_t addr, uint8_t ar, uintptr_t ra);
> +void s390_cpu_topology_set_changed(bool changed);
>
>  /* 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 13168341b6..7ec9319272 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -90,6 +90,17 @@ static void s390_topology_init(MachineState *ms)
>                                              smp->books * smp->drawers);
>  }
>
> +/**
> + * s390_topology_reset:
> + *
> + * Generic reset for CPU topology, calls s390_topology_reset()
> + * to reset the kernel Modified Topology Change Record.
> + */
> +void s390_topology_reset(void)
> +{
> +    s390_cpu_topology_set_changed(false);
> +}
> +
>  /**
>   * s390_topology_cpu_default:
>   * @cpu: pointer to a S390CPU
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 7fe2bce20c..6012165d41 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -124,6 +124,9 @@ static void subsystem_reset(void)
>              device_cold_reset(dev);
>          }
>      }
> +    if (s390_has_topology()) {
> +        s390_topology_reset();
> +    }
>  }
>
>  static int virtio_ccw_hcall_notify(const uint64_t *args)
> diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
> index 8112561e5e..1cd30c1d84 100644
> --- a/target/s390x/cpu-sysemu.c
> +++ b/target/s390x/cpu-sysemu.c
> @@ -307,3 +307,16 @@ void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg)
>          kvm_s390_set_diag318(cs, arg.host_ulong);
>      }
>  }
> +
> +void s390_cpu_topology_set_changed(bool changed)
> +{
> +    int ret;
> +
> +    if (kvm_enabled()) {
> +        ret = kvm_s390_topology_set_mtcr(changed);
> +        if (ret) {
> +            error_report("Failed to set Modified Topology Change Report: %s",
> +                         strerror(-ret));
> +        }
> +    }
> +}
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 53d6300809..d6bda3a2a8 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -2664,6 +2664,23 @@ int kvm_s390_get_zpci_op(void)
>      return cap_zpci_op;
>  }
>
> +int kvm_s390_topology_set_mtcr(uint64_t attr)
> +{
> +    struct kvm_device_attr attribute = {
> +        .group = KVM_S390_VM_CPU_TOPOLOGY,
> +        .attr  = attr,
> +    };
> +
> +    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
> +        return 0;
> +    }
> +    if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) {
> +        return -ENOTSUP;
> +    }
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute);
> +}
> +
>  void kvm_arch_accel_class_init(ObjectClass *oc)
>  {
>  }
> --
> 2.41.0
>
>
Re: [PULL 06/25] s390x/cpu topology: resetting the Topology-Change-Report
Posted by Nina Schoetterl-Glausch 1 year, 1 month ago
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.
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)
 {
Re: [PULL 06/25] s390x/cpu topology: resetting the Topology-Change-Report
Posted by Thomas Huth 1 year, 1 month ago
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
Re: [PULL 06/25] s390x/cpu topology: resetting the Topology-Change-Report
Posted by Nina Schoetterl-Glausch 1 year, 1 month ago
On Thu, 2023-10-19 at 21:32 +0200, Thomas Huth wrote:

[...]

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

Thanks!