Messages in this thread | | | Subject | Re: [PATCH 04/16] arm64: capabilities: Prepare for fine grained capabilities | From | Suzuki K Poulose <> | Date | Fri, 26 Jan 2018 12:13:55 +0000 |
| |
On 26/01/18 10:00, Dave Martin wrote: > On Thu, Jan 25, 2018 at 05:56:02PM +0000, Suzuki K Poulose wrote: >> On 25/01/18 17:33, Dave Martin wrote: >>> On Tue, Jan 23, 2018 at 12:27:57PM +0000, Suzuki K Poulose wrote: >>>> We use arm64_cpu_capabilities to represent CPU ELF HWCAPs exposed >>>> to the userspace and the CPU hwcaps used by the kernel, which >>>> include cpu features and CPU errata work arounds. >>>> >>>> At the moment we have the following restricions: >>>> >>>> a) CPU feature hwcaps (arm64_features) and ELF HWCAPs (arm64_elf_hwcap) >>>> - Detected mostly on system wide CPU feature register. But >>>> there are some which really uses a local CPU's value to >>>> decide the availability (e.g, availability of hardware >>>> prefetch). So, we run the check only once, after all the >>>> boot-time active CPUs are turned on. >>> >>> [ARM64_HAS_NO_HW_PREFETCH is kinda broken, but we also get away with it >>> presumably because the only systems to which it applies are homogeneous, >>> and anyway it's only an optimisation IIUC. >>> >>> This could be a separate category, but as a one-off that may be a bit >>> pointless. >> I understand and was planning to fix this back when it was introduced. >> But then it was pointless at that time, given that it was always >> guaranteed to be a homogeneous system. We do something about it in >> Patch 9. > > This was just on observation than something that needs to be fixed, > but it it's been cleaned up then so much the better :) > > I'll take a look. > >>> .def_scope == SCOPE_SYSTEM appears anomalous there, but it's also >>> unused in that case.] >>> >>>> - Any late CPU which doesn't posses all the established features >>>> is killed. >>> >>> Does "established feature" above ... >>> >>>> - Any late CPU which possess a feature *not* already available >>>> is allowed to boot. >>> >>> mean the same as "feature already available" here? >> >> Yes, its the same. I should have been more consistent. >> >>> >>>> >>>> b) CPU Errata work arounds (arm64_errata) >>>> - Detected mostly based on a local CPU's feature register. >>>> The checks are run on each boot time activated CPUs. >>>> - Any late CPU which doesn't have any of the established errata >>>> work around capabilities is ignored and is allowed to boot. >>>> - Any late CPU which has an errata work around not already available >>>> is killed. >>>> >>>> However there are some exceptions to the cases above. >>>> >>>> 1) KPTI is a feature that we need to enable when at least one CPU needs it. >>>> And any late CPU that has this "feature" should be killed. >>> >>> Should that be "If KPTI is not enabled during system boot, then any late >>> CPU that has this "feature" should be killed." >> >> Yes. >> >>> >>>> 2) Hardware DBM feature is a non-conflicting capability which could be >>>> enabled on CPUs which has it without any issues, even if the CPU is >>> >>> have >>> >> >>>> brought up late. >>>> >>>> So this calls for a lot more fine grained behavior for each capability. >>>> And if we define all the attributes to control their behavior properly, >>>> we may be able to use a single table for the CPU hwcaps (not the >>>> ELF HWCAPs, which cover errata and features). This is a prepartory step >>>> to get there. We define type for a capability, which for now encodes the >>>> scope of the check. i.e, whether it should be checked system wide or on >>>> each local CPU. We define two types : >>>> >>>> 1) ARM64_CPUCAP_BOOT_SYSTEM_FEATURE - Implies (a) as described above. >>>> 1) ARM64_CPUCAP_STRICT_CPU_LOCAL_ERRATUM - Implies (b) as described above. >>> >>> 2) > > Meaning you've got 1) twice above (in case you didn't spot it). >
Yes, you're right.
>>> >>>> As such there is no change in how the capabilities are treated. >>> >>> OK, I think I finally have my head around this, more or less. >>> >>> Mechanism (operations on architectural feature regs) and policy (kernel >>> runtime configuration) seem to be rather mixed together. This works >>> fairly naturally for things like deriving the sanitised feature regs >>> seen by userspace and determining the ELF hwcaps; but not so naturally >>> for errata workarounds and other anomalous things like >>> ARM64_HAS_NO_HW_PREFETCH. >> >> Right. We are stuck with "cpu_hwcaps" for both erratum and features, >> based on which we make some decisions to change the kernel behavior, >> as it is tied to alternative patching. >> >>> >>> I'm not sure that there is a better approach though -- anyway, that >>> would be out of scope for this series. >>> >>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>>> --- >>>> arch/arm64/include/asm/cpufeature.h | 24 +++++++++++++++++------ >>>> arch/arm64/kernel/cpu_errata.c | 8 ++++---- >>>> arch/arm64/kernel/cpufeature.c | 38 ++++++++++++++++++------------------- >>>> 3 files changed, 41 insertions(+), 29 deletions(-) >>>> >>>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h >>>> index a23c0d4f27e9..4fd5de8ef33e 100644 >>>> --- a/arch/arm64/include/asm/cpufeature.h >>>> +++ b/arch/arm64/include/asm/cpufeature.h >>>> @@ -86,16 +86,23 @@ struct arm64_ftr_reg { >>>> extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0; >>>> -/* scope of capability check */ >>>> -enum { >>>> - SCOPE_SYSTEM, >>>> - SCOPE_LOCAL_CPU, >>>> -}; >>>> + >>>> +/* Decide how the capability is detected. On a local CPU vs System wide */ >>>> +#define ARM64_CPUCAP_SCOPE_MASK 0x3 >>>> +#define ARM64_CPUCAP_SCOPE_LOCAL_CPU BIT(0) >>>> +#define ARM64_CPUCAP_SCOPE_SYSTEM BIT(1) >>>> +#define SCOPE_SYSTEM ARM64_CPUCAP_SCOPE_SYSTEM >>>> +#define SCOPE_LOCAL_CPU ARM64_CPUCAP_SCOPE_LOCAL_CPU >>> >>> Are these really orthogonal? What would be meant by (LOCAL_CPU | SYSTEM)? >> >> It is an unsupported configuration. > > Surely meaningless, not just unsupported?
Yep.
> >>> >>> Otherwise, perhaps they should be 0 and 1, not BIT(0), BIT(1). >>> >> >> It is a bit tricky. I chose separate bits to allow filter an entry in a table >> of capabilities based on the bits. e.g, we want to >> >> 1) Process only the local scope (e.g detecting CPU local capabilities, where >> we are not yet ready to run the system wide checks) >> >> 2) Process all the entries including local/system. (e.g, verifying all the >> capabilities for a late CPU). > > OK, so LOCAL_CPU and SYSTEM are mutually exclusive for the cap type, but > by making them separate bits in a bitmask then (LOCAL_CPU | SYSTEM) as a > match value will match on either. > >> Things get further complicated by the addition of "EARLY", where we want to >> filter entries based on "EARLY" bit. So, we need to pass on a mask of bits >> to the helpers, which are not just the binary scope. See Patch 7 for more >> info. >> >>>> + >>>> +/* CPU errata detected at boot time based on feature of one or more CPUs */ >>>> +#define ARM64_CPUCAP_STRICT_CPU_LOCAL_ERRATUM (ARM64_CPUCAP_SCOPE_LOCAL_CPU) >>> >>>> +/* CPU feature detected at boot time based on system-wide value of a feature */ >>> >>> I'm still not overly keen on these names, but I do at least see where >>> they come from now.
I will try to make this patch a bit more simpler, by not doing a forward reference of the conflict "behavior" we introduce in the next patch and, keeping it just to changing the field name.
Thanks a lot for the feedback.
Cheers Suzuki
| |