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

[Pierre Morel <pmorel@linux.ibm.com>]

On 2/6/23 12:57, Thomas Huth wrote:
> On 01/02/2023 14.20, Pierre Morel wrote:
>> The KVM capability KVM_CAP_S390_CPU_TOPOLOGY is used to
>> activate the S390_FEAT_CONFIGURATION_TOPOLOGY feature and
>> the topology facility in the host CPU model for the guest
>> in the case the topology 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    | 12 ++++++++++++
>>   3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> index 1028bf4476..c33378577b 100644
>> --- a/hw/s390x/cpu-topology.c
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -55,7 +55,7 @@ int s390_socket_nb(S390CPU *cpu)
>>    */
>>   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 065ec6d66c..aca2c5c96b 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -254,6 +254,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..808e35a7bd 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -2470,6 +2470,18 @@ void kvm_s390_get_host_cpu_model(S390CPUModel 
>> *model, Error **errp)
>>           set_bit(S390_FEAT_UNPACK, model->features);
>>       }
>> +    /*
>> +     * If we have kernel support for CPU Topology indicate the
>> +     * configuration-topology facility.
>> +     */
>> +    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);
>> +    }
> 
> Not sure, but for the other capabilities, the kvm_vm_enable_cap() is 
> rather done in kvm_arch_init() instead ... likely that it is properly 
> available in case you don't run with the "host" cpu model? So should the 
> kvm_vm_enable_cap(KVM_CAP_S390_CPU_TOPOLOGY) also be moved there (but of 
> course keep the set_bit() here in kvm_s390_get_host_cpu_model())?

I think you are right, I do this.
Thanks.

regards,
PIerre

> 
>   Thomas
> 

-- 
Pierre Morel
IBM Lab Boeblingen