Message-ID: <e049d937-b4c4-8472-8995-3fe51fa52805@arm.com>
Date:   Fri, 26 Jan 2018 12:13:55 +0000
From:   Suzuki K Poulose <Suzuki.Poulose@....com>
To:     Dave Martin <Dave.Martin@....com>
Cc:     mark.rutland@....com, ckadabi@...eaurora.org,
        ard.biesheuvel@...aro.org, marc.zyngier@....com,
        catalin.marinas@....com, will.deacon@....com,
        linux-kernel@...r.kernel.org, jnair@...iumnetworks.com,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 04/16] arm64: capabilities: Prepare for fine grained
 capabilities

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

Powered by blists - more mailing lists