Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 18:46:19 +0200
From:   Claudio Imbrenda <imbrenda@linux.ibm.com>
To:     Steffen Eiden <seiden@linux.ibm.com>
Cc:     Heiko Carstens <hca@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org, nrb@linux.ibm.com
Subject: Re: [PATCH 1/3] s390/cpufeature: rework to allow more than only
 hwcap bits
Message-ID: <20220712184619.45edd420@p-imbrenda>
In-Reply-To: <20220712105220.325010-2-seiden@linux.ibm.com>
References: <20220712105220.325010-1-seiden@linux.ibm.com>
        <20220712105220.325010-2-seiden@linux.ibm.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 12 Jul 2022 12:52:18 +0200
Steffen Eiden <seiden@linux.ibm.com> wrote:

> Rework cpufeature implementation to allow for various cpu feature
> indications, which is not only limited to hwcap bits. This is achieved
> by adding a sequential list of cpu feature numbers, where each of them
> is mapped to an entry which indicates what this number is about.
> 
> Each entry contains a type member, which indicates what feature
> name space to look into (e.g. hwcap, or cpu facility). If wanted this
> allows also to automatically load modules only in e.g. z/VM
> configurations.
> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>

[...]

> +
> +/*
> + * cpu_have_feature - Test CPU features on module initialization
> + */
> +int cpu_have_feature(unsigned int num)
> +{
> +	struct s390_cpu_feature *feature;
> +
> +	feature = &s390_cpu_features[num];

I would put some check to make sure you are going past the end of the
array. 

Maybe something like

if (num >= MAX_CPU_FEATURES) {
	WARN(1, "Invalid feature %d", num);
	return 0;
}

> +	switch (feature->type) {
> +	case TYPE_HWCAP:
> +		return !!(elf_hwcap & (1UL << feature->num));
> +	default:
> +		WARN_ON_ONCE(1);
> +		return 0;
> +	}
> +}
> +EXPORT_SYMBOL(cpu_have_feature);
> diff --git a/arch/s390/kernel/processor.c b/arch/s390/kernel/processor.c
> index aa0e0e7fc773..a194611ba88c 100644
> --- a/arch/s390/kernel/processor.c
> +++ b/arch/s390/kernel/processor.c
> @@ -8,7 +8,6 @@
>  #define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
>  
>  #include <linux/stop_machine.h>
> -#include <linux/cpufeature.h>
>  #include <linux/bitops.h>
>  #include <linux/kernel.h>
>  #include <linux/random.h>
> @@ -96,15 +95,6 @@ void cpu_init(void)
>  	enter_lazy_tlb(&init_mm, current);
>  }
>  
> -/*
> - * cpu_have_feature - Test CPU features on module initialization
> - */
> -int cpu_have_feature(unsigned int num)
> -{
> -	return elf_hwcap & (1UL << num);
> -}
> -EXPORT_SYMBOL(cpu_have_feature);
> -
>  static void show_facilities(struct seq_file *m)
>  {
>  	unsigned int bit;
> diff --git a/drivers/char/hw_random/s390-trng.c b/drivers/char/hw_random/s390-trng.c
> index 2beaa35c0d74..12fbac0ed8ca 100644
> --- a/drivers/char/hw_random/s390-trng.c
> +++ b/drivers/char/hw_random/s390-trng.c
> @@ -261,5 +261,5 @@ static void __exit trng_exit(void)
>  	trng_debug_exit();
>  }
>  
> -module_cpu_feature_match(MSA, trng_init);
> +module_cpu_feature_match(S390_CPU_FEATURE_MSA, trng_init);
>  module_exit(trng_exit);
> diff --git a/drivers/s390/crypto/pkey_api.c b/drivers/s390/crypto/pkey_api.c
> index 7329caa7d467..5a05d1cdfec2 100644
> --- a/drivers/s390/crypto/pkey_api.c
> +++ b/drivers/s390/crypto/pkey_api.c
> @@ -2115,5 +2115,5 @@ static void __exit pkey_exit(void)
>  	pkey_debug_exit();
>  }
>  
> -module_cpu_feature_match(MSA, pkey_init);
> +module_cpu_feature_match(S390_CPU_FEATURE_MSA, pkey_init);
>  module_exit(pkey_exit);