Message-ID: <7221b67c-6581-93bd-ebb3-7e48736ecb02@arm.com>
Date:   Wed, 7 Feb 2018 11:31:29 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Dave Martin <Dave.Martin@....com>,
        Suzuki K Poulose <suzuki.poulose@....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 v2 05/20] arm64: capabilities: Add flags to handle the
 conflicts on late CPU

On 07/02/18 10:38, Dave Martin wrote:
> On Wed, Jan 31, 2018 at 06:27:52PM +0000, Suzuki K Poulose wrote:
>> When a CPU is brought up, it is checked against the caps that are
>> known to be enabled on the system (via verify_local_cpu_capabilities()).
>> Based on the state of the capability on the CPU vs. that of System we
>> could have the following combinations of conflict.
>>
>> 	x-----------------------------x
>> 	| Type  | System   | Late CPU |
>> 	|-----------------------------|
>> 	|  a    |   y      |    n     |
>> 	|-----------------------------|
>> 	|  b    |   n      |    y     |
>> 	x-----------------------------x
>>
>> Case (a) is not permitted for caps which are system features, which the
>> system expects all the CPUs to have (e.g VHE). While (a) is ignored for
>> all errata work arounds. However, there could be exceptions to the plain
>> filtering approach. e.g, KPTI is an optional feature for a late CPU as
>> long as the system already enables it.
>>
>> Case (b) is not permitted for errata work arounds which requires some work
>> around, which cannot be delayed. And we ignore (b) for features. Here, yet
> 
> Nit, maybe:
> 
> "Case (b) is not permitted for any errata workaround that cannot be
> activated if the kernel has finished booting and has not already enabled
> it."

Nit^2: I think it would suffice to say "...that cannot be activated 
after the kernel has finished booting." - since we don't really have the 
concept of *de*activating workarounds, it is already implicit in that 
statement that the one in question wasn't activated *before* the kernel 
finished booting.

Robin.

>> again, KPTI is an exception, where if a late CPU needs KPTI we are too late
>> to enable it (because we change the allocation of ASIDs etc).
>>
>> Add two different flags to indicate how the conflict should be handled.
>>
>>   ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU - CPUs may have the capability
>>   ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU - CPUs may not have the cappability.
>>
>> Now that we have the flags to describe the behavior of the errata and
>> the features, as we treat them, define types for ERRATUM and FEATURE.
>>
>> Cc: Dave Martin <dave.martin@....com>
>> Cc: Will Deacon <will.deacon@....com>
>> Cc: Mark Rutland <mark.rutland@....com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
> 
> All my suggestions in this patch are rewordings of comments, so they're
> not vital, and anyway you may disagree that they make the meaning
> clearer.  So I don't think they're essential.
> 
> For the actual code
> 
> Reviewed-by: Dave Martin <Dave.Martin@....com>
> 
>> ---
>>   arch/arm64/include/asm/cpufeature.h | 61 ++++++++++++++++++++++++++++++++++++-
>>   arch/arm64/kernel/cpu_errata.c      |  8 ++---
>>   arch/arm64/kernel/cpufeature.c      | 30 +++++++++---------
>>   3 files changed, 79 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index 05da54f1b4c7..7460b1f7e611 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -142,7 +142,8 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>>    *    capabilities and if there is a conflict, the kernel takes an action, based
>>    *    on the severity (e.g, a CPU could be prevented from booting or cause a
>>    *    kernel panic). The CPU is allowed to "affect" the state of the capability,
>> - *    if it has not been finalised already.
>> + *    if it has not been finalised already. See section 5 for more details on
>> + *    conflicts.
>>    *
>>    * 4) Action: As mentioned in (2), the kernel can take an action for each detected
>>    *    capability, on all CPUs on the system. This is always initiated only after
>> @@ -155,6 +156,32 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>>    *	b) Any late CPU, brought up after (1), the action is triggered via:
>>    *		check_local_cpu_capabilities() -> verify_local_cpu_capabilities()
>>    *
>> + * 5) Conflicts: Based on the state of the capability on a late CPU vs. the system
>> + *    state, we could have the following combinations :
>> + *
>> + *		x-----------------------------x
>> + *		| Type  | System   | Late CPU |
>> + *		|-----------------------------|
>> + *		|  a    |   y      |    n     |
>> + *		|-----------------------------|
>> + *		|  b    |   n      |    y     |
>> + *		x-----------------------------x
> 
> Nit: I find this a bit hard to follow.  Maybe reordering things a bit
> would help but putting the rule (the precise bit) first, and the
> rationale (necessarily more vague) afterward:
> 
> "Two capability type flag bits are defined to indicate whether each kind
> of conflict can be tolerated:
> 
> 		ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU - Case (a) is allowed
> 		ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU - Case (b) is allowed."
> 
>> + *
>> + *     Case (a) is not permitted for capabilities which are usually system
>> + *     features, which the system expects all CPUs to have. While (a) is ignored
>> + *     for capabilities which represents an erratum work around.
> 
> Similarly "Case (a) is not permitted for a capability that the system
> requires all CPUs to have in order for the capability to be enabled.
> This is typical for capabilities that represent enhanced functionality."
> 
>> + *
>> + *     Case (b) is not permitted for erratum capabilities, which might require
>> + *     some work arounds which cannot be applied really late. Meanwhile, most
>> + *     of the features could safely ignore (b), as the system doesn't use it
>> + *     anyway.
> 
> and
> 
> "Case (b) is not permitted for a capability that the system must enable
> during boot if any CPU in the system requires it in order to run safely.
> This is typical for erratum workarounds that cannot be enabled after the
> corresponding capability is finalised.
> 
> In some non-typical cases, either both (a) and (b), or neither, should
> be permitted.  This can be described by including neither or both flags
> in the capability's type field."
> 
> [...]
> 
>>   
>>   
>> @@ -165,6 +192,26 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>>   #define SCOPE_SYSTEM				ARM64_CPUCAP_SCOPE_SYSTEM
>>   #define SCOPE_LOCAL_CPU				ARM64_CPUCAP_SCOPE_LOCAL_CPU
>>   
>> +/* Is it permitted for a late CPU to have this capability when system doesn't already have */
> 
> "doesn't already have" -> "hasn't already enabled it"?
> 
>> +#define ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU	((u16)BIT(4))
>> +/* Is it safe for a late CPU to miss this capability when system has it */
>> +#define ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU	((u16)BIT(5))
>> +
>> +/*
>> + * CPU errata detected at boot time based on feature of one or more CPUs.
>> + * It is not safe for a late CPU to have this feature when the system doesn't
> 
> Can we around using the word "feature" to describe errata here?
> 
> Maybe "CPU errata workarounds that need to be enabled at boot time if
> one or more CPUs in the system requires the workaround.  When one of
> these workaround capabilities has been enabled, it is safe to allow any
> CPU to boot that does not require the workaround."
> 
>> + * have it. But it is safe to miss the feature if the system has it.
>> + */
>> +#define ARM64_CPUCAP_LOCAL_CPU_ERRATUM		\
>> +	(ARM64_CPUCAP_SCOPE_LOCAL_CPU | ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU)
>> +/*
>> + * CPU feature detected at boot time based on system-wide value of a feature.
>> + * It is safe for a late CPU to have this feature even though the system doesn't
>> + * have it already. But the CPU must have this feature if the system does.
> 
> "hasn't enabled it, although the feature will not be used by Linux
> in this case.  If the system has enabled this feature already then every
> late CPU must have it."
> 
>> + */
>> +#define ARM64_CPUCAP_SYSTEM_FEATURE	\
>> +	(ARM64_CPUCAP_SCOPE_SYSTEM | ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU)
>> +
>>   struct arm64_cpu_capabilities {
>>   	const char *desc;
>>   	u16 capability;
>> @@ -198,6 +245,18 @@ static inline int cpucap_default_scope(const struct arm64_cpu_capabilities *cap)
>>   	return cap->type & ARM64_CPUCAP_SCOPE_MASK;
>>   }
>>   
>> +static inline bool
>> +cpucap_late_cpu_optional(const struct arm64_cpu_capabilities *cap)
>> +{
>> +	return !!(cap->type & ARM64_CPUCAP_OPTIONAL_FOR_LATE_CPU);
>> +}
>> +
>> +static inline bool
>> +cpucap_late_cpu_permitted(const struct arm64_cpu_capabilities *cap)
>> +{
>> +	return !!(cap->type & ARM64_CPUCAP_PERMITTED_FOR_LATE_CPU);
>> +}
>> +
>>   extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
>>   extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS];
>>   extern struct static_key_false arm64_const_caps_ready;
>> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
>> index 328c5a031e45..22ec3960a0c5 100644
>> --- a/arch/arm64/kernel/cpu_errata.c
>> +++ b/arch/arm64/kernel/cpu_errata.c
>> @@ -175,14 +175,14 @@ static void qcom_enable_link_stack_sanitization(
>>   #endif	/* CONFIG_HARDEN_BRANCH_PREDICTOR */
>>   
>>   #define MIDR_RANGE(model, min, max) \
>> -	.type = ARM64_CPUCAP_SCOPE_LOCAL_CPU, \
>> +	.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM, \
>>   	.matches = is_affected_midr_range, \
>>   	.midr_model = model, \
>>   	.midr_range_min = min, \
>>   	.midr_range_max = max
>>   
>>   #define MIDR_ALL_VERSIONS(model) \
>> -	.type = ARM64_CPUCAP_SCOPE_LOCAL_CPU, \
>> +	.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM, \
>>   	.matches = is_affected_midr_range, \
>>   	.midr_model = model, \
>>   	.midr_range_min = 0, \
>> @@ -286,7 +286,7 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
>>   		.desc = "Mismatched cache line size",
>>   		.capability = ARM64_MISMATCHED_CACHE_LINE_SIZE,
>>   		.matches = has_mismatched_cache_line_size,
>> -		.type = ARM64_CPUCAP_SCOPE_LOCAL_CPU,
>> +		.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,
>>   		.cpu_enable = cpu_enable_trap_ctr_access,
>>   	},
>>   #ifdef CONFIG_QCOM_FALKOR_ERRATUM_1003
>> @@ -300,7 +300,7 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
>>   	{
>>   		.desc = "Qualcomm Technologies Kryo erratum 1003",
>>   		.capability = ARM64_WORKAROUND_QCOM_FALKOR_E1003,
>> -		.type = ARM64_CPUCAP_SCOPE_LOCAL_CPU,
>> +		.type = ARM64_CPUCAP_LOCAL_CPU_ERRATUM,
>>   		.midr_model = MIDR_QCOM_KRYO,
>>   		.matches = is_kryo_midr,
>>   	},
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 8d22f0ef0927..1b29b3f0a1bc 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -921,7 +921,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   	{
>>   		.desc = "GIC system register CPU interface",
>>   		.capability = ARM64_HAS_SYSREG_GIC_CPUIF,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = has_useable_gicv3_cpuif,
>>   		.sys_reg = SYS_ID_AA64PFR0_EL1,
>>   		.field_pos = ID_AA64PFR0_GIC_SHIFT,
>> @@ -932,7 +932,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   	{
>>   		.desc = "Privileged Access Never",
>>   		.capability = ARM64_HAS_PAN,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = has_cpuid_feature,
>>   		.sys_reg = SYS_ID_AA64MMFR1_EL1,
>>   		.field_pos = ID_AA64MMFR1_PAN_SHIFT,
>> @@ -945,7 +945,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   	{
>>   		.desc = "LSE atomic instructions",
>>   		.capability = ARM64_HAS_LSE_ATOMICS,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = has_cpuid_feature,
>>   		.sys_reg = SYS_ID_AA64ISAR0_EL1,
>>   		.field_pos = ID_AA64ISAR0_ATOMICS_SHIFT,
>> @@ -956,14 +956,14 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   	{
>>   		.desc = "Software prefetching using PRFM",
>>   		.capability = ARM64_HAS_NO_HW_PREFETCH,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = has_no_hw_prefetch,
>>   	},
>>   #ifdef CONFIG_ARM64_UAO
>>   	{
>>   		.desc = "User Access Override",
>>   		.capability = ARM64_HAS_UAO,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = has_cpuid_feature,
>>   		.sys_reg = SYS_ID_AA64MMFR2_EL1,
>>   		.field_pos = ID_AA64MMFR2_UAO_SHIFT,
>> @@ -977,21 +977,21 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   #ifdef CONFIG_ARM64_PAN
>>   	{
>>   		.capability = ARM64_ALT_PAN_NOT_UAO,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = cpufeature_pan_not_uao,
>>   	},
>>   #endif /* CONFIG_ARM64_PAN */
>>   	{
>>   		.desc = "Virtualization Host Extensions",
>>   		.capability = ARM64_HAS_VIRT_HOST_EXTN,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = runs_at_el2,
>>   		.cpu_enable = cpu_copy_el2regs,
>>   	},
>>   	{
>>   		.desc = "32-bit EL0 Support",
>>   		.capability = ARM64_HAS_32BIT_EL0,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = has_cpuid_feature,
>>   		.sys_reg = SYS_ID_AA64PFR0_EL1,
>>   		.sign = FTR_UNSIGNED,
>> @@ -1001,21 +1001,21 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   	{
>>   		.desc = "Reduced HYP mapping offset",
>>   		.capability = ARM64_HYP_OFFSET_LOW,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = hyp_offset_low,
>>   	},
>>   #ifdef CONFIG_UNMAP_KERNEL_AT_EL0
>>   	{
>>   		.desc = "Kernel page table isolation (KPTI)",
>>   		.capability = ARM64_UNMAP_KERNEL_AT_EL0,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = unmap_kernel_at_el0,
>>   	},
>>   #endif
>>   	{
>>   		/* FP/SIMD is not implemented */
>>   		.capability = ARM64_HAS_NO_FPSIMD,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.min_field_value = 0,
>>   		.matches = has_no_fpsimd,
>>   	},
>> @@ -1023,7 +1023,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   	{
>>   		.desc = "Data cache clean to Point of Persistence",
>>   		.capability = ARM64_HAS_DCPOP,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = has_cpuid_feature,
>>   		.sys_reg = SYS_ID_AA64ISAR1_EL1,
>>   		.field_pos = ID_AA64ISAR1_DPB_SHIFT,
>> @@ -1033,7 +1033,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   #ifdef CONFIG_ARM64_SVE
>>   	{
>>   		.desc = "Scalable Vector Extension",
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.capability = ARM64_SVE,
>>   		.sys_reg = SYS_ID_AA64PFR0_EL1,
>>   		.sign = FTR_UNSIGNED,
>> @@ -1047,7 +1047,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   	{
>>   		.desc = "RAS Extension Support",
>>   		.capability = ARM64_HAS_RAS_EXTN,
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
>>   		.matches = has_cpuid_feature,
>>   		.sys_reg = SYS_ID_AA64PFR0_EL1,
>>   		.sign = FTR_UNSIGNED,
>> @@ -1062,7 +1062,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
>>   #define HWCAP_CAP(reg, field, s, min_value, cap_type, cap)	\
>>   	{							\
>>   		.desc = #cap,					\
>> -		.type = ARM64_CPUCAP_SCOPE_SYSTEM,		\
>> +		.type = ARM64_CPUCAP_SYSTEM_FEATURE,		\
>>   		.matches = has_cpuid_feature,			\
>>   		.sys_reg = reg,					\
>>   		.field_pos = field,				\
>> -- 
>> 2.14.3
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Powered by blists - more mailing lists