Re: [qemu-s390x] [PATCH v2 5/5] s390x/cpumodel: Set up CPU model for AP device support

[Halil Pasic]

On 02/27/2018 06:52 PM, David Hildenbrand wrote:
>>      vfio_group = vfio_ap_get_group(vapdev, &local_err);
>>      if (!vfio_group) {
>>          goto out_err;
>> diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
>> index 11def14..35a6d04 100644
>> --- a/linux-headers/asm-s390/kvm.h
>> +++ b/linux-headers/asm-s390/kvm.h
>> @@ -130,6 +130,7 @@ struct kvm_s390_vm_cpu_machine {
>>  #define KVM_S390_VM_CPU_FEAT_PFMFI  11
>>  #define KVM_S390_VM_CPU_FEAT_SIGPIF 12
>>  #define KVM_S390_VM_CPU_FEAT_KSS    13
>> +#define KVM_S390_VM_CPU_FEAT_AP             14
>>  struct kvm_s390_vm_cpu_feat {
>>      __u64 feat[16];
>>  };
>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>> index a5619f2..65b91bd 100644
>> --- a/target/s390x/cpu_features.c
>> +++ b/target/s390x/cpu_features.c
>> @@ -36,8 +36,10 @@ static const S390FeatDef s390_features[] = {
>>      FEAT_INIT("srs", S390_FEAT_TYPE_STFL, 9, "Sense-running-status 
>> facility"),
>>      FEAT_INIT("csske", S390_FEAT_TYPE_STFL, 10, "Conditional-SSKE 
>> facility"),
>>      FEAT_INIT("ctop", S390_FEAT_TYPE_STFL, 11, "Configuration-topology 
>> facility"),
>> +    FEAT_INIT("qci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration 
>> facility"),
>>      FEAT_INIT("ipter", S390_FEAT_TYPE_STFL, 13, "IPTE-range facility"),
>>      FEAT_INIT("nonqks", S390_FEAT_TYPE_STFL, 14, "Nonquiescing key-setting 
>> facility"),
>> +    FEAT_INIT("apft", S390_FEAT_TYPE_STFL, 15, "Adjunct Processor 
>> Facilities Test facility"),
>>      FEAT_INIT("etf2", S390_FEAT_TYPE_STFL, 16, "Extended-translation 
>> facility 2"),
>>      FEAT_INIT("msa-base", S390_FEAT_TYPE_STFL, 17, "Message-security-assist 
>> facility (excluding subfunctions)"),
>>      FEAT_INIT("ldisp", S390_FEAT_TYPE_STFL, 18, "Long-displacement 
>> facility"),
>> @@ -125,6 +127,7 @@ static const S390FeatDef s390_features[] = {
>>  
>>      FEAT_INIT("dateh2", S390_FEAT_TYPE_MISC, 0, "DAT-enhancement facility 
>> 2"),
>>      FEAT_INIT("cmm", S390_FEAT_TYPE_MISC, 0, 
>> "Collaborative-memory-management facility"),
>> +    FEAT_INIT("ap", S390_FEAT_TYPE_MISC, 1, "AP facilities installed"),
> 
> How exactly is this feature communicated to the guest? How does KVM
> sense support for it?
> 

The ap_bus has a function for determining if the ap instructions are
installed. I think it's basically trial execution. 

> IOW: is this really a CPU model feature?
> 

I think it's best modeled with a CPU model feature. In the end
it's about having or not having ap instructions in the guest, and
making stable guest ABI is exactly the thing of cpu-models AFAIU.

>>  
>>      FEAT_INIT("plo-cl", S390_FEAT_TYPE_PLO, 0, "PLO Compare and load (32 
>> bit in general registers)"),
>>      FEAT_INIT("plo-clg", S390_FEAT_TYPE_PLO, 1, "PLO Compare and load (64 
>> bit in parameter list)"),
>> diff --git a/target/s390x/cpu_features_def.h 
>> b/target/s390x/cpu_features_def.h
>> index 7c5915c..8998b65 100644
>> --- a/target/s390x/cpu_features_def.h
>> +++ b/target/s390x/cpu_features_def.h
>> @@ -27,8 +27,10 @@ typedef enum {
>>      S390_FEAT_SENSE_RUNNING_STATUS,
>>      S390_FEAT_CONDITIONAL_SSKE,
>>      S390_FEAT_CONFIGURATION_TOPOLOGY,
>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>>      S390_FEAT_IPTE_RANGE,
>>      S390_FEAT_NONQ_KEY_SETTING,
>> +    S390_FEAT_AP_FACILITIES_TEST,
>>      S390_FEAT_EXTENDED_TRANSLATION_2,
>>      S390_FEAT_MSA,
>>      S390_FEAT_LONG_DISPLACEMENT,
>> @@ -118,6 +120,7 @@ typedef enum {
>>      /* Misc */
>>      S390_FEAT_DAT_ENH_2,
>>      S390_FEAT_CMM,
>> +    S390_FEAT_AP,
>>  
>>      /* PLO */
>>      S390_FEAT_PLO_CL,
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 1d5f0da..35f91ea 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -770,6 +770,8 @@ static void check_consistency(const S390CPUModel *model)
>>          { S390_FEAT_PRNO_TRNG_QRTCR, S390_FEAT_MSA_EXT_5 },
>>          { S390_FEAT_PRNO_TRNG, S390_FEAT_MSA_EXT_5 },
>>          { S390_FEAT_SIE_KSS, S390_FEAT_SIE_F2 },
>> +        { S390_FEAT_AP_QUERY_CONFIG_INFO, S390_FEAT_AP },
>> +        { S390_FEAT_AP_FACILITIES_TEST, S390_FEAT_AP },
>>      };
>>      int i;
>>  
>> @@ -900,6 +902,16 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
>>      cpu->model->cpu_id_format = max_model->cpu_id_format;
>>      cpu->model->cpu_ver = max_model->cpu_ver;
>>  
>> +    /*
>> +     * If the AP facilities are not installed on the guest, then it makes
>> +     * no sense to enable the QCI or APFT facilities because they are only
>> +     * needed by AP facilities.
>> +     */
>> +    if (!test_bit(S390_FEAT_AP, cpu->model->features)) {
>> +        clear_bit(S390_FEAT_AP_QUERY_CONFIG_INFO, cpu->model->features);
>> +        clear_bit(S390_FEAT_AP_FACILITIES_TEST, cpu->model->features);
>> +    }
> 
> Please don't silently disable things. Instead
>

I agree, this has to go (already commented on this).
 
> a) Add consistency checks (check_consistency())

The consistency checks are already in place. As already stated
before, one could make it produce an error.

> b) Mask the bits out in the KVM CPU model sensing part
>   (kvm_s390_get_host_cpu_model()) - which you already have :)
> 

Getting no ap but qci and apft indicated by KVM is unlikely to
happen, ever.

>> +
>>      check_consistency(cpu->model);
>>      check_compatibility(max_model, cpu->model, errp);
>>      if (*errp) {
>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>> index 0cdbc15..2d01b52 100644
>> --- a/target/s390x/gen-features.c
>> +++ b/target/s390x/gen-features.c
>> @@ -447,6 +447,9 @@ static uint16_t full_GEN12_GA1[] = {
>>      S390_FEAT_ADAPTER_INT_SUPPRESSION,
>>      S390_FEAT_EDAT_2,
>>      S390_FEAT_SIDE_EFFECT_ACCESS_ESOP2,
>> +    S390_FEAT_AP,
>> +    S390_FEAT_AP_QUERY_CONFIG_INFO,
>> +    S390_FEAT_AP_FACILITIES_TEST,
>>  };
> 
> Please keep the order as defined in target/s390x/cpu_features_def.h
> 
>>  
>>  static uint16_t full_GEN12_GA2[] = {
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index e13c890..ae20ed8 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -2105,6 +2105,7 @@ static int kvm_to_feat[][2] = {
>>      { KVM_S390_VM_CPU_FEAT_PFMFI, S390_FEAT_SIE_PFMFI},
>>      { KVM_S390_VM_CPU_FEAT_SIGPIF, S390_FEAT_SIE_SIGPIF},
>>      { KVM_S390_VM_CPU_FEAT_KSS, S390_FEAT_SIE_KSS},
>> +    { KVM_S390_VM_CPU_FEAT_AP, S390_FEAT_AP},
> 
> Nothing speaks against the STFL bits, want to learn more about the
> S390_FEAT_AP feature :)
> 
> 

Kernel series wise what you are looking for is 
'[PATCH v2 04/15] KVM: s390: CPU model support for AP virtualization'(MID: 
<address@hidden>) 
and
'[PATCH v2 08/15] KVM: s390: interface to enable AP execution mode' (MID: 
<address@hidden>)

Happy reviewing!

Regards,
Halil