Re: [PATCH v9 06/10] s390x/cpu_topology: resetting the Topology-Change-Report

[Janis Schoetterl-Glausch <scgl@linux.ibm.com>]

On Fri, 2022-09-02 at 09:55 +0200, 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>
> ---
>  hw/s390x/cpu-topology.c      | 12 ++++++++++++
>  hw/s390x/s390-virtio-ccw.c   |  1 +
>  target/s390x/cpu-sysemu.c    |  7 +++++++
>  target/s390x/cpu.h           |  1 +
>  target/s390x/kvm/kvm.c       | 23 +++++++++++++++++++++++
>  target/s390x/kvm/kvm_s390x.h |  1 +
>  6 files changed, 45 insertions(+)

[...]

> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index f96630440b..9c994d27d5 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -2585,3 +2585,26 @@ 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,
> +    };
> +    int ret;
> +
> +    if (!s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY)) {
> +        return -EFAULT;

Why EFAULT?
The return value is just ignored when resetting, isn't it?
I wonder if it would be better not to.
Is it necessary because you're detecting the feature after you've
already created the S390Topology instance?
And you're doing that because that's just the order in which QEMU does
things? So the machine class is inited before the cpu model?
I wonder if there is a nice way to create the S390Topology only if the
feature is selected.

Anyway:
Reviewed-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>

> +    }
> +    if (!kvm_vm_check_attr(kvm_state, KVM_S390_VM_CPU_TOPOLOGY, attr)) {
> +        return -ENOENT;
> +    }
> +
> +    ret = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attribute);
> +    if (ret) {
> +        error_report("Failed to set cpu topology attribute %lu: %s",
> +                     attr, strerror(-ret));
> +    }
> +    return ret;
> +}
> 
[...]