Messages in this thread | | | Date | Wed, 13 Jul 2022 10:32:21 +0200 | Subject | Re: [PATCH 1/3] s390/cpufeature: rework to allow more than only hwcap bits | From | Steffen Eiden <> |
| |
On 7/12/22 21:25, Heiko Carstens wrote: > On Tue, Jul 12, 2022 at 12:52:18PM +0200, Steffen Eiden 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> > ... >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright IBM Corp. 2022 >> + * Author(s): Steffen Eiden <seiden@linux.ibm.com> >> + * Heiko Carstens <hca@linux.ibm.com> > > Please don't add my name + email address in source code. I just > recently removed that everywhere since email addresses may change, and > git history is more than enough for me. It's up to you if you want to > keep your name + email address here.
OK, makes sense.
> >> +static struct s390_cpu_feature s390_cpu_features[MAX_CPU_FEATURES] = { >> + [S390_CPU_FEATURE_ESAN3] = {.type = TYPE_HWCAP, .num = HWCAP_NR_ESAN3}, >> + [S390_CPU_FEATURE_ZARCH] = {.type = TYPE_HWCAP, .num = HWCAP_NR_ZARCH}, >> + [S390_CPU_FEATURE_STFLE] = {.type = TYPE_HWCAP, .num = HWCAP_NR_STFLE}, >> + [S390_CPU_FEATURE_MSA] = {.type = TYPE_HWCAP, .num = HWCAP_NR_MSA}, >> + [S390_CPU_FEATURE_LDISP] = {.type = TYPE_HWCAP, .num = HWCAP_NR_LDISP}, >> + [S390_CPU_FEATURE_EIMM] = {.type = TYPE_HWCAP, .num = HWCAP_NR_EIMM}, >> + [S390_CPU_FEATURE_DFP] = {.type = TYPE_HWCAP, .num = HWCAP_NR_DFP}, >> + [S390_CPU_FEATURE_HPAGE] = {.type = TYPE_HWCAP, .num = HWCAP_NR_HPAGE}, >> + [S390_CPU_FEATURE_ETF3EH] = {.type = TYPE_HWCAP, .num = HWCAP_NR_ETF3EH}, >> + [S390_CPU_FEATURE_HIGH_GPRS] = {.type = TYPE_HWCAP, .num = HWCAP_NR_HIGH_GPRS}, >> + [S390_CPU_FEATURE_TE] = {.type = TYPE_HWCAP, .num = HWCAP_NR_TE}, >> + [S390_CPU_FEATURE_VXRS] = {.type = TYPE_HWCAP, .num = HWCAP_NR_VXRS}, >> + [S390_CPU_FEATURE_VXRS_BCD] = {.type = TYPE_HWCAP, .num = HWCAP_NR_VXRS_BCD}, >> + [S390_CPU_FEATURE_VXRS_EXT] = {.type = TYPE_HWCAP, .num = HWCAP_NR_VXRS_EXT}, >> + [S390_CPU_FEATURE_GS] = {.type = TYPE_HWCAP, .num = HWCAP_NR_GS}, >> + [S390_CPU_FEATURE_VXRS_EXT2] = {.type = TYPE_HWCAP, .num = HWCAP_NR_VXRS_EXT2}, >> + [S390_CPU_FEATURE_VXRS_PDE] = {.type = TYPE_HWCAP, .num = HWCAP_NR_VXRS_PDE}, >> + [S390_CPU_FEATURE_SORT] = {.type = TYPE_HWCAP, .num = HWCAP_NR_SORT}, >> + [S390_CPU_FEATURE_DFLT] = {.type = TYPE_HWCAP, .num = HWCAP_NR_DFLT}, >> + [S390_CPU_FEATURE_VXRS_PDE2] = {.type = TYPE_HWCAP, .num = HWCAP_NR_VXRS_PDE2}, >> + [S390_CPU_FEATURE_NNPA] = {.type = TYPE_HWCAP, .num = HWCAP_NR_NNPA}, >> + [S390_CPU_FEATURE_PCI_MIO] = {.type = TYPE_HWCAP, .num = HWCAP_NR_PCI_MIO}, >> + [S390_CPU_FEATURE_SIE] = {.type = TYPE_HWCAP, .num = HWCAP_NR_SIE}, >> +}; > > I only realized now that you added all HWCAP bits here. It was > intentional that I added only the two bits which are currently used > for several reasons: > > - Keep the array as small as possible. > - No need to keep this array in sync with HWCAPs, if new ones are added. > - There is a for loop in print_cpu_modalias() which iterates over all > MAX_CPU_FEATURES entries; this should be as fast as possible. Adding > extra entries burns cycles for no added value. The loop in print_cpu_modalias() was the reason why I added all current HWCAPs. The current implementation runs through all HWCAPs using cpu_have_feature() and I feared that reducing to just MSA and VXRS has effects in the reporting of CPU-features to userspace.
I double checked the output of 'grep features /proc/cpuinfo' and it stays the same, for 5.19-rc6, 5.19-rc6+this series, 5.19-rc6+this series with just the two S390_CPU_FEATUREs. I might have misunderstood what happens in that loop in print_cpu_modalias().
Now that I think again over this piece of code my additions do not make sense at all for me.
I will reduce that array again to the two explicitly needed entries.
> > Any future user which requires a not yet listed feature, can simply > add it when needed. > >> +int cpu_have_feature(unsigned int num) >> +{ >> + struct s390_cpu_feature *feature; >> + >> + feature = &s390_cpu_features[num]; >> + switch (feature->type) { >> + case TYPE_HWCAP: >> + return !!(elf_hwcap & (1UL << feature->num)); > > Before somebody else mentions it, I could have done better. Nowadays > this should be: > > return !!(elf_hwcap & BIT(feature->num)); I'll change it.
| |