Add a facility to globally override a feature, no matter what
the HW says. Yes, this is dangerous.
Nothing uses this yet, so we are pretty safe. For now.
Signed-off-by: Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/cpufeature.h | 2 ++
arch/arm64/kernel/cpufeature.c | 26 +++++++++++++++++++++-----
2 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 9a555809b89c..465d2cb63bfc 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -75,6 +75,8 @@ struct arm64_ftr_reg {
u64 sys_val;
u64 user_val;
const struct arm64_ftr_bits *ftr_bits;
+ u64 *override_val;
+ u64 *override_mask;
};
extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index e99eddec0a46..492321054bd5 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -544,13 +544,17 @@ static const struct arm64_ftr_bits ftr_raz[] = {
ARM64_FTR_END,
};
-#define ARM64_FTR_REG(id, table) { \
- .sys_id = id, \
- .reg = &(struct arm64_ftr_reg){ \
- .name = #id, \
- .ftr_bits = &((table)[0]), \
+#define ARM64_FTR_REG_OVERRIDE(id, table, v, m) { \
+ .sys_id = id, \
+ .reg = &(struct arm64_ftr_reg){ \
+ .name = #id, \
+ .ftr_bits = &((table)[0]), \
+ .override_val = v, \
+ .override_mask = m, \
}}
+#define ARM64_FTR_REG(id, table) ARM64_FTR_REG_OVERRIDE(id, table, NULL, NULL)
+
static const struct __ftr_reg_entry {
u32 sys_id;
struct arm64_ftr_reg *reg;
@@ -786,6 +790,18 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
val &= valid_mask;
+ if (reg->override_mask && reg->override_val) {
+ u64 override = val;
+ override &= ~*reg->override_mask;
+ override |= (*reg->override_val & *reg->override_mask);
+
+ if (val != override)
+ pr_warn("%s: forced from %016llx to %016llx\n",
+ reg->name, val, override);
+
+ val = override;
+ }
+
reg->sys_val = val;
reg->strict_mask = strict_mask;
reg->user_mask = user_mask;
--
2.29.2
Hi Marc,
On Mon, Jan 11, 2021 at 01:27:59PM +0000, Marc Zyngier wrote:
> Add a facility to globally override a feature, no matter what
> the HW says. Yes, this is dangerous.
Yeah, it's dangerous. We can make it less so if we only allow safe
values (e.g. lower if FTR_UNSIGNED).
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 9a555809b89c..465d2cb63bfc 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -75,6 +75,8 @@ struct arm64_ftr_reg {
> u64 sys_val;
> u64 user_val;
> const struct arm64_ftr_bits *ftr_bits;
> + u64 *override_val;
> + u64 *override_mask;
> };
At the arm64_ftr_reg level, we don't have any information about the safe
values for a feature. Could we instead move this to arm64_ftr_bits? We
probably only need a single field. When populating the feature values,
we can make sure it doesn't go above the hardware one.
I attempted a feature modification for MTE here, though I dropped the
entire series in the meantime as we clarified the ARM ARM:
https://lore.kernel.org/linux-arm-kernel/[email protected]/
Srinivas copied it in his patch (but forgot to give credit ;)):
https://lore.kernel.org/linux-arm-msm/[email protected]/
The above adds a filter function but, instead, just use your mechanism in
this series for idreg.feature setting via cmdline. The arm64_ftr_value()
function extracts the hardware value and lowers it if a cmdline argument
was passed.
--
Catalin
Hi Catalin,
On 2021-01-11 18:41, Catalin Marinas wrote:
> Hi Marc,
>
> On Mon, Jan 11, 2021 at 01:27:59PM +0000, Marc Zyngier wrote:
>> Add a facility to globally override a feature, no matter what
>> the HW says. Yes, this is dangerous.
>
> Yeah, it's dangerous. We can make it less so if we only allow safe
> values (e.g. lower if FTR_UNSIGNED).
My plan was also to allow non-safe values in order to trigger features
that are not advertised by the HW. But I can understand if you are
reluctant to allow such thing! :D
>> diff --git a/arch/arm64/include/asm/cpufeature.h
>> b/arch/arm64/include/asm/cpufeature.h
>> index 9a555809b89c..465d2cb63bfc 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -75,6 +75,8 @@ struct arm64_ftr_reg {
>> u64 sys_val;
>> u64 user_val;
>> const struct arm64_ftr_bits *ftr_bits;
>> + u64 *override_val;
>> + u64 *override_mask;
>> };
>
> At the arm64_ftr_reg level, we don't have any information about the
> safe
> values for a feature. Could we instead move this to arm64_ftr_bits? We
> probably only need a single field. When populating the feature values,
> we can make sure it doesn't go above the hardware one.
>
> I attempted a feature modification for MTE here, though I dropped the
> entire series in the meantime as we clarified the ARM ARM:
>
> https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> Srinivas copied it in his patch (but forgot to give credit ;)):
>
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> The above adds a filter function but, instead, just use your mechanism
> in
> this series for idreg.feature setting via cmdline. The
> arm64_ftr_value()
> function extracts the hardware value and lowers it if a cmdline
> argument
> was passed.
One thing is that it is not always possible to sanitise the value passed
if it is required very early on, as I do with VHE. But in that case
I actually check that we are VHE capable before starting to poke at
VHE-specific state.
I came up with the following patch on top, which preserves the current
global approach (no per arm64_ftr_bits state), but checks (and alters)
the override as it iterates through the various fields.
For example, if I pass "arm64.nopauth kvm-arm.mode=nvhe
id_aa64pfr1.bt=5"
to the FVP, I get the following output:
[ 0.000000] CPU features: SYS_ID_AA64ISAR1_EL1[31:28]: forced from 1
to 0
[ 0.000000] CPU features: SYS_ID_AA64ISAR1_EL1[11:8]: forced from 1
to 0
[ 0.000000] CPU features: SYS_ID_AA64MMFR1_EL1[11:8]: forced from 1
to 0
[ 0.000000] CPU features: SYS_ID_AA64PFR1_EL1[3:0]: not forcing 1 to
5
[ 0.000000] CPU features: detected: GIC system register CPU interface
[ 0.000000] CPU features: detected: Hardware dirty bit management
[ 0.000000] CPU features: detected: Spectre-v4
[ 0.000000] CPU features: detected: Branch Target Identification
showing that the PAC features have been downgraded, together with VHE,
but that BTI is still detected as value 5 was obviously bogus.
Thoughts?
M.
diff --git a/arch/arm64/kernel/cpufeature.c
b/arch/arm64/kernel/cpufeature.c
index 894af60b9669..00d99e593b65 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64
new)
u64 strict_mask = ~0x0ULL;
u64 user_mask = 0;
u64 valid_mask = 0;
+ u64 override_val = 0, override_mask = 0;
const struct arm64_ftr_bits *ftrp;
struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
@@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32 sys_reg,
u64 new)
if (!reg)
return;
+ if (reg->override_mask && reg->override_val) {
+ override_mask = *reg->override_mask;
+ override_val = *reg->override_val;
+ }
+
for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
u64 ftr_mask = arm64_ftr_mask(ftrp);
s64 ftr_new = arm64_ftr_value(ftrp, new);
+ s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
+
+ if ((ftr_mask & override_mask) == ftr_mask) {
+ if (ftr_ovr < ftr_new) {
+ pr_warn("%s[%d:%d]: forced from %llx to %llx\n",
+ reg->name,
+ ftrp->shift + ftrp->width - 1,
+ ftrp->shift, ftr_new, ftr_ovr);
+
+ ftr_new = ftr_ovr;
+ } else if (ftr_ovr != ftr_new) {
+ pr_warn("%s[%d:%d]: not forcing %llx to %llx\n",
+ reg->name,
+ ftrp->shift + ftrp->width - 1,
+ ftrp->shift, ftr_new, ftr_ovr);
+
+ /* Remove the override */
+ *reg->override_mask &= ~ftr_mask;
+ *reg->override_val &= ~ftr_mask;
+ }
+ }
val = arm64_ftr_set_value(ftrp, val, ftr_new);
@@ -800,18 +827,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg,
u64 new)
val &= valid_mask;
- if (reg->override_mask && reg->override_val) {
- u64 override = val;
- override &= ~*reg->override_mask;
- override |= (*reg->override_val & *reg->override_mask);
-
- if (val != override)
- pr_warn("%s: forced from %016llx to %016llx\n",
- reg->name, val, override);
-
- val = override;
- }
-
reg->sys_val = val;
reg->strict_mask = strict_mask;
reg->user_mask = user_mask;
--
Jazz is not dead. It just smells funny...
Hi Catalin,
On 1/11/2021 10:41 AM, Catalin Marinas wrote:
> Hi Marc,
>
> On Mon, Jan 11, 2021 at 01:27:59PM +0000, Marc Zyngier wrote:
>> Add a facility to globally override a feature, no matter what
>> the HW says. Yes, this is dangerous.
> Yeah, it's dangerous. We can make it less so if we only allow safe
> values (e.g. lower if FTR_UNSIGNED).
>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index 9a555809b89c..465d2cb63bfc 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -75,6 +75,8 @@ struct arm64_ftr_reg {
>> u64 sys_val;
>> u64 user_val;
>> const struct arm64_ftr_bits *ftr_bits;
>> + u64 *override_val;
>> + u64 *override_mask;
>> };
> At the arm64_ftr_reg level, we don't have any information about the safe
> values for a feature. Could we instead move this to arm64_ftr_bits? We
> probably only need a single field. When populating the feature values,
> we can make sure it doesn't go above the hardware one.
>
> I attempted a feature modification for MTE here, though I dropped the
> entire series in the meantime as we clarified the ARM ARM:
>
> https://lore.kernel.org/linux-arm-kernel/[email protected]/
>
> Srinivas copied it in his patch (but forgot to give credit ;)):
Sorry about that. I did mention that its taken from your patch-set in my
cover letter. But missed your signed-off-by in the patch.
https://lore.kernel.org/linux-arm-msm/[email protected]/T/#m1ae76e6096c07ab5f1636a4e383a3fd6cfb4665f
Since we can ignore my patch with the mechanism added by Marc, I am not
re-sending this. Thanks.
>
> https://lore.kernel.org/linux-arm-msm/[email protected]/
>
> The above adds a filter function but, instead, just use your mechanism in
> this series for idreg.feature setting via cmdline. The arm64_ftr_value()
> function extracts the hardware value and lowers it if a cmdline argument
> was passed.
>
Hi Marc,
On 1/11/21 7:48 PM, Marc Zyngier wrote:
> Hi Catalin,
>
> On 2021-01-11 18:41, Catalin Marinas wrote:
>> Hi Marc,
>>
>> On Mon, Jan 11, 2021 at 01:27:59PM +0000, Marc Zyngier wrote:
>>> Add a facility to globally override a feature, no matter what
>>> the HW says. Yes, this is dangerous.
>>
>> Yeah, it's dangerous. We can make it less so if we only allow safe
>> values (e.g. lower if FTR_UNSIGNED).
>
> My plan was also to allow non-safe values in order to trigger features
> that are not advertised by the HW. But I can understand if you are
> reluctant to allow such thing! :D
>
>>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>>> index 9a555809b89c..465d2cb63bfc 100644
>>> --- a/arch/arm64/include/asm/cpufeature.h
>>> +++ b/arch/arm64/include/asm/cpufeature.h
>>> @@ -75,6 +75,8 @@ struct arm64_ftr_reg {
>>> u64 sys_val;
>>> u64 user_val;
>>> const struct arm64_ftr_bits *ftr_bits;
>>> + u64 *override_val;
>>> + u64 *override_mask;
>>> };
>>
>> At the arm64_ftr_reg level, we don't have any information about the safe
>> values for a feature. Could we instead move this to arm64_ftr_bits? We
>> probably only need a single field. When populating the feature values,
>> we can make sure it doesn't go above the hardware one.
>>
>> I attempted a feature modification for MTE here, though I dropped the
>> entire series in the meantime as we clarified the ARM ARM:
>>
>> https://lore.kernel.org/linux-arm-kernel/[email protected]/
>>
>> Srinivas copied it in his patch (but forgot to give credit ;)):
>>
>> https://lore.kernel.org/linux-arm-msm/[email protected]/
>>
>> The above adds a filter function but, instead, just use your mechanism in
>> this series for idreg.feature setting via cmdline. The arm64_ftr_value()
>> function extracts the hardware value and lowers it if a cmdline argument
>> was passed.
>
> One thing is that it is not always possible to sanitise the value passed
> if it is required very early on, as I do with VHE. But in that case
> I actually check that we are VHE capable before starting to poke at
> VHE-specific state.
>
> I came up with the following patch on top, which preserves the current
> global approach (no per arm64_ftr_bits state), but checks (and alters)
> the override as it iterates through the various fields.
>
> For example, if I pass "arm64.nopauth kvm-arm.mode=nvhe id_aa64pfr1.bt=5"
> to the FVP, I get the following output:
>
> [ 0.000000] CPU features: SYS_ID_AA64ISAR1_EL1[31:28]: forced from 1 to 0
> [ 0.000000] CPU features: SYS_ID_AA64ISAR1_EL1[11:8]: forced from 1 to 0
> [ 0.000000] CPU features: SYS_ID_AA64MMFR1_EL1[11:8]: forced from 1 to 0
> [ 0.000000] CPU features: SYS_ID_AA64PFR1_EL1[3:0]: not forcing 1 to 5
> [ 0.000000] CPU features: detected: GIC system register CPU interface
> [ 0.000000] CPU features: detected: Hardware dirty bit management
> [ 0.000000] CPU features: detected: Spectre-v4
> [ 0.000000] CPU features: detected: Branch Target Identification
>
> showing that the PAC features have been downgraded, together with VHE,
> but that BTI is still detected as value 5 was obviously bogus.
>
> Thoughts?
>
> M.
>
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 894af60b9669..00d99e593b65 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
> u64 strict_mask = ~0x0ULL;
> u64 user_mask = 0;
> u64 valid_mask = 0;
> + u64 override_val = 0, override_mask = 0;
>
> const struct arm64_ftr_bits *ftrp;
> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
> @@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
> if (!reg)
> return;
>
> + if (reg->override_mask && reg->override_val) {
> + override_mask = *reg->override_mask;
> + override_val = *reg->override_val;
> + }
> +
> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
> u64 ftr_mask = arm64_ftr_mask(ftrp);
> s64 ftr_new = arm64_ftr_value(ftrp, new);
> + s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
> +
> + if ((ftr_mask & override_mask) == ftr_mask) {
> + if (ftr_ovr < ftr_new) {
Here we assume that all the features are FTR_LOWER_SAFE. We could
probably use arm64_ftr_safe_value(ftrp, ftr_new, ftr_ovr) here ?
That would cover us for both HIGHER_SAFE and LOWER_SAFE features.
However that may be restrictive for FTR_EXACT, as we the safe
value would be set to "ftr->safe_val". I guess that may be better
than forcing to use an unsafe value for the boot CPU, which could
anyway conflict with the other CPUs and eventually trigger the
ftr alue to be safe_val.
i.e,
ftr_val = arm64_ftr_safe_value(ftrp, ftr_ovr, ftr_new);
Suzuki
Hi Suzuki,
On 2021-01-12 09:17, Suzuki K Poulose wrote:
> Hi Marc,
>
> On 1/11/21 7:48 PM, Marc Zyngier wrote:
[...]
>> diff --git a/arch/arm64/kernel/cpufeature.c
>> b/arch/arm64/kernel/cpufeature.c
>> index 894af60b9669..00d99e593b65 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg,
>> u64 new)
>> u64 strict_mask = ~0x0ULL;
>> u64 user_mask = 0;
>> u64 valid_mask = 0;
>> + u64 override_val = 0, override_mask = 0;
>>
>> const struct arm64_ftr_bits *ftrp;
>> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>> @@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32 sys_reg,
>> u64 new)
>> if (!reg)
>> return;
>>
>> + if (reg->override_mask && reg->override_val) {
>> + override_mask = *reg->override_mask;
>> + override_val = *reg->override_val;
>> + }
>> +
>> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>> u64 ftr_mask = arm64_ftr_mask(ftrp);
>> s64 ftr_new = arm64_ftr_value(ftrp, new);
>> + s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
>> +
>> + if ((ftr_mask & override_mask) == ftr_mask) {
>> + if (ftr_ovr < ftr_new) {
>
> Here we assume that all the features are FTR_LOWER_SAFE. We could
> probably use arm64_ftr_safe_value(ftrp, ftr_new, ftr_ovr) here ?
> That would cover us for both HIGHER_SAFE and LOWER_SAFE features.
> However that may be restrictive for FTR_EXACT, as we the safe
> value would be set to "ftr->safe_val". I guess that may be better
> than forcing to use an unsafe value for the boot CPU, which could
> anyway conflict with the other CPUs and eventually trigger the
> ftr alue to be safe_val.
I like the idea of using the helper, as it cleanups up the code a bit.
However, not being to set a feature to a certain value could be
restrictive,
as in general, it means that we can only disable a feature and not
adjust
its level of support.
Take PMUVER for example: with the helper, I can't override it from v8.4
to
v8.1. I can only go to v8.0.
Is it something we care about?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 2021-01-12 11:50, Marc Zyngier wrote:
> Hi Suzuki,
>
> On 2021-01-12 09:17, Suzuki K Poulose wrote:
>> Hi Marc,
>>
>> On 1/11/21 7:48 PM, Marc Zyngier wrote:
>
> [...]
>
>>> diff --git a/arch/arm64/kernel/cpufeature.c
>>> b/arch/arm64/kernel/cpufeature.c
>>> index 894af60b9669..00d99e593b65 100644
>>> --- a/arch/arm64/kernel/cpufeature.c
>>> +++ b/arch/arm64/kernel/cpufeature.c
>>> @@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg,
>>> u64 new)
>>> u64 strict_mask = ~0x0ULL;
>>> u64 user_mask = 0;
>>> u64 valid_mask = 0;
>>> + u64 override_val = 0, override_mask = 0;
>>>
>>> const struct arm64_ftr_bits *ftrp;
>>> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>>> @@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32 sys_reg,
>>> u64 new)
>>> if (!reg)
>>> return;
>>>
>>> + if (reg->override_mask && reg->override_val) {
>>> + override_mask = *reg->override_mask;
>>> + override_val = *reg->override_val;
>>> + }
>>> +
>>> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>>> u64 ftr_mask = arm64_ftr_mask(ftrp);
>>> s64 ftr_new = arm64_ftr_value(ftrp, new);
>>> + s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
>>> +
>>> + if ((ftr_mask & override_mask) == ftr_mask) {
>>> + if (ftr_ovr < ftr_new) {
>>
>> Here we assume that all the features are FTR_LOWER_SAFE. We could
>> probably use arm64_ftr_safe_value(ftrp, ftr_new, ftr_ovr) here ?
>> That would cover us for both HIGHER_SAFE and LOWER_SAFE features.
>> However that may be restrictive for FTR_EXACT, as we the safe
>> value would be set to "ftr->safe_val". I guess that may be better
>> than forcing to use an unsafe value for the boot CPU, which could
>> anyway conflict with the other CPUs and eventually trigger the
>> ftr alue to be safe_val.
>
> I like the idea of using the helper, as it cleanups up the code a bit.
> However, not being to set a feature to a certain value could be
> restrictive,
> as in general, it means that we can only disable a feature and not
> adjust
> its level of support.
>
> Take PMUVER for example: with the helper, I can't override it from v8.4
> to
> v8.1. I can only go to v8.0.
Actually, we can only *disable* the PMU altogether. Same question
though...
M.
>
> Is it something we care about?
>
> Thanks,
>
> M.
--
Jazz is not dead. It just smells funny...
On 1/12/21 11:50 AM, Marc Zyngier wrote:
> Hi Suzuki,
>
> On 2021-01-12 09:17, Suzuki K Poulose wrote:
>> Hi Marc,
>>
>> On 1/11/21 7:48 PM, Marc Zyngier wrote:
>
> [...]
>
>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>> index 894af60b9669..00d99e593b65 100644
>>> --- a/arch/arm64/kernel/cpufeature.c
>>> +++ b/arch/arm64/kernel/cpufeature.c
>>> @@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
>>> u64 strict_mask = ~0x0ULL;
>>> u64 user_mask = 0;
>>> u64 valid_mask = 0;
>>> + u64 override_val = 0, override_mask = 0;
>>>
>>> const struct arm64_ftr_bits *ftrp;
>>> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>>> @@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
>>> if (!reg)
>>> return;
>>>
>>> + if (reg->override_mask && reg->override_val) {
>>> + override_mask = *reg->override_mask;
>>> + override_val = *reg->override_val;
>>> + }
>>> +
>>> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>>> u64 ftr_mask = arm64_ftr_mask(ftrp);
>>> s64 ftr_new = arm64_ftr_value(ftrp, new);
>>> + s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
>>> +
>>> + if ((ftr_mask & override_mask) == ftr_mask) {
>>> + if (ftr_ovr < ftr_new) {
>>
>> Here we assume that all the features are FTR_LOWER_SAFE. We could
>> probably use arm64_ftr_safe_value(ftrp, ftr_new, ftr_ovr) here ?
>> That would cover us for both HIGHER_SAFE and LOWER_SAFE features.
>> However that may be restrictive for FTR_EXACT, as we the safe
>> value would be set to "ftr->safe_val". I guess that may be better
>> than forcing to use an unsafe value for the boot CPU, which could
>> anyway conflict with the other CPUs and eventually trigger the
>> ftr alue to be safe_val.
>
> I like the idea of using the helper, as it cleanups up the code a bit.
> However, not being to set a feature to a certain value could be restrictive,
> as in general, it means that we can only disable a feature and not adjust
> its level of support.
>
> Take PMUVER for example: with the helper, I can't override it from v8.4 to
> v8.1. I can only go to v8.0.
My point is, we set this only for the "init" of cpu features. So, even if we
init to a custom , non-(default-safe) value, the secondary CPUs could scream,
and the system wide safe value could fall back to the "safe" value for EXACT features,
no matter what you did to init it.
Also, it will be dangerous for things like PAC, as the lower level value may
not be compatible with the "actual" cpu feature supported.
So simply setting to a lower value for EXACT features is generally not safe,
though I understand some are exceptions.
Suzuki
>
> Is it something we care about?
>
> Thanks,
>
> M.
On 1/12/21 11:51 AM, Marc Zyngier wrote:
> On 2021-01-12 11:50, Marc Zyngier wrote:
>> Hi Suzuki,
>>
>> On 2021-01-12 09:17, Suzuki K Poulose wrote:
>>> Hi Marc,
>>>
>>> On 1/11/21 7:48 PM, Marc Zyngier wrote:
>>
>> [...]
>>
>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>> index 894af60b9669..00d99e593b65 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
>>>> u64 strict_mask = ~0x0ULL;
>>>> u64 user_mask = 0;
>>>> u64 valid_mask = 0;
>>>> + u64 override_val = 0, override_mask = 0;
>>>>
>>>> const struct arm64_ftr_bits *ftrp;
>>>> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>>>> @@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
>>>> if (!reg)
>>>> return;
>>>>
>>>> + if (reg->override_mask && reg->override_val) {
>>>> + override_mask = *reg->override_mask;
>>>> + override_val = *reg->override_val;
>>>> + }
>>>> +
>>>> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>>>> u64 ftr_mask = arm64_ftr_mask(ftrp);
>>>> s64 ftr_new = arm64_ftr_value(ftrp, new);
>>>> + s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
>>>> +
>>>> + if ((ftr_mask & override_mask) == ftr_mask) {
>>>> + if (ftr_ovr < ftr_new) {
>>>
>>> Here we assume that all the features are FTR_LOWER_SAFE. We could
>>> probably use arm64_ftr_safe_value(ftrp, ftr_new, ftr_ovr) here ?
>>> That would cover us for both HIGHER_SAFE and LOWER_SAFE features.
>>> However that may be restrictive for FTR_EXACT, as we the safe
>>> value would be set to "ftr->safe_val". I guess that may be better
>>> than forcing to use an unsafe value for the boot CPU, which could
>>> anyway conflict with the other CPUs and eventually trigger the
>>> ftr alue to be safe_val.
>>
>> I like the idea of using the helper, as it cleanups up the code a bit.
>> However, not being to set a feature to a certain value could be restrictive,
>> as in general, it means that we can only disable a feature and not adjust
>> its level of support.
>>
>> Take PMUVER for example: with the helper, I can't override it from v8.4 to
>> v8.1. I can only go to v8.0.
>
> Actually, we can only *disable* the PMU altogether. Same question though...
It depends on two things :
1) What is the safe value for an EXACT typed feature ?
Usually, that means either disabled, or the lowest possible value.
2) How is this value consumed ?
a) i.e, Do we use the per-CPU value
Then none of these changes have any effect
b) System wide value ?
Then we get the safe value as "influenced" by the infrastructure.
The safe value we use for EXACT features is exclusively for making sure
that the system uses the safe assumption and thus should be the best
option.
To answer your question, for PMU, it is 0, implies, v8.0. Or we could
update the safe value to -1 (0xf) as the safe value, which is a bit more safer,
kind of implying that the PMU is not a standard one.
Cheers
Suzuki
>
> M.
>
>>
>> Is it something we care about?
>>
>> Thanks,
>>
>> M.
>
On 2021-01-12 11:59, Suzuki K Poulose wrote:
> On 1/12/21 11:50 AM, Marc Zyngier wrote:
>> Hi Suzuki,
>>
>> On 2021-01-12 09:17, Suzuki K Poulose wrote:
>>> Hi Marc,
>>>
>>> On 1/11/21 7:48 PM, Marc Zyngier wrote:
>>
>> [...]
>>
>>>> diff --git a/arch/arm64/kernel/cpufeature.c
>>>> b/arch/arm64/kernel/cpufeature.c
>>>> index 894af60b9669..00d99e593b65 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg,
>>>> u64 new)
>>>> u64 strict_mask = ~0x0ULL;
>>>> u64 user_mask = 0;
>>>> u64 valid_mask = 0;
>>>> + u64 override_val = 0, override_mask = 0;
>>>>
>>>> const struct arm64_ftr_bits *ftrp;
>>>> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>>>> @@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32
>>>> sys_reg, u64 new)
>>>> if (!reg)
>>>> return;
>>>>
>>>> + if (reg->override_mask && reg->override_val) {
>>>> + override_mask = *reg->override_mask;
>>>> + override_val = *reg->override_val;
>>>> + }
>>>> +
>>>> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>>>> u64 ftr_mask = arm64_ftr_mask(ftrp);
>>>> s64 ftr_new = arm64_ftr_value(ftrp, new);
>>>> + s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
>>>> +
>>>> + if ((ftr_mask & override_mask) == ftr_mask) {
>>>> + if (ftr_ovr < ftr_new) {
>>>
>>> Here we assume that all the features are FTR_LOWER_SAFE. We could
>>> probably use arm64_ftr_safe_value(ftrp, ftr_new, ftr_ovr) here ?
>>> That would cover us for both HIGHER_SAFE and LOWER_SAFE features.
>>> However that may be restrictive for FTR_EXACT, as we the safe
>>> value would be set to "ftr->safe_val". I guess that may be better
>>> than forcing to use an unsafe value for the boot CPU, which could
>>> anyway conflict with the other CPUs and eventually trigger the
>>> ftr alue to be safe_val.
>>
>> I like the idea of using the helper, as it cleanups up the code a bit.
>> However, not being to set a feature to a certain value could be
>> restrictive,
>> as in general, it means that we can only disable a feature and not
>> adjust
>> its level of support.
>>
>> Take PMUVER for example: with the helper, I can't override it from
>> v8.4 to
>> v8.1. I can only go to v8.0.
>
> My point is, we set this only for the "init" of cpu features. So, even
> if we
> init to a custom , non-(default-safe) value, the secondary CPUs could
> scream,
> and the system wide safe value could fall back to the "safe" value for
> EXACT features, no matter what you did to init it.
Right. So let's go with the safe value for EXACT features for now,
and let the override fail if that's not what the user asked for.
After all, there are only so many things we want to support as
an override, and in all the cases at hand, using the safe value
actually matches what we want to do.
We can always revisit this if and when we need a different behaviour.
Thanks,
M.
--
Jazz is not dead. It just smells funny...