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

[Thomas Huth]

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?

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

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

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