Re: [PATCH 2/3] target/s390x: refractor AP functionalities

[Thomas Huth <thuth@redhat.com>]

In the subject: s/refractor/refactor/

Please also add a short patch description why you are doing these changes.

> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   target/s390x/kvm/kvm.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 3ac7ec9acf..bd62a7f606 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -250,7 +250,7 @@ static void kvm_s390_enable_cmma(void)
>       trace_kvm_enable_cmma(rc);
>   }
>   
> -static void kvm_s390_set_attr(uint64_t attr)
> +static void kvm_s390_set_crypto_attr(uint64_t attr)
>   {
>       struct kvm_device_attr attribute = {
>           .group = KVM_S390_VM_CRYPTO,
> @@ -275,7 +275,7 @@ static void kvm_s390_init_aes_kw(void)
>       }
>   
>       if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, attr)) {
> -            kvm_s390_set_attr(attr);
> +            kvm_s390_set_crypto_attr(attr);
>       }
>   }
>   
> @@ -289,7 +289,7 @@ static void kvm_s390_init_dea_kw(void)
>       }
>   
>       if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, attr)) {
> -            kvm_s390_set_attr(attr);
> +            kvm_s390_set_crypto_attr(attr);
>       }
>   }

I'd maybe move the renaming into a separate patch.

> @@ -2296,6 +2296,11 @@ static int configure_cpu_subfunc(const S390FeatBitmap features)
>       return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
>   }
>   
> +static bool ap_available(void)
> +{
> +    return kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, KVM_S390_VM_CRYPTO_ENABLE_APIE);

The line is already quite long ... maybe put it on two lines to avoid 
checkpatch.pl warnings.

> +}
> +
>   static int kvm_to_feat[][2] = {
>       { KVM_S390_VM_CPU_FEAT_ESOP, S390_FEAT_ESOP },
>       { KVM_S390_VM_CPU_FEAT_SIEF2, S390_FEAT_SIE_F2 },
> @@ -2475,8 +2480,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
>           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)) {
> +    if (ap_available()) {
>           set_bit(S390_FEAT_AP, model->features);
>       }
>   
> @@ -2502,7 +2506,7 @@ static void kvm_s390_configure_apie(bool interpret)
>                                   KVM_S390_VM_CRYPTO_DISABLE_APIE;
>   
>       if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_CRYPTO, attr)) {
> -        kvm_s390_set_attr(attr);
> +        kvm_s390_set_crypto_attr(attr);
>       }
>   }
>   
> @@ -2556,7 +2560,7 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
>           kvm_s390_enable_cmma();
>       }
>   
> -    if (test_bit(S390_FEAT_AP, model->features)) {
> +    if (ap_enabled(model->features)) {

You only introduce ap_enabled() in the next patch ... please move it here to 
avoid breaking bisection later.

  Thomas


>           kvm_s390_configure_apie(true);
>       }
>   }