[PATCH] target/arm: don't clobber ID_AA64ISAR1 pointer auth

[PATCH] target/arm: don't clobber ID_AA64ISAR1 pointer auth

[Jamie Iles]

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;
+    }
+
     /* 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;
-- 
2.30.2

Re: [PATCH] target/arm: don't clobber ID_AA64ISAR1 pointer auth

[Richard Henderson]

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~

Re: [PATCH] target/arm: don't clobber ID_AA64ISAR1 pointer auth

[Richard Henderson]

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~

[PATCHv2] target/arm: make pointer authentication a switchable feature

[Jamie Iles]

Rather than having pointer authentication properties be specific to the
max CPU type, turn this into a generic feature that can be set for each
CPU model.  This means that for future CPU types the feature can be set
without having the ID_AA64ISAR1 bits clobbered in
arm_cpu_pauth_finalize.  This also makes it possible for real CPU models
to use the impdef algorithm for improved performance by setting
pauth-impdef=on on the command line.

Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jamie Iles <jamie@nuviainc.com>
---

Following Richard's suggestion to make impdef selectable for all CPUs 
where pointer auth is supported, I've moved this up to a full feature 
and then any future CPUs supporting pointer auth can just set 
ARM_FEATURE_PAUTH.

 target/arm/cpu.h   |  2 ++
 target/arm/cpu64.c | 13 +++++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 272fde83ca4e..f724744c4f2b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -969,6 +969,7 @@ struct ARMCPU {
      */
     bool prop_pauth;
     bool prop_pauth_impdef;
+    bool has_pauth;
 
     /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
     uint32_t dcz_blocksize;
@@ -2115,6 +2116,7 @@ enum arm_features {
     ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
     ARM_FEATURE_M_MAIN, /* M profile Main Extension */
     ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
+    ARM_FEATURE_PAUTH, /* has pointer authentication support */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index f42803ecaf1d..5a4386ce9218 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -760,10 +760,7 @@ static void aarch64_max_initfn(Object *obj)
         cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT icache */
         cpu->dcz_blocksize = 7; /*  512 bytes */
 #endif
-
-        /* Default to PAUTH on, with the architected algorithm. */
-        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
-        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
+        set_feature(&cpu->env, ARM_FEATURE_PAUTH);
     }
 
     aarch64_add_sve_properties(obj);
@@ -835,8 +832,16 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 static void aarch64_cpu_instance_init(Object *obj)
 {
     ARMCPUClass *acc = ARM_CPU_GET_CLASS(obj);
+    ARMCPU *cpu = ARM_CPU(obj);
 
     acc->info->initfn(obj);
+
+    /* Default to PAUTH on, with the architected algorithm. */
+    if (arm_feature(&cpu->env, ARM_FEATURE_PAUTH)) {
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
+    }
+
     arm_cpu_post_init(obj);
 }
 
-- 
2.30.2

Re: [PATCHv2] target/arm: make pointer authentication a switchable feature

[Richard Henderson]

On 5/25/21 2:01 AM, Jamie Iles wrote:
> Rather than having pointer authentication properties be specific to the
> max CPU type, turn this into a generic feature that can be set for each
> CPU model.  This means that for future CPU types the feature can be set
> without having the ID_AA64ISAR1 bits clobbered in
> arm_cpu_pauth_finalize.  This also makes it possible for real CPU models
> to use the impdef algorithm for improved performance by setting
> pauth-impdef=on on the command line.
> 
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Jamie Iles <jamie@nuviainc.com>
> ---
> 
> Following Richard's suggestion to make impdef selectable for all CPUs
> where pointer auth is supported, I've moved this up to a full feature
> and then any future CPUs supporting pointer auth can just set
> ARM_FEATURE_PAUTH.

New patches should not be in-reply-to another thread.
They get lost that way.

>       bool prop_pauth;
>       bool prop_pauth_impdef;
> +    bool has_pauth;

What's this for?  It doesn't even seem to be used in this patch.

> @@ -2115,6 +2116,7 @@ enum arm_features {
>       ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>       ARM_FEATURE_M_MAIN, /* M profile Main Extension */
>       ARM_FEATURE_V8_1M, /* M profile extras only in v8.1M and later */
> +    ARM_FEATURE_PAUTH, /* has pointer authentication support */
>   };
>   
>   static inline int arm_feature(CPUARMState *env, int feature)
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index f42803ecaf1d..5a4386ce9218 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -760,10 +760,7 @@ static void aarch64_max_initfn(Object *obj)
>           cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT icache */
>           cpu->dcz_blocksize = 7; /*  512 bytes */
>   #endif
> -
> -        /* Default to PAUTH on, with the architected algorithm. */
> -        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
> -        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
> +        set_feature(&cpu->env, ARM_FEATURE_PAUTH);

I would rather you split out a function akin to aarch64_add_sve_properties, 
e.g. aarch64_add_pauth_properties.  We would call this function in any 
cpufoo_initfn that enables pauth.  It is hard to say more without the patch 
that adds the cpufoo_initfn which you are interested in.


> +    /* Default to PAUTH on, with the architected algorithm. */
> +    if (arm_feature(&cpu->env, ARM_FEATURE_PAUTH)) {

FWIW, this test is

     cpu_isar_feature(aa64_pauth, cpu)

without having to add ARM_FEATURE_PAUTH.


r~