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

[Pierre Morel]

On 9/8/22 09:57, Janis Schoetterl-Glausch wrote:
> 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?
any proposition?

> The return value is just ignored when resetting, isn't it?

In migration the same function is used and we need to return an error there.
But if we would not use it, after the comments you did for migration, we 
may indeed not need it, then I guess using error_report and a void 
function may be better.

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

I do not know if it is possible. I will look.

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

Thanks,
Pierre

> > +    }
> > +    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;
> > +}
> [...]

--
Pierre Morel
IBM Lab Boeblingen