Re: [qemu-s390x] [PATCH v9 2/6] s390x/cpumodel: Set up CPU model for AP device support

[David Hildenbrand]

On 27/09/2018 00:54, Tony Krowiak wrote:
> A new CPU model feature and two new CPU model facilities are
> introduced to support AP devices for a KVM guest.
> 
> CPU model features:
> 
> 1. The S390_FEAT_AP CPU model feature indicates whether AP
>    instructions are available to the guest. This feature will
>    be enabled only if the AP instructions are available on the
>    linux host as determined by the availability of the
>    KVM_S390_VM_CRYPTO_ENABLE_APIE VM attribute which is exposed
>    by KVM only if the AP instructions are available on the
>    host.
> 
>    This feature must be turned on from userspace to execute AP
>    instructions on the KVM guest. The QEMU command line to turn
>    this feature on looks something like this:
> 
>       qemu-system-s390x ... -cpu xxx,ap=on ...
> 
>    This feature will be supported for zEC12 and newer CPU models.
>    The feature will not be supported for older models because
>    there are few older systems on which to test and the older
>    crypto cards will be going out of service in the relatively
>    near future.
> 
> CPU model facilities:
> 
> 1. The S390_FEAT_AP_QUERY_CONFIG_INFO feature indicates whether the
>    AP Query Configuration Information (QCI) facility is available
>    to the guest as determined by whether the facility is available
>    on the host. This feature will be exposed by KVM only if the
>    QCI facility is installed on the host.
> 
> 2. The S390_FEAT_AP_FACILITY_TEST feature indicates whether the AP
>    Facility Test (APFT) facility is available to the guest as
>    determined by whether the facility is available on the host.
>    This feature will be exposed by KVM only if APFT is installed
>    on the host.
> 
> Signed-off-by: Tony Krowiak <address@hidden>
> ---
>  target/s390x/cpu_features.c     | 3 +++
>  target/s390x/cpu_features_def.h | 3 +++
>  target/s390x/cpu_models.c       | 2 ++
>  target/s390x/gen-features.c     | 3 +++
>  target/s390x/kvm.c              | 5 +++++
>  5 files changed, 16 insertions(+)
> 
> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
> index 172fb18df718..60cfeba48f4e 100644
> --- a/target/s390x/cpu_features.c
> +++ b/target/s390x/cpu_features.c
> @@ -39,8 +39,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("apqci", S390_FEAT_TYPE_STFL, 12, "Query AP Configuration 
> Information 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, "AP 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"),
> @@ -129,6 +131,7 @@ static const S390FeatDef s390_features[] = {
>  
>      FEAT_INIT_MISC("dateh2", "DAT-enhancement facility 2"),
>      FEAT_INIT_MISC("cmm", "Collaborative-memory-management facility"),
> +    FEAT_INIT_MISC("ap", "AP instructions installed"),
>  
>      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 ac2c947f30a8..5fc7e7bf0116 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,
> @@ -119,6 +121,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 265d25c937bb..7c253ff308c5 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -786,6 +786,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;
>  
> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
> index 384b61cd67b9..70015eaaf5df 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_QUERY_CONFIG_INFO,
> +    S390_FEAT_AP_FACILITIES_TEST,
> +    S390_FEAT_AP,
>  };
>  
>  static uint16_t full_GEN12_GA2[] = {
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 348e8cc5467a..5277acd79a2c 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -2291,6 +2291,11 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, 
> Error **errp)
>          error_setg(errp, "KVM: host CPU model could not be identified");
>          return;
>      }
> +    /* for now, we can only provide the AP feature with HW support */
> +    if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO,
> +        KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
> +        set_bit(S390_FEAT_AP, model->features);
> +    }
>      /* strip of features that are not part of the maximum model */
>      bitmap_and(model->features, model->features, model->def->full_feat,
>                 S390_FEAT_MAX);
> 

I'd move this hunk to the next (KVM specific) patch, so
indication+enablement happens at one time. (with this patch only you
would see and could enable ap=on, but the guest would not see AP)

Apart from that, looks good to me

Reviewed-by: David Hildenbrand <address@hidden>

-- 

Thanks,

David / dhildenb