| Message ID | 20210524084306.1666265-1-jamie@nuviainc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: don't clobber ID_AA64ISAR1 pointer auth | expand |
On 5/24/21 1:43 AM, Jamie Iles wrote: > The pointer auth properties are added to the max CPU type but the > finalization happens for all CPUs. It makes sense to be able to disable > pointer authentication for the max CPU type, but for future CPUs that > implement pointer authentication and have bits set in ID_AA64ISAR1, > don't clobber them unless there is a property registered that can > disable them. > > Cc: Richard Henderson <richard.henderson@linaro.org> > Cc: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Jamie Iles <jamie@nuviainc.com> > --- > target/arm/cpu64.c | 33 +++++++++++++++++++++------------ > 1 file changed, 21 insertions(+), 12 deletions(-) > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > index f0a9e968c9c1..81c9e494acb6 100644 > --- a/target/arm/cpu64.c > +++ b/target/arm/cpu64.c > @@ -575,26 +575,31 @@ void aarch64_add_sve_properties(Object *obj) > > void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) > { > - int arch_val = 0, impdef_val = 0; > + int apa = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, APA); > + int gpa = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, GPA); > + int api = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, API); > + int gpi = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, GPI); > uint64_t t; > > + if (object_property_find(OBJECT(cpu), "pauth-impdef")) { > + api = gpi = cpu->prop_pauth_impdef; > + } > + > + if (object_property_find(OBJECT(cpu), "pauth")) { > + apa = gpa = cpu->prop_pauth; > + } This seems overly complex. If the pauth property doesn't exist, can you just early exit from the function? And surely the pauth-impdef properly would exist if and only if pauth does. r~
On 5/24/21 8:29 PM, Richard Henderson wrote: > On 5/24/21 1:43 AM, Jamie Iles wrote: >> The pointer auth properties are added to the max CPU type but the >> finalization happens for all CPUs. It makes sense to be able to disable >> pointer authentication for the max CPU type, but for future CPUs that >> implement pointer authentication and have bits set in ID_AA64ISAR1, >> don't clobber them unless there is a property registered that can >> disable them. >> >> Cc: Richard Henderson <richard.henderson@linaro.org> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Jamie Iles <jamie@nuviainc.com> >> --- >> target/arm/cpu64.c | 33 +++++++++++++++++++++------------ >> 1 file changed, 21 insertions(+), 12 deletions(-) >> >> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c >> index f0a9e968c9c1..81c9e494acb6 100644 >> --- a/target/arm/cpu64.c >> +++ b/target/arm/cpu64.c >> @@ -575,26 +575,31 @@ void aarch64_add_sve_properties(Object *obj) >> void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) >> { >> - int arch_val = 0, impdef_val = 0; >> + int apa = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, APA); >> + int gpa = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, GPA); >> + int api = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, API); >> + int gpi = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, GPI); >> uint64_t t; >> + if (object_property_find(OBJECT(cpu), "pauth-impdef")) { >> + api = gpi = cpu->prop_pauth_impdef; >> + } >> + >> + if (object_property_find(OBJECT(cpu), "pauth")) { >> + apa = gpa = cpu->prop_pauth; >> + } > > This seems overly complex. If the pauth property doesn't exist, can you just > early exit from the function? And surely the pauth-impdef properly would exist > if and only if pauth does. Alternately, the bug is that the pauth properties have not been registered for the new cpu. Given the performance overhead of the QARMA cipher under TCG, it will always make sense to be able to disable it. r~
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index f0a9e968c9c1..81c9e494acb6 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -575,26 +575,31 @@ void aarch64_add_sve_properties(Object *obj) void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { - int arch_val = 0, impdef_val = 0; + int apa = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, APA); + int gpa = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, GPA); + int api = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, API); + int gpi = FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, GPI); uint64_t t; + if (object_property_find(OBJECT(cpu), "pauth-impdef")) { + api = gpi = cpu->prop_pauth_impdef; + } + + if (object_property_find(OBJECT(cpu), "pauth")) { + apa = gpa = cpu->prop_pauth; + } + /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */ - if (cpu->prop_pauth) { - if (cpu->prop_pauth_impdef) { - impdef_val = 1; - } else { - arch_val = 1; - } - } else if (cpu->prop_pauth_impdef) { + if (cpu->prop_pauth_impdef && !cpu->prop_pauth) { error_setg(errp, "cannot enable pauth-impdef without pauth"); error_append_hint(errp, "Add pauth=on to the CPU property list.\n"); } t = cpu->isar.id_aa64isar1; - t = FIELD_DP64(t, ID_AA64ISAR1, APA, arch_val); - t = FIELD_DP64(t, ID_AA64ISAR1, GPA, arch_val); - t = FIELD_DP64(t, ID_AA64ISAR1, API, impdef_val); - t = FIELD_DP64(t, ID_AA64ISAR1, GPI, impdef_val); + t = FIELD_DP64(t, ID_AA64ISAR1, APA, apa); + t = FIELD_DP64(t, ID_AA64ISAR1, GPA, gpa); + t = FIELD_DP64(t, ID_AA64ISAR1, API, api); + t = FIELD_DP64(t, ID_AA64ISAR1, GPI, gpi); cpu->isar.id_aa64isar1 = t; } @@ -662,6 +667,10 @@ static void aarch64_max_initfn(Object *obj) t = FIELD_DP64(t, ID_AA64ISAR1, SPECRES, 1); t = FIELD_DP64(t, ID_AA64ISAR1, FRINTTS, 1); t = FIELD_DP64(t, ID_AA64ISAR1, LRCPC, 2); /* ARMv8.4-RCPC */ + t = FIELD_DP64(t, ID_AA64ISAR1, APA, 1); + t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 1); + t = FIELD_DP64(t, ID_AA64ISAR1, API, 0); + t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0); cpu->isar.id_aa64isar1 = t; t = cpu->isar.id_aa64pfr0;
The pointer auth properties are added to the max CPU type but the finalization happens for all CPUs. It makes sense to be able to disable pointer authentication for the max CPU type, but for future CPUs that implement pointer authentication and have bits set in ID_AA64ISAR1, don't clobber them unless there is a property registered that can disable them. Cc: Richard Henderson <richard.henderson@linaro.org> Cc: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Jamie Iles <jamie@nuviainc.com> --- target/arm/cpu64.c | 33 +++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-)