Re: [PATCH 04/16] arm64: capabilities: Prepare for fine grained capabilities

[Suzuki K Poulose]

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@xxxxxxx>
> > > > ---
> > > >   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