Re: [PATCH 4/4] KVM: s390: Minor refactor of base/ext facility lists

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Fri,  3 Nov 2023 18:30:08 +0100
Nina Schoetterl-Glausch <nsg@linux.ibm.com> wrote:

> Directly use the size of the arrays instead of going through the
> indirection of kvm_s390_fac_size().
> Don't use magic number for the number of entries in the non hypervisor
> managed facility bit mask list.
> Make the constraint of that number on kvm_s390_fac_base obvious.
> Get rid of implicit double anding of stfle_fac_list.
> 
> Signed-off-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
> 
> 
> I found it confusing before and think it's nicer this way but
> it might be needless churn.

some things are probably overkill

> 
> 
>  arch/s390/kvm/kvm-s390.c | 44 +++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index b3f17e014cab..e00ab2f38c89 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -217,33 +217,25 @@ static int async_destroy = 1;
>  module_param(async_destroy, int, 0444);
>  MODULE_PARM_DESC(async_destroy, "Asynchronous destroy for protected guests");
>  
> -/*
> - * For now we handle at most 16 double words as this is what the s390 base
> - * kernel handles and stores in the prefix page. If we ever need to go beyond
> - * this, this requires changes to code, but the external uapi can stay.
> - */
> -#define SIZE_INTERNAL 16
> -
> +#define HMFAI_DWORDS 16
>  /*
>   * Base feature mask that defines default mask for facilities. Consists of the
>   * defines in FACILITIES_KVM and the non-hypervisor managed bits.
>   */
> -static unsigned long kvm_s390_fac_base[SIZE_INTERNAL] = { FACILITIES_KVM };
> +static unsigned long kvm_s390_fac_base[HMFAI_DWORDS] = { FACILITIES_KVM };
> +static_assert(ARRAY_SIZE(((long[]){ FACILITIES_KVM })) <= HMFAI_DWORDS);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_base) <= S390_ARCH_FAC_MASK_SIZE_U64);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_base) <= S390_ARCH_FAC_LIST_SIZE_U64);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_base) <= ARRAY_SIZE(stfle_fac_list));
> +
>  /*
>   * Extended feature mask. Consists of the defines in FACILITIES_KVM_CPUMODEL
>   * and defines the facilities that can be enabled via a cpu model.
>   */
> -static unsigned long kvm_s390_fac_ext[SIZE_INTERNAL] = { FACILITIES_KVM_CPUMODEL };
> -
> -static unsigned long kvm_s390_fac_size(void)
> -{
> -	BUILD_BUG_ON(SIZE_INTERNAL > S390_ARCH_FAC_MASK_SIZE_U64);
> -	BUILD_BUG_ON(SIZE_INTERNAL > S390_ARCH_FAC_LIST_SIZE_U64);
> -	BUILD_BUG_ON(SIZE_INTERNAL * sizeof(unsigned long) >
> -		sizeof(stfle_fac_list));
> -
> -	return SIZE_INTERNAL;
> -}
> +static const unsigned long kvm_s390_fac_ext[] = { FACILITIES_KVM_CPUMODEL };

this was sized to [SIZE_INTERNAL], now it doesn't have a fixed size. is
this intentional?

> +static_assert(ARRAY_SIZE(kvm_s390_fac_ext) <= S390_ARCH_FAC_MASK_SIZE_U64);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_ext) <= S390_ARCH_FAC_LIST_SIZE_U64);
> +static_assert(ARRAY_SIZE(kvm_s390_fac_ext) <= ARRAY_SIZE(stfle_fac_list));
>  
>  /* available cpu features supported by kvm */
>  static DECLARE_BITMAP(kvm_s390_available_cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
> @@ -3341,13 +3333,16 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	kvm->arch.sie_page2->kvm = kvm;
>  	kvm->arch.model.fac_list = kvm->arch.sie_page2->fac_list;
>  
> -	for (i = 0; i < kvm_s390_fac_size(); i++) {
> +	for (i = 0; i < ARRAY_SIZE(kvm_s390_fac_base); i++) {
>  		kvm->arch.model.fac_mask[i] = stfle_fac_list[i] &
> -					      (kvm_s390_fac_base[i] |
> -					       kvm_s390_fac_ext[i]);
> +					      kvm_s390_fac_base[i];
>  		kvm->arch.model.fac_list[i] = stfle_fac_list[i] &
>  					      kvm_s390_fac_base[i];
>  	}
> +	for (i = 0; i < ARRAY_SIZE(kvm_s390_fac_ext); i++) {
> +		kvm->arch.model.fac_mask[i] |= stfle_fac_list[i] &
> +					       kvm_s390_fac_ext[i];
> +	}

I like it better when it's all in one place, instead of having two loops

>  	kvm->arch.model.subfuncs = kvm_s390_available_subfunc;
>  
>  	/* we are always in czam mode - even on pre z14 machines */
> @@ -5859,9 +5854,8 @@ static int __init kvm_s390_init(void)
>  		return -EINVAL;
>  	}
>  
> -	for (i = 0; i < 16; i++)
> -		kvm_s390_fac_base[i] |=
> -			stfle_fac_list[i] & nonhyp_mask(i);
> +	for (i = 0; i < HMFAI_DWORDS; i++)
> +		kvm_s390_fac_base[i] |= nonhyp_mask(i);

where did the stfle_fac_list[i] go?

>  
>  	r = __kvm_s390_init();
>  	if (r)