Re: [PATCH v14 07/11] target/s390x/cpu topology: activating CPU topology

[Pierre Morel]

On 1/11/23 11:04, Thomas Huth wrote:
> On 05/01/2023 15.53, Pierre Morel wrote:
> > The KVM capability, KVM_CAP_S390_CPU_TOPOLOGY is used to
>
> Remove the "," in above line?

OK

> > activate the S390_FEAT_CONFIGURATION_TOPOLOGY feature and
> > the topology facility for the guest in the case the topology
>
> I'd like to suggest to add "in the host CPU model" after "facility".

Yes, thanks.

> > is available in QEMU and in KVM.
> >
> > The feature is disabled by default and fenced for SE
> > (secure execution).
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   hw/s390x/cpu-topology.c   |  2 +-
> >   target/s390x/cpu_models.c |  1 +
> >   target/s390x/kvm/kvm.c    | 13 +++++++++++++
> >   3 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> > index e6b4692581..b69955a1cd 100644
> > --- a/hw/s390x/cpu-topology.c
> > +++ b/hw/s390x/cpu-topology.c
> > @@ -52,7 +52,7 @@ static int s390_socket_nb(s390_topology_id id)
> >    */
> >   bool s390_has_topology(void)
> >   {
> > -    return false;
> > +    return s390_has_feat(S390_FEAT_CONFIGURATION_TOPOLOGY);
> >   }
> >   /**
> > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> > index c3a4f80633..3f05e05fd3 100644
> > --- a/target/s390x/cpu_models.c
> > +++ b/target/s390x/cpu_models.c
> > @@ -253,6 +253,7 @@ bool s390_has_feat(S390Feat feat)
> >           case S390_FEAT_SIE_CMMA:
> >           case S390_FEAT_SIE_PFMFI:
> >           case S390_FEAT_SIE_IBS:
> > +        case S390_FEAT_CONFIGURATION_TOPOLOGY:
> >               return false;
> >               break;
> >           default:
> > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> > index fb63be41b7..4e2a2ff516 100644
> > --- a/target/s390x/kvm/kvm.c
> > +++ b/target/s390x/kvm/kvm.c
> > @@ -2470,6 +2470,19 @@ void kvm_s390_get_host_cpu_model(S390CPUModel 
> > *model, Error **errp)
> >           set_bit(S390_FEAT_UNPACK, model->features);
> >       }
> > +    /*
> > +     * If we have support for CPU Topology prevent overrule
> > +     * S390_FEAT_CONFIGURATION_TOPOLOGY with 
> > S390_FEAT_DISABLE_CPU_TOPOLOGY
>
> That S390_FEAT_DISABLE_CPU_TOPOLOGY looks like a leftover from v12 ?

Right, sorry, I remove it and change the comment for:

    /*
     * If we have kernel support for CPU Topology indicate the
     * configuration-topology facility.
     */


> Apart from that, patch looks fine to me now.
>
>   Thomas
>
>
> > +     * implemented in KVM, activate the CPU TOPOLOGY feature.
> > +     */
> > +    if (kvm_check_extension(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY)) {
> > +        if (kvm_vm_enable_cap(kvm_state, KVM_CAP_S390_CPU_TOPOLOGY, 
> > 0) < 0) {
> > +            error_setg(errp, "KVM: Error enabling 
> > KVM_CAP_S390_CPU_TOPOLOGY");
> > +            return;
> > +        }
> > +        set_bit(S390_FEAT_CONFIGURATION_TOPOLOGY, model->features);
> > +    }
> > +
> >       /* We emulate a zPCI bus and AEN, therefore we don't need HW 
> > support */
> >       set_bit(S390_FEAT_ZPCI, model->features);
> >       set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);

--
Pierre Morel
IBM Lab Boeblingen