* [PATCH v2 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection
2023-02-22 19:35 [PATCH v2 0/7] Implement Most ARMv8.3 Pointer Authentication Features Aaron Lindsay
@ 2023-02-22 19:35 ` Aaron Lindsay
2023-02-22 20:27 ` Richard Henderson
2023-02-22 19:35 ` [PATCH v2 2/7] target/arm: Implement v8.3 QARMA3 PAC cipher Aaron Lindsay
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Aaron Lindsay @ 2023-02-22 19:35 UTC (permalink / raw)
To: qemu-devel, qemu-arm, Richard Henderson, Vincent Dehors,
Alex Bennée, Peter Maydell
Cc: Aaron Lindsay
Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
target/arm/cpu.h | 61 +++++++++++++++++++++++++++++++++++++--
target/arm/helper.c | 4 +--
target/arm/pauth_helper.c | 2 +-
3 files changed, 61 insertions(+), 6 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8cf70693be..9c3cbc9a29 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1001,6 +1001,7 @@ struct ArchCPU {
uint32_t dbgdevid1;
uint64_t id_aa64isar0;
uint64_t id_aa64isar1;
+ uint64_t id_aa64isar2;
uint64_t id_aa64pfr0;
uint64_t id_aa64pfr1;
uint64_t id_aa64mmfr0;
@@ -3902,18 +3903,72 @@ static inline bool isar_feature_aa64_pauth(const ARMISARegisters *id)
(FIELD_DP64(0, ID_AA64ISAR1, APA, 0xf) |
FIELD_DP64(0, ID_AA64ISAR1, API, 0xf) |
FIELD_DP64(0, ID_AA64ISAR1, GPA, 0xf) |
- FIELD_DP64(0, ID_AA64ISAR1, GPI, 0xf))) != 0;
+ FIELD_DP64(0, ID_AA64ISAR1, GPI, 0xf))) != 0 ||
+ (id->id_aa64isar2 &
+ (FIELD_DP64(0, ID_AA64ISAR2, APA3, 0xf) |
+ FIELD_DP64(0, ID_AA64ISAR2, GPA3, 0xf))) != 0;
}
-static inline bool isar_feature_aa64_pauth_arch(const ARMISARegisters *id)
+static inline bool isar_feature_aa64_pauth_arch_qarma5(const ARMISARegisters *id)
{
/*
- * Return true if pauth is enabled with the architected QARMA algorithm.
+ * Return true if pauth is enabled with the architected QARMA5 algorithm.
* QEMU will always set APA+GPA to the same value.
*/
return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA) != 0;
}
+static inline bool isar_feature_aa64_pauth_arch_qarma3(const ARMISARegisters *id)
+{
+ /*
+ * Return true if pauth is enabled with the architected QARMA3 algorithm.
+ * QEMU will always set APA3+GPA3 to the same value.
+ */
+ return FIELD_EX64(id->id_aa64isar2, ID_AA64ISAR2, APA3) != 0;
+}
+
+static inline bool isar_feature_aa64_pauth_arch(const ARMISARegisters *id)
+{
+ return isar_feature_aa64_pauth_arch_qarma5(id) ||
+ isar_feature_aa64_pauth_arch_qarma3(id);
+}
+
+static inline uint8_t isar_feature_pauth_get_features(const ARMISARegisters *id)
+{
+ if (isar_feature_aa64_pauth_arch_qarma5(id))
+ return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA);
+ else if (isar_feature_aa64_pauth_arch_qarma3(id))
+ return FIELD_EX64(id->id_aa64isar2, ID_AA64ISAR2, APA3);
+ else
+ return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, API);
+}
+
+static inline bool isar_feature_aa64_pauth_epac(const ARMISARegisters *id)
+{
+ /*
+ * Note that unlike most AArch64 features, EPAC is treated (in the ARM
+ * psedocode, at least) as not being implemented by larger values of this
+ * field. Our usage of '>=' rather than '==' here causes our implementation
+ * of PAC logic to diverge slightly from ARM pseudocode.
+ */
+ return isar_feature_pauth_get_features(id) >= 0b0010;
+}
+
+static inline bool isar_feature_aa64_pauth2(const ARMISARegisters *id)
+{
+ return isar_feature_pauth_get_features(id) >= 0b0011;
+}
+
+static inline bool isar_feature_aa64_fpac(const ARMISARegisters *id)
+{
+ return isar_feature_pauth_get_features(id) >= 0b0100;
+}
+
+static inline bool isar_feature_aa64_fpac_combine(const ARMISARegisters *id)
+{
+ return isar_feature_pauth_get_features(id) >= 0b0101;
+}
+
static inline bool isar_feature_aa64_tlbirange(const ARMISARegisters *id)
{
return FIELD_EX64(id->id_aa64isar0, ID_AA64ISAR0, TLB) == 2;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 72b37b7cf1..448ebf8301 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8028,11 +8028,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
.access = PL1_R, .type = ARM_CP_CONST,
.accessfn = access_aa64_tid3,
.resetvalue = cpu->isar.id_aa64isar1 },
- { .name = "ID_AA64ISAR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
+ { .name = "ID_AA64ISAR2_EL1", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2,
.access = PL1_R, .type = ARM_CP_CONST,
.accessfn = access_aa64_tid3,
- .resetvalue = 0 },
+ .resetvalue = cpu->isar.id_aa64isar2 },
{ .name = "ID_AA64ISAR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
.opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 3,
.access = PL1_R, .type = ARM_CP_CONST,
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index d0483bf051..e5206453f6 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -282,7 +282,7 @@ static uint64_t pauth_computepac_impdef(uint64_t data, uint64_t modifier,
static uint64_t pauth_computepac(CPUARMState *env, uint64_t data,
uint64_t modifier, ARMPACKey key)
{
- if (cpu_isar_feature(aa64_pauth_arch, env_archcpu(env))) {
+ if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
return pauth_computepac_architected(data, modifier, key);
} else {
return pauth_computepac_impdef(data, modifier, key);
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection
2023-02-22 19:35 ` [PATCH v2 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection Aaron Lindsay
@ 2023-02-22 20:27 ` Richard Henderson
2023-02-23 14:02 ` Peter Maydell
0 siblings, 1 reply; 22+ messages in thread
From: Richard Henderson @ 2023-02-22 20:27 UTC (permalink / raw)
To: Aaron Lindsay, qemu-devel, qemu-arm, Vincent Dehors,
Alex Bennée, Peter Maydell
On 2/22/23 09:35, Aaron Lindsay wrote:
> +static inline bool isar_feature_aa64_pauth_arch_qarma3(const ARMISARegisters *id)
> +{
> + /*
> + * Return true if pauth is enabled with the architected QARMA3 algorithm.
> + * QEMU will always set APA3+GPA3 to the same value.
> + */
This language isn't quite right, since GPA3 only defines values 0 and 1.
Perhaps "to the same result"?
> +static inline uint8_t isar_feature_pauth_get_features(const ARMISARegisters *id)
'int' is a better generic result, as 'uint8_t' is 'unsigned char' to the debugger and
generally printed as such.
> + if (isar_feature_aa64_pauth_arch_qarma5(id))
> + return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA);
> + else if (isar_feature_aa64_pauth_arch_qarma3(id))
> + return FIELD_EX64(id->id_aa64isar2, ID_AA64ISAR2, APA3);
> + else
> + return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, API);
Braces with if+else, always.
That said, exactly one of these fields is allowed to be non-zero, so we can just
unconditionally OR them all together.
> +static inline bool isar_feature_aa64_pauth_epac(const ARMISARegisters *id)
> +{
> + /*
> + * Note that unlike most AArch64 features, EPAC is treated (in the ARM
> + * psedocode, at least) as not being implemented by larger values of this
> + * field. Our usage of '>=' rather than '==' here causes our implementation
> + * of PAC logic to diverge slightly from ARM pseudocode.
> + */
I find this comment scary -- "diverge slightly"?
All I need is once sentence to indicate how this is mitigated (by testing pauth2 first
where required?), or "See function_foo" (where there is more commentary), or something.
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 72b37b7cf1..448ebf8301 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8028,11 +8028,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> .access = PL1_R, .type = ARM_CP_CONST,
> .accessfn = access_aa64_tid3,
> .resetvalue = cpu->isar.id_aa64isar1 },
> - { .name = "ID_AA64ISAR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
> + { .name = "ID_AA64ISAR2_EL1", .state = ARM_CP_STATE_AA64,
> .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2,
> .access = PL1_R, .type = ARM_CP_CONST,
> .accessfn = access_aa64_tid3,
> - .resetvalue = 0 },
> + .resetvalue = cpu->isar.id_aa64isar2 },
All the code adding aa64isar2 should be a separate patch.
You've missed initializing it in kvm_arm_get_host_cpu_features and
hvf_arm_get_host_cpu_features.
r~
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection
2023-02-22 20:27 ` Richard Henderson
@ 2023-02-23 14:02 ` Peter Maydell
0 siblings, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2023-02-23 14:02 UTC (permalink / raw)
To: Richard Henderson
Cc: Aaron Lindsay, qemu-devel, qemu-arm, Vincent Dehors,
Alex Bennée
On Wed, 22 Feb 2023 at 20:27, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/22/23 09:35, Aaron Lindsay wrote:
> > +static inline bool isar_feature_aa64_pauth_epac(const ARMISARegisters *id)
> > +{
> > + /*
> > + * Note that unlike most AArch64 features, EPAC is treated (in the ARM
> > + * psedocode, at least) as not being implemented by larger values of this
> > + * field. Our usage of '>=' rather than '==' here causes our implementation
> > + * of PAC logic to diverge slightly from ARM pseudocode.
> > + */
>
> I find this comment scary -- "diverge slightly"?
>
> All I need is once sentence to indicate how this is mitigated (by testing pauth2 first
> where required?), or "See function_foo" (where there is more commentary), or something.
Yeah, we structure the one place the check is used (patch 4) so that
we only check the pauth_epac feature if we already tested pauth2:
+ if (cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {
+ /* No action required */
+ } else if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
pac = 0;
} else {
where the pseudocode currently has:
if HaveEnhancedPAC() then
pac = 0;
elsif !HaveEnhancedPAC2() then
old stuff;
and is relying on anything with PAuth2 not returning true for HaveEnhancedPAC().
It is of course possible that the pseudocode might be rephrased in future;
I think the way they've done it at the moment is kind of confusing.
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 2/7] target/arm: Implement v8.3 QARMA3 PAC cipher
2023-02-22 19:35 [PATCH v2 0/7] Implement Most ARMv8.3 Pointer Authentication Features Aaron Lindsay
2023-02-22 19:35 ` [PATCH v2 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection Aaron Lindsay
@ 2023-02-22 19:35 ` Aaron Lindsay
2023-02-22 20:35 ` Richard Henderson
2023-02-22 19:35 ` [PATCH v2 3/7] target/arm: Implement v8.3 EnhancedPAC Aaron Lindsay
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Aaron Lindsay @ 2023-02-22 19:35 UTC (permalink / raw)
To: qemu-devel, qemu-arm, Richard Henderson, Vincent Dehors,
Alex Bennée, Peter Maydell
Cc: Aaron Lindsay
Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/pauth_helper.c | 50 +++++++++++++++++++++++++++++++--------
1 file changed, 40 insertions(+), 10 deletions(-)
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index e5206453f6..f525ef7fad 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -96,6 +96,21 @@ static uint64_t pac_sub(uint64_t i)
return o;
}
+static uint64_t pac_sub1(uint64_t i)
+{
+ static const uint8_t sub1[16] = {
+ 0xa, 0xd, 0xe, 0x6, 0xf, 0x7, 0x3, 0x5,
+ 0x9, 0x8, 0x0, 0xc, 0xb, 0x1, 0x2, 0x4,
+ };
+ uint64_t o = 0;
+ int b;
+
+ for (b = 0; b < 64; b += 4) {
+ o |= (uint64_t)sub1[(i >> b) & 0xf] << b;
+ }
+ return o;
+}
+
static uint64_t pac_inv_sub(uint64_t i)
{
static const uint8_t inv_sub[16] = {
@@ -209,7 +224,7 @@ static uint64_t tweak_inv_shuffle(uint64_t i)
}
static uint64_t pauth_computepac_architected(uint64_t data, uint64_t modifier,
- ARMPACKey key)
+ ARMPACKey key, bool isqarma3)
{
static const uint64_t RC[5] = {
0x0000000000000000ull,
@@ -219,6 +234,7 @@ static uint64_t pauth_computepac_architected(uint64_t data, uint64_t modifier,
0x452821E638D01377ull,
};
const uint64_t alpha = 0xC0AC29B7C97C50DDull;
+ int iterations = isqarma3 ? 2 : 4;
/*
* Note that in the ARM pseudocode, key0 contains bits <127:64>
* and key1 contains bits <63:0> of the 128-bit key.
@@ -231,7 +247,7 @@ static uint64_t pauth_computepac_architected(uint64_t data, uint64_t modifier,
runningmod = modifier;
workingval = data ^ key0;
- for (i = 0; i <= 4; ++i) {
+ for (i = 0; i <= iterations; ++i) {
roundkey = key1 ^ runningmod;
workingval ^= roundkey;
workingval ^= RC[i];
@@ -239,32 +255,44 @@ static uint64_t pauth_computepac_architected(uint64_t data, uint64_t modifier,
workingval = pac_cell_shuffle(workingval);
workingval = pac_mult(workingval);
}
- workingval = pac_sub(workingval);
+ if (isqarma3)
+ workingval = pac_sub1(workingval);
+ else
+ workingval = pac_sub(workingval);
runningmod = tweak_shuffle(runningmod);
}
roundkey = modk0 ^ runningmod;
workingval ^= roundkey;
workingval = pac_cell_shuffle(workingval);
workingval = pac_mult(workingval);
- workingval = pac_sub(workingval);
+ if (isqarma3)
+ workingval = pac_sub1(workingval);
+ else
+ workingval = pac_sub(workingval);
workingval = pac_cell_shuffle(workingval);
workingval = pac_mult(workingval);
workingval ^= key1;
workingval = pac_cell_inv_shuffle(workingval);
- workingval = pac_inv_sub(workingval);
+ if (isqarma3)
+ workingval = pac_sub1(workingval);
+ else
+ workingval = pac_inv_sub(workingval);
workingval = pac_mult(workingval);
workingval = pac_cell_inv_shuffle(workingval);
workingval ^= key0;
workingval ^= runningmod;
- for (i = 0; i <= 4; ++i) {
- workingval = pac_inv_sub(workingval);
- if (i < 4) {
+ for (i = 0; i <= iterations; ++i) {
+ if (isqarma3)
+ workingval = pac_sub1(workingval);
+ else
+ workingval = pac_inv_sub(workingval);
+ if (i < iterations) {
workingval = pac_mult(workingval);
workingval = pac_cell_inv_shuffle(workingval);
}
runningmod = tweak_inv_shuffle(runningmod);
roundkey = key1 ^ runningmod;
- workingval ^= RC[4 - i];
+ workingval ^= RC[iterations - i];
workingval ^= roundkey;
workingval ^= alpha;
}
@@ -283,7 +311,9 @@ static uint64_t pauth_computepac(CPUARMState *env, uint64_t data,
uint64_t modifier, ARMPACKey key)
{
if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
- return pauth_computepac_architected(data, modifier, key);
+ return pauth_computepac_architected(data, modifier, key, false);
+ } else if (cpu_isar_feature(aa64_pauth_arch_qarma3, env_archcpu(env))) {
+ return pauth_computepac_architected(data, modifier, key, true);
} else {
return pauth_computepac_impdef(data, modifier, key);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 3/7] target/arm: Implement v8.3 EnhancedPAC
2023-02-22 19:35 [PATCH v2 0/7] Implement Most ARMv8.3 Pointer Authentication Features Aaron Lindsay
2023-02-22 19:35 ` [PATCH v2 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection Aaron Lindsay
2023-02-22 19:35 ` [PATCH v2 2/7] target/arm: Implement v8.3 QARMA3 PAC cipher Aaron Lindsay
@ 2023-02-22 19:35 ` Aaron Lindsay
2023-02-22 20:39 ` Richard Henderson
2023-02-22 20:41 ` Richard Henderson
2023-02-22 19:35 ` [PATCH v2 4/7] target/arm: Implement v8.3 Pauth2 Aaron Lindsay
` (3 subsequent siblings)
6 siblings, 2 replies; 22+ messages in thread
From: Aaron Lindsay @ 2023-02-22 19:35 UTC (permalink / raw)
To: qemu-devel, qemu-arm, Richard Henderson, Vincent Dehors,
Alex Bennée, Peter Maydell
Cc: Aaron Lindsay
Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/pauth_helper.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index f525ef7fad..a83956652f 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -347,11 +347,15 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
*/
test = sextract64(ptr, bot_bit, top_bit - bot_bit);
if (test != 0 && test != -1) {
- /*
- * Note that our top_bit is one greater than the pseudocode's
- * version, hence "- 2" here.
- */
- pac ^= MAKE_64BIT_MASK(top_bit - 2, 1);
+ if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
+ pac = 0;
+ } else {
+ /*
+ * Note that our top_bit is one greater than the pseudocode's
+ * version, hence "- 2" here.
+ */
+ pac ^= MAKE_64BIT_MASK(top_bit - 2, 1);
+ }
}
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 3/7] target/arm: Implement v8.3 EnhancedPAC
2023-02-22 19:35 ` [PATCH v2 3/7] target/arm: Implement v8.3 EnhancedPAC Aaron Lindsay
@ 2023-02-22 20:39 ` Richard Henderson
2023-02-22 20:41 ` Richard Henderson
1 sibling, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2023-02-22 20:39 UTC (permalink / raw)
To: Aaron Lindsay, qemu-devel, qemu-arm, Vincent Dehors,
Alex Bennée, Peter Maydell
On 2/22/23 09:35, Aaron Lindsay wrote:
> Signed-off-by: Aaron Lindsay<aaron@os.amperecomputing.com>
> Reviewed-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> target/arm/pauth_helper.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/7] target/arm: Implement v8.3 EnhancedPAC
2023-02-22 19:35 ` [PATCH v2 3/7] target/arm: Implement v8.3 EnhancedPAC Aaron Lindsay
2023-02-22 20:39 ` Richard Henderson
@ 2023-02-22 20:41 ` Richard Henderson
1 sibling, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2023-02-22 20:41 UTC (permalink / raw)
To: Aaron Lindsay, qemu-devel, qemu-arm, Vincent Dehors,
Alex Bennée, Peter Maydell
On 2/22/23 09:35, Aaron Lindsay wrote:
> + if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
It might be cleaner, especially later, to have
ARMCPU *cpu = env_archcpu(env);
at the top of the function.
r~
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 4/7] target/arm: Implement v8.3 Pauth2
2023-02-22 19:35 [PATCH v2 0/7] Implement Most ARMv8.3 Pointer Authentication Features Aaron Lindsay
` (2 preceding siblings ...)
2023-02-22 19:35 ` [PATCH v2 3/7] target/arm: Implement v8.3 EnhancedPAC Aaron Lindsay
@ 2023-02-22 19:35 ` Aaron Lindsay
2023-02-22 20:50 ` Richard Henderson
2023-02-22 19:35 ` [PATCH v2 5/7] targer/arm: Inform helpers whether a PAC instruction is 'combined' Aaron Lindsay
` (2 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Aaron Lindsay @ 2023-02-22 19:35 UTC (permalink / raw)
To: qemu-devel, qemu-arm, Richard Henderson, Vincent Dehors,
Alex Bennée, Peter Maydell
Cc: Aaron Lindsay
Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
target/arm/pauth_helper.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index a83956652f..c4ee040da7 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -347,7 +347,9 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
*/
test = sextract64(ptr, bot_bit, top_bit - bot_bit);
if (test != 0 && test != -1) {
- if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
+ if (cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {
+ /* No action required */
+ } else if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
pac = 0;
} else {
/*
@@ -362,6 +364,9 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
* Preserve the determination between upper and lower at bit 55,
* and insert pointer authentication code.
*/
+ if (cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {
+ pac ^= ptr;
+ }
if (param.tbi) {
ptr &= ~MAKE_64BIT_MASK(bot_bit, 55 - bot_bit + 1);
pac &= MAKE_64BIT_MASK(bot_bit, 54 - bot_bit + 1);
@@ -389,23 +394,30 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
ARMVAParameters param = aa64_va_parameters(env, ptr, mmu_idx, data);
int bot_bit, top_bit;
- uint64_t pac, orig_ptr, test;
+ uint64_t pac, orig_ptr, test, result;
orig_ptr = pauth_original_ptr(ptr, param);
pac = pauth_computepac(env, orig_ptr, modifier, *key);
bot_bit = 64 - param.tsz;
top_bit = 64 - 8 * param.tbi;
- test = (pac ^ ptr) & ~MAKE_64BIT_MASK(55, 1);
- if (unlikely(extract64(test, bot_bit, top_bit - bot_bit))) {
- int error_code = (keynumber << 1) | (keynumber ^ 1);
- if (param.tbi) {
- return deposit64(orig_ptr, 53, 2, error_code);
- } else {
- return deposit64(orig_ptr, 61, 2, error_code);
+ if (cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {
+ uint64_t xor_mask = MAKE_64BIT_MASK(bot_bit, top_bit - bot_bit + 1) &
+ ~MAKE_64BIT_MASK(55, 1);
+ result = ((ptr ^ pac) & xor_mask) | (ptr & ~xor_mask);
+ } else {
+ test = (pac ^ ptr) & ~MAKE_64BIT_MASK(55, 1);
+ if (unlikely(extract64(test, bot_bit, top_bit - bot_bit))) {
+ int error_code = (keynumber << 1) | (keynumber ^ 1);
+ if (param.tbi) {
+ return deposit64(orig_ptr, 53, 2, error_code);
+ } else {
+ return deposit64(orig_ptr, 61, 2, error_code);
+ }
}
+ result = orig_ptr;
}
- return orig_ptr;
+ return result;
}
static uint64_t pauth_strip(CPUARMState *env, uint64_t ptr, bool data)
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 5/7] targer/arm: Inform helpers whether a PAC instruction is 'combined'
2023-02-22 19:35 [PATCH v2 0/7] Implement Most ARMv8.3 Pointer Authentication Features Aaron Lindsay
` (3 preceding siblings ...)
2023-02-22 19:35 ` [PATCH v2 4/7] target/arm: Implement v8.3 Pauth2 Aaron Lindsay
@ 2023-02-22 19:35 ` Aaron Lindsay
2023-02-22 20:56 ` Richard Henderson
2023-02-22 19:35 ` [PATCH v2 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE Aaron Lindsay
2023-02-22 19:35 ` [PATCH v2 7/7] target/arm: Add CPU properties for most v8.3 PAC features Aaron Lindsay
6 siblings, 1 reply; 22+ messages in thread
From: Aaron Lindsay @ 2023-02-22 19:35 UTC (permalink / raw)
To: qemu-devel, qemu-arm, Richard Henderson, Vincent Dehors,
Alex Bennée, Peter Maydell
Cc: Aaron Lindsay
An instruction is a 'combined' Pointer Authentication instruction if it
does something in addition to PAC - for instance, branching to or
loading an address from the authenticated pointer. Knowing whether a PAC
operation is 'combined' is needed to implement the FPACCOMBINE feature
for ARMv8.3.
Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
target/arm/helper-a64.h | 4 +++
target/arm/pauth_helper.c | 71 +++++++++++++++++++++++++++++++-------
target/arm/translate-a64.c | 20 +++++------
3 files changed, 72 insertions(+), 23 deletions(-)
diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h
index 7b706571bb..829aaf4919 100644
--- a/target/arm/helper-a64.h
+++ b/target/arm/helper-a64.h
@@ -98,9 +98,13 @@ DEF_HELPER_FLAGS_3(pacda, TCG_CALL_NO_WG, i64, env, i64, i64)
DEF_HELPER_FLAGS_3(pacdb, TCG_CALL_NO_WG, i64, env, i64, i64)
DEF_HELPER_FLAGS_3(pacga, TCG_CALL_NO_WG, i64, env, i64, i64)
DEF_HELPER_FLAGS_3(autia, TCG_CALL_NO_WG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_3(autia_combined, TCG_CALL_NO_WG, i64, env, i64, i64)
DEF_HELPER_FLAGS_3(autib, TCG_CALL_NO_WG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_3(autib_combined, TCG_CALL_NO_WG, i64, env, i64, i64)
DEF_HELPER_FLAGS_3(autda, TCG_CALL_NO_WG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_3(autda_combined, TCG_CALL_NO_WG, i64, env, i64, i64)
DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_3(autdb_combined, TCG_CALL_NO_WG, i64, env, i64, i64)
DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64)
DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64)
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index c4ee040da7..96770d7860 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -389,7 +389,8 @@ static uint64_t pauth_original_ptr(uint64_t ptr, ARMVAParameters param)
}
static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
- ARMPACKey *key, bool data, int keynumber)
+ ARMPACKey *key, bool data, int keynumber,
+ bool is_combined)
{
ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
ARMVAParameters param = aa64_va_parameters(env, ptr, mmu_idx, data);
@@ -510,44 +511,88 @@ uint64_t HELPER(pacga)(CPUARMState *env, uint64_t x, uint64_t y)
return pac & 0xffffffff00000000ull;
}
-uint64_t HELPER(autia)(CPUARMState *env, uint64_t x, uint64_t y)
+static uint64_t pauth_autia(CPUARMState *env, uint64_t x, uint64_t y,
+ uintptr_t ra, bool is_combined)
{
int el = arm_current_el(env);
if (!pauth_key_enabled(env, el, SCTLR_EnIA)) {
return x;
}
- pauth_check_trap(env, el, GETPC());
- return pauth_auth(env, x, y, &env->keys.apia, false, 0);
+ pauth_check_trap(env, el, ra);
+ return pauth_auth(env, x, y, &env->keys.apia, false, 0, is_combined);
}
-uint64_t HELPER(autib)(CPUARMState *env, uint64_t x, uint64_t y)
+uint64_t HELPER(autia)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+ return pauth_autia(env, x, y, GETPC(), false);
+}
+
+uint64_t HELPER(autia_combined)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+ return pauth_autia(env, x, y, GETPC(), true);
+}
+
+static uint64_t pauth_autib(CPUARMState *env, uint64_t x, uint64_t y,
+ uintptr_t ra, bool is_combined)
{
int el = arm_current_el(env);
if (!pauth_key_enabled(env, el, SCTLR_EnIB)) {
return x;
}
- pauth_check_trap(env, el, GETPC());
- return pauth_auth(env, x, y, &env->keys.apib, false, 1);
+ pauth_check_trap(env, el, ra);
+ return pauth_auth(env, x, y, &env->keys.apib, false, 1, is_combined);
}
-uint64_t HELPER(autda)(CPUARMState *env, uint64_t x, uint64_t y)
+uint64_t HELPER(autib)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+ return pauth_autib(env, x, y, GETPC(), false);
+}
+
+uint64_t HELPER(autib_combined)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+ return pauth_autib(env, x, y, GETPC(), true);
+}
+
+static uint64_t pauth_autda(CPUARMState *env, uint64_t x, uint64_t y,
+ uintptr_t ra, bool is_combined)
{
int el = arm_current_el(env);
if (!pauth_key_enabled(env, el, SCTLR_EnDA)) {
return x;
}
- pauth_check_trap(env, el, GETPC());
- return pauth_auth(env, x, y, &env->keys.apda, true, 0);
+ pauth_check_trap(env, el, ra);
+ return pauth_auth(env, x, y, &env->keys.apda, true, 0, is_combined);
}
-uint64_t HELPER(autdb)(CPUARMState *env, uint64_t x, uint64_t y)
+uint64_t HELPER(autda)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+ return pauth_autda(env, x, y, GETPC(), false);
+}
+
+uint64_t HELPER(autda_combined)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+ return pauth_autda(env, x, y, GETPC(), true);
+}
+
+static uint64_t pauth_autdb(CPUARMState *env, uint64_t x, uint64_t y,
+ uintptr_t ra, bool is_combined)
{
int el = arm_current_el(env);
if (!pauth_key_enabled(env, el, SCTLR_EnDB)) {
return x;
}
- pauth_check_trap(env, el, GETPC());
- return pauth_auth(env, x, y, &env->keys.apdb, true, 1);
+ pauth_check_trap(env, el, ra);
+ return pauth_auth(env, x, y, &env->keys.apdb, true, 1, is_combined);
+}
+
+uint64_t HELPER(autdb)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+ return pauth_autdb(env, x, y, GETPC(), false);
+}
+
+uint64_t HELPER(autdb_combined)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+ return pauth_autdb(env, x, y, GETPC(), true);
}
uint64_t HELPER(xpaci)(CPUARMState *env, uint64_t a)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 52b1b8a1f0..37cccfda8a 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2318,9 +2318,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
if (s->pauth_active) {
dst = new_tmp_a64(s);
if (op3 == 2) {
- gen_helper_autia(dst, cpu_env, cpu_reg(s, rn), modifier);
+ gen_helper_autia_combined(dst, cpu_env, cpu_reg(s, rn), modifier);
} else {
- gen_helper_autib(dst, cpu_env, cpu_reg(s, rn), modifier);
+ gen_helper_autib_combined(dst, cpu_env, cpu_reg(s, rn), modifier);
}
} else {
dst = cpu_reg(s, rn);
@@ -2356,9 +2356,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
dst = new_tmp_a64(s);
modifier = cpu_reg_sp(s, op4);
if (op3 == 2) {
- gen_helper_autia(dst, cpu_env, cpu_reg(s, rn), modifier);
+ gen_helper_autia_combined(dst, cpu_env, cpu_reg(s, rn), modifier);
} else {
- gen_helper_autib(dst, cpu_env, cpu_reg(s, rn), modifier);
+ gen_helper_autib_combined(dst, cpu_env, cpu_reg(s, rn), modifier);
}
} else {
dst = cpu_reg(s, rn);
@@ -2404,9 +2404,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
if (s->pauth_active) {
modifier = cpu_X[31];
if (op3 == 2) {
- gen_helper_autia(dst, cpu_env, dst, modifier);
+ gen_helper_autia_combined(dst, cpu_env, dst, modifier);
} else {
- gen_helper_autib(dst, cpu_env, dst, modifier);
+ gen_helper_autib_combined(dst, cpu_env, dst, modifier);
}
}
break;
@@ -3583,11 +3583,11 @@ static void disas_ldst_pac(DisasContext *s, uint32_t insn,
if (s->pauth_active) {
if (use_key_a) {
- gen_helper_autda(dirty_addr, cpu_env, dirty_addr,
- new_tmp_a64_zero(s));
+ gen_helper_autda_combined(dirty_addr, cpu_env, dirty_addr,
+ new_tmp_a64_zero(s));
} else {
- gen_helper_autdb(dirty_addr, cpu_env, dirty_addr,
- new_tmp_a64_zero(s));
+ gen_helper_autdb_combined(dirty_addr, cpu_env, dirty_addr,
+ new_tmp_a64_zero(s));
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/7] targer/arm: Inform helpers whether a PAC instruction is 'combined'
2023-02-22 19:35 ` [PATCH v2 5/7] targer/arm: Inform helpers whether a PAC instruction is 'combined' Aaron Lindsay
@ 2023-02-22 20:56 ` Richard Henderson
0 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2023-02-22 20:56 UTC (permalink / raw)
To: Aaron Lindsay, qemu-devel, qemu-arm, Vincent Dehors,
Alex Bennée, Peter Maydell
On 2/22/23 09:35, Aaron Lindsay wrote:
> static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
> - ARMPACKey *key, bool data, int keynumber)
> + ARMPACKey *key, bool data, int keynumber,
> + bool is_combined)
Add 'ra' argument here at the same time, to avoid modifying calls to this function in
successive patches.
Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE
2023-02-22 19:35 [PATCH v2 0/7] Implement Most ARMv8.3 Pointer Authentication Features Aaron Lindsay
` (4 preceding siblings ...)
2023-02-22 19:35 ` [PATCH v2 5/7] targer/arm: Inform helpers whether a PAC instruction is 'combined' Aaron Lindsay
@ 2023-02-22 19:35 ` Aaron Lindsay
2023-02-22 21:37 ` Richard Henderson
2023-02-22 19:35 ` [PATCH v2 7/7] target/arm: Add CPU properties for most v8.3 PAC features Aaron Lindsay
6 siblings, 1 reply; 22+ messages in thread
From: Aaron Lindsay @ 2023-02-22 19:35 UTC (permalink / raw)
To: qemu-devel, qemu-arm, Richard Henderson, Vincent Dehors,
Alex Bennée, Peter Maydell
Cc: Aaron Lindsay
Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
target/arm/pauth_helper.c | 35 ++++++++++++++++++++++++++++++-----
target/arm/syndrome.h | 7 +++++++
2 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index 96770d7860..db6cf9b5bc 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -388,9 +388,24 @@ static uint64_t pauth_original_ptr(uint64_t ptr, ARMVAParameters param)
return deposit64(ptr, bot_pac_bit, top_pac_bit - bot_pac_bit, extfield);
}
+static G_NORETURN
+void pauth_fail_exception(CPUARMState *env, bool data, int keynumber, uintptr_t ra)
+{
+ int target_el = arm_current_el(env);
+ if (target_el == 0) {
+ uint64_t hcr = arm_hcr_el2_eff(env);
+ if (arm_is_el2_enabled(env) && (hcr & HCR_TGE))
+ target_el = 2;
+ else
+ target_el = 1;
+ }
+
+ raise_exception_ra(env, EXCP_UDEF, syn_pacfail(data, keynumber), target_el, ra);
+}
+
static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
ARMPACKey *key, bool data, int keynumber,
- bool is_combined)
+ uintptr_t ra, bool is_combined)
{
ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
ARMVAParameters param = aa64_va_parameters(env, ptr, mmu_idx, data);
@@ -406,6 +421,16 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
uint64_t xor_mask = MAKE_64BIT_MASK(bot_bit, top_bit - bot_bit + 1) &
~MAKE_64BIT_MASK(55, 1);
result = ((ptr ^ pac) & xor_mask) | (ptr & ~xor_mask);
+ if (cpu_isar_feature(aa64_fpac_combine, env_archcpu(env)) ||
+ (cpu_isar_feature(aa64_fpac, env_archcpu(env)) &&
+ !is_combined)) {
+ int fpac_top = param.tbi ? 55 : 64;
+ uint64_t fpac_mask = MAKE_64BIT_MASK(bot_bit, fpac_top - bot_bit);
+ test = (result ^ sextract64(result, 55, 1)) & fpac_mask;
+ if (unlikely(test)) {
+ pauth_fail_exception(env, data, keynumber, ra);
+ }
+ }
} else {
test = (pac ^ ptr) & ~MAKE_64BIT_MASK(55, 1);
if (unlikely(extract64(test, bot_bit, top_bit - bot_bit))) {
@@ -519,7 +544,7 @@ static uint64_t pauth_autia(CPUARMState *env, uint64_t x, uint64_t y,
return x;
}
pauth_check_trap(env, el, ra);
- return pauth_auth(env, x, y, &env->keys.apia, false, 0, is_combined);
+ return pauth_auth(env, x, y, &env->keys.apia, false, 0, ra, is_combined);
}
uint64_t HELPER(autia)(CPUARMState *env, uint64_t x, uint64_t y)
@@ -540,7 +565,7 @@ static uint64_t pauth_autib(CPUARMState *env, uint64_t x, uint64_t y,
return x;
}
pauth_check_trap(env, el, ra);
- return pauth_auth(env, x, y, &env->keys.apib, false, 1, is_combined);
+ return pauth_auth(env, x, y, &env->keys.apib, false, 1, ra, is_combined);
}
uint64_t HELPER(autib)(CPUARMState *env, uint64_t x, uint64_t y)
@@ -561,7 +586,7 @@ static uint64_t pauth_autda(CPUARMState *env, uint64_t x, uint64_t y,
return x;
}
pauth_check_trap(env, el, ra);
- return pauth_auth(env, x, y, &env->keys.apda, true, 0, is_combined);
+ return pauth_auth(env, x, y, &env->keys.apda, true, 0, ra, is_combined);
}
uint64_t HELPER(autda)(CPUARMState *env, uint64_t x, uint64_t y)
@@ -582,7 +607,7 @@ static uint64_t pauth_autdb(CPUARMState *env, uint64_t x, uint64_t y,
return x;
}
pauth_check_trap(env, el, ra);
- return pauth_auth(env, x, y, &env->keys.apdb, true, 1, is_combined);
+ return pauth_auth(env, x, y, &env->keys.apdb, true, 1, ra, is_combined);
}
uint64_t HELPER(autdb)(CPUARMState *env, uint64_t x, uint64_t y)
diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
index 73df5e3793..99ed4c7d3d 100644
--- a/target/arm/syndrome.h
+++ b/target/arm/syndrome.h
@@ -48,6 +48,7 @@ enum arm_exception_class {
EC_AA64_SMC = 0x17,
EC_SYSTEMREGISTERTRAP = 0x18,
EC_SVEACCESSTRAP = 0x19,
+ EC_PACFAIL = 0x1c,
EC_SMETRAP = 0x1d,
EC_INSNABORT = 0x20,
EC_INSNABORT_SAME_EL = 0x21,
@@ -221,6 +222,12 @@ static inline uint32_t syn_smetrap(SMEExceptionType etype, bool is_16bit)
| (is_16bit ? 0 : ARM_EL_IL) | etype;
}
+static inline uint32_t syn_pacfail(bool data, int keynumber)
+{
+ int error_code = ((data ? 1 : 0) << 1) | (keynumber);
+ return (EC_PACFAIL << ARM_EL_EC_SHIFT) | ARM_EL_IL | error_code;
+}
+
static inline uint32_t syn_pactrap(void)
{
return EC_PACTRAP << ARM_EL_EC_SHIFT;
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE
2023-02-22 19:35 ` [PATCH v2 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE Aaron Lindsay
@ 2023-02-22 21:37 ` Richard Henderson
2023-03-22 20:33 ` Aaron Lindsay
0 siblings, 1 reply; 22+ messages in thread
From: Richard Henderson @ 2023-02-22 21:37 UTC (permalink / raw)
To: Aaron Lindsay, qemu-devel, qemu-arm, Vincent Dehors,
Alex Bennée, Peter Maydell
On 2/22/23 09:35, Aaron Lindsay wrote:
> +static G_NORETURN
> +void pauth_fail_exception(CPUARMState *env, bool data, int keynumber, uintptr_t ra)
> +{
> + int target_el = arm_current_el(env);
> + if (target_el == 0) {
> + uint64_t hcr = arm_hcr_el2_eff(env);
> + if (arm_is_el2_enabled(env) && (hcr & HCR_TGE))
> + target_el = 2;
> + else
> + target_el = 1;
> + }
> +
> + raise_exception_ra(env, EXCP_UDEF, syn_pacfail(data, keynumber), target_el, ra);
Use exception_target_el(), no need to check TGE here.
> @@ -406,6 +421,16 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
> uint64_t xor_mask = MAKE_64BIT_MASK(bot_bit, top_bit - bot_bit + 1) &
> ~MAKE_64BIT_MASK(55, 1);
> result = ((ptr ^ pac) & xor_mask) | (ptr & ~xor_mask);
> + if (cpu_isar_feature(aa64_fpac_combine, env_archcpu(env)) ||
> + (cpu_isar_feature(aa64_fpac, env_archcpu(env)) &&
> + !is_combined)) {
Indentation is off.
> + int error_code = ((data ? 1 : 0) << 1) | (keynumber);
'? 1 : 0' is not required.
r~
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE
2023-02-22 21:37 ` Richard Henderson
@ 2023-03-22 20:33 ` Aaron Lindsay
2023-03-22 22:39 ` Richard Henderson
0 siblings, 1 reply; 22+ messages in thread
From: Aaron Lindsay @ 2023-03-22 20:33 UTC (permalink / raw)
To: Richard Henderson
Cc: qemu-devel, qemu-arm, Vincent Dehors, Alex Bennée,
Peter Maydell
On Feb 22 11:37, Richard Henderson wrote:
> On 2/22/23 09:35, Aaron Lindsay wrote:
> > @@ -406,6 +421,16 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
> > uint64_t xor_mask = MAKE_64BIT_MASK(bot_bit, top_bit - bot_bit + 1) &
> > ~MAKE_64BIT_MASK(55, 1);
> > result = ((ptr ^ pac) & xor_mask) | (ptr & ~xor_mask);
> > + if (cpu_isar_feature(aa64_fpac_combine, env_archcpu(env)) ||
> > + (cpu_isar_feature(aa64_fpac, env_archcpu(env)) &&
> > + !is_combined)) {
>
> Indentation is off.
I pulled `env_archcpu(env)` out of this if-statement in my latest
patchset in addition to the indentation, but am not confident I have
done what you intended. The QEMU Coding Style guide doesn't seem to
address longer statements like this in its section on indentation, so I
attempted to follow other examples in the code, but I'll take further
direction here.
-Aaron
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE
2023-03-22 20:33 ` Aaron Lindsay
@ 2023-03-22 22:39 ` Richard Henderson
0 siblings, 0 replies; 22+ messages in thread
From: Richard Henderson @ 2023-03-22 22:39 UTC (permalink / raw)
To: Aaron Lindsay
Cc: qemu-devel, qemu-arm, Vincent Dehors, Alex Bennée,
Peter Maydell
On 3/22/23 13:33, Aaron Lindsay wrote:
> On Feb 22 11:37, Richard Henderson wrote:
>> On 2/22/23 09:35, Aaron Lindsay wrote:
>>> @@ -406,6 +421,16 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>>> uint64_t xor_mask = MAKE_64BIT_MASK(bot_bit, top_bit - bot_bit + 1) &
>>> ~MAKE_64BIT_MASK(55, 1);
>>> result = ((ptr ^ pac) & xor_mask) | (ptr & ~xor_mask);
>>> + if (cpu_isar_feature(aa64_fpac_combine, env_archcpu(env)) ||
>>> + (cpu_isar_feature(aa64_fpac, env_archcpu(env)) &&
>>> + !is_combined)) {
>>
>> Indentation is off.
>
> I pulled `env_archcpu(env)` out of this if-statement in my latest
> patchset in addition to the indentation, but am not confident I have
> done what you intended. The QEMU Coding Style guide doesn't seem to
> address longer statements like this in its section on indentation, so I
> attempted to follow other examples in the code, but I'll take further
> direction here.
if (function(a) ||
(function(b) &&
function(c))) {
...
1234567890
r~
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 7/7] target/arm: Add CPU properties for most v8.3 PAC features
2023-02-22 19:35 [PATCH v2 0/7] Implement Most ARMv8.3 Pointer Authentication Features Aaron Lindsay
` (5 preceding siblings ...)
2023-02-22 19:35 ` [PATCH v2 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE Aaron Lindsay
@ 2023-02-22 19:35 ` Aaron Lindsay
2023-02-22 22:14 ` Richard Henderson
6 siblings, 1 reply; 22+ messages in thread
From: Aaron Lindsay @ 2023-02-22 19:35 UTC (permalink / raw)
To: qemu-devel, qemu-arm, Richard Henderson, Vincent Dehors,
Alex Bennée, Peter Maydell
Cc: Aaron Lindsay
Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
target/arm/cpu.h | 5 +++
target/arm/cpu64.c | 81 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 72 insertions(+), 14 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9c3cbc9a29..40b4631f11 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1039,6 +1039,11 @@ struct ArchCPU {
*/
bool prop_pauth;
bool prop_pauth_impdef;
+ bool prop_pauth_qarma3;
+ bool prop_pauth_epac;
+ bool prop_pauth2; // also known as EnhancedPAC2/EPAC2
+ bool prop_pauth_fpac;
+ bool prop_pauth_fpac_combine;
bool prop_lpa2;
/* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 0e021960fb..315acabbe2 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -590,8 +590,7 @@ static void aarch64_add_sme_properties(Object *obj)
void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
{
- int arch_val = 0, impdef_val = 0;
- uint64_t t;
+ int address_auth = 0, generic_auth = 0;
/* Exit early if PAuth is enabled, and fall through to disable it */
if ((kvm_enabled() || hvf_enabled()) && cpu->prop_pauth) {
@@ -603,30 +602,79 @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
return;
}
- /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
+ if (cpu->prop_pauth_epac &&
+ (cpu->prop_pauth2 ||
+ cpu->prop_pauth_fpac ||
+ cpu->prop_pauth_fpac_combine)) {
+ error_setg(errp, "'pauth-epac' feature not compatible with any of "
+ "'pauth-2', 'pauth-fpac', or 'pauth-fpac-combine'");
+ return;
+ }
+
+ /* Determine the PAC features independently of the algorithm */
+ if (cpu->prop_pauth_fpac_combine) {
+ address_auth = 0b0101;
+ } else if (cpu->prop_pauth_fpac) {
+ address_auth = 0b0100;
+ } else if (cpu->prop_pauth2) {
+ address_auth = 0b0011;
+ } else if (cpu->prop_pauth_epac) {
+ address_auth = 0b0010;
+ }
+
+ /* Write the features into the correct field for the algorithm in use */
if (cpu->prop_pauth) {
+ uint64_t t;
+
+ if (cpu->prop_pauth_impdef && cpu->prop_pauth_qarma3) {
+ error_setg(errp, "Cannot set both qarma3 ('pauth-qarma3') and "
+ "impdef ('pauth-impdef') pointer authentication ciphers");
+ return;
+ }
+
+ if (address_auth == 0)
+ address_auth = 0b0001;
+ generic_auth = 1;
+
if (cpu->prop_pauth_impdef) {
- impdef_val = 1;
+ t = cpu->isar.id_aa64isar1;
+ t = FIELD_DP64(t, ID_AA64ISAR1, API, address_auth);
+ t = FIELD_DP64(t, ID_AA64ISAR1, GPI, generic_auth);
+ cpu->isar.id_aa64isar1 = t;
+ } else if (cpu->prop_pauth_qarma3) {
+ t = cpu->isar.id_aa64isar2;
+ t = FIELD_DP64(t, ID_AA64ISAR2, APA3, address_auth);
+ t = FIELD_DP64(t, ID_AA64ISAR2, GPA3, generic_auth);
+ cpu->isar.id_aa64isar2 = t;
} else {
- arch_val = 1;
+ t = cpu->isar.id_aa64isar1;
+ t = FIELD_DP64(t, ID_AA64ISAR1, APA, address_auth);
+ t = FIELD_DP64(t, ID_AA64ISAR1, GPA, generic_auth);
+ cpu->isar.id_aa64isar1 = t;
}
- } else if (cpu->prop_pauth_impdef) {
- error_setg(errp, "cannot enable pauth-impdef without pauth");
+ } else if (cpu->prop_pauth_impdef || cpu->prop_pauth_qarma3) {
+ error_setg(errp, "cannot enable pauth-impdef or pauth-qarma3 without pauth");
+ error_append_hint(errp, "Add pauth=on to the CPU property list.\n");
+ } else if (address_auth != 0) {
+ error_setg(errp, "cannot enable any pauth* features 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);
- cpu->isar.id_aa64isar1 = t;
}
static Property arm_cpu_pauth_property =
DEFINE_PROP_BOOL("pauth", ARMCPU, prop_pauth, true);
static Property arm_cpu_pauth_impdef_property =
DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false);
+static Property arm_cpu_pauth_qarma3_property =
+ DEFINE_PROP_BOOL("pauth-qarma3", ARMCPU, prop_pauth_qarma3, false);
+static Property arm_cpu_pauth_epac_property =
+ DEFINE_PROP_BOOL("pauth-epac", ARMCPU, prop_pauth_epac, false);
+static Property arm_cpu_pauth2_property =
+ DEFINE_PROP_BOOL("pauth2", ARMCPU, prop_pauth2, false);
+static Property arm_cpu_pauth_fpac_property =
+ DEFINE_PROP_BOOL("pauth-fpac", ARMCPU, prop_pauth_fpac, false);
+static Property arm_cpu_pauth_fpac_combine_property =
+ DEFINE_PROP_BOOL("pauth-fpac-combine", ARMCPU, prop_pauth_fpac_combine, false);
static void aarch64_add_pauth_properties(Object *obj)
{
@@ -646,6 +694,11 @@ static void aarch64_add_pauth_properties(Object *obj)
cpu->prop_pauth = cpu_isar_feature(aa64_pauth, cpu);
} else {
qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
+ qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_qarma3_property);
+ qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_epac_property);
+ qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth2_property);
+ qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_fpac_property);
+ qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_fpac_combine_property);
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/7] target/arm: Add CPU properties for most v8.3 PAC features
2023-02-22 19:35 ` [PATCH v2 7/7] target/arm: Add CPU properties for most v8.3 PAC features Aaron Lindsay
@ 2023-02-22 22:14 ` Richard Henderson
2023-02-24 11:23 ` Peter Maydell
2023-03-22 20:36 ` Aaron Lindsay
0 siblings, 2 replies; 22+ messages in thread
From: Richard Henderson @ 2023-02-22 22:14 UTC (permalink / raw)
To: Aaron Lindsay, qemu-devel, qemu-arm, Vincent Dehors,
Alex Bennée, Peter Maydell
On 2/22/23 09:35, Aaron Lindsay wrote:
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
> target/arm/cpu.h | 5 +++
> target/arm/cpu64.c | 81 ++++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 72 insertions(+), 14 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9c3cbc9a29..40b4631f11 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1039,6 +1039,11 @@ struct ArchCPU {
> */
> bool prop_pauth;
> bool prop_pauth_impdef;
> + bool prop_pauth_qarma3;
> + bool prop_pauth_epac;
> + bool prop_pauth2; // also known as EnhancedPAC2/EPAC2
No c++ comments.
> + if (cpu->prop_pauth_epac &&
> + (cpu->prop_pauth2 ||
> + cpu->prop_pauth_fpac ||
> + cpu->prop_pauth_fpac_combine)) {
Indentation.
> + if (address_auth == 0)
> + address_auth = 0b0001;
Missing braces.
> +static Property arm_cpu_pauth2_property =
> + DEFINE_PROP_BOOL("pauth2", ARMCPU, prop_pauth2, false);
> +static Property arm_cpu_pauth_fpac_property =
> + DEFINE_PROP_BOOL("pauth-fpac", ARMCPU, prop_pauth_fpac, false);
> +static Property arm_cpu_pauth_fpac_combine_property =
> + DEFINE_PROP_BOOL("pauth-fpac-combine", ARMCPU, prop_pauth_fpac_combine, false);
For -cpu max, I would expect these to default on.
Or perhaps not expose these or epac as properties at all.
> @@ -646,6 +694,11 @@ static void aarch64_add_pauth_properties(Object *obj)
> cpu->prop_pauth = cpu_isar_feature(aa64_pauth, cpu);
> } else {
> qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
> + qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_qarma3_property);
> + qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_epac_property);
> + qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth2_property);
> + qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_fpac_property);
> + qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_fpac_combine_property);
I think the *only* property that makes sense for KVM is pauth=on/off, which controls if
KVM exposes the key registers at all (and if off, APA/GPA/etc all get zeroed). There is
certainly no way to adjust the algorithm exposed by the hardware.
The primary reason we have a property for pauth at all is speed of emulation. When we
first enabled qarma5, we saw a major slowdown, with pauth_computepac consuming nearly 50%
of the entire runtime. Later we added impdef, as a way of doing *some* testing of pauth
without the extreme overhead of qarma5.
I see that qarma3 does about half the work of qarma5, so it would be interesting to
measure the relative speed of the 3 implementations on a boot of kernel + selftests.
You may want to look a the code generated and play with flatten and noinline attributes
around pauth_computepac and subroutines. E.g.
static uint64_t __attribute__((flatten, noinline))
pauth_computepac_qarma5(uint64_t data, uint64_t modifier, ARMPACKey key)
{
return pauth_computepac_architected(data, modifier, key, false);
}
static uint64_t __attribute__((flatten, noinline))
pauth_computepac_qarma3(uint64_t data, uint64_t modifier, ARMPACKey key)
{
return pauth_computepac_architected(data, modifier, key, true);
}
static uint64_t __attribute__((flatten, noinline))
pauth_computepac_impdef(uint64_t data, uint64_t modifier, ARMPACKey key)
{
return qemu_xxhash64_4(data, modifier, key.lo, key.hi);
}
static uint64_t pauth_computepac(CPUARMState *env, uint64_t data,
uint64_t modifier, ARMPACKey key)
{
if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
return pauth_computepac_qarma5(data, modifier, key);
} else if (cpu_isar_feature(aa64_pauth_arch_qarma3, env_archcpu(env))) {
return pauth_computepac_qarma3(data, modifier, key);
} else {
return pauth_computepac_impdef(data, modifier, key);
}
}
r~
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/7] target/arm: Add CPU properties for most v8.3 PAC features
2023-02-22 22:14 ` Richard Henderson
@ 2023-02-24 11:23 ` Peter Maydell
2023-03-22 20:36 ` Aaron Lindsay
1 sibling, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2023-02-24 11:23 UTC (permalink / raw)
To: Richard Henderson
Cc: Aaron Lindsay, qemu-devel, qemu-arm, Vincent Dehors,
Alex Bennée
On Wed, 22 Feb 2023 at 22:14, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/22/23 09:35, Aaron Lindsay wrote:
> > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> > ---
> > +static Property arm_cpu_pauth2_property =
> > + DEFINE_PROP_BOOL("pauth2", ARMCPU, prop_pauth2, false);
> > +static Property arm_cpu_pauth_fpac_property =
> > + DEFINE_PROP_BOOL("pauth-fpac", ARMCPU, prop_pauth_fpac, false);
> > +static Property arm_cpu_pauth_fpac_combine_property =
> > + DEFINE_PROP_BOOL("pauth-fpac-combine", ARMCPU, prop_pauth_fpac_combine, false);
>
> For -cpu max, I would expect these to default on.
> Or perhaps not expose these or epac as properties at all.
Yes; unless there's a reason why we want the properties, we
shouldn't bother defining them. As Richard says, having
a 'pauth' property is a special case where we needed to be
able to avoid the massive extra emulation overhead of doing
it per the architected algorithm.
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v2 7/7] target/arm: Add CPU properties for most v8.3 PAC features
2023-02-22 22:14 ` Richard Henderson
2023-02-24 11:23 ` Peter Maydell
@ 2023-03-22 20:36 ` Aaron Lindsay
2023-03-22 23:01 ` Richard Henderson
1 sibling, 1 reply; 22+ messages in thread
From: Aaron Lindsay @ 2023-03-22 20:36 UTC (permalink / raw)
To: Richard Henderson
Cc: qemu-devel, qemu-arm, Vincent Dehors, Alex Bennée,
Peter Maydell
On Feb 22 12:14, Richard Henderson wrote:
> On 2/22/23 09:35, Aaron Lindsay wrote:
> > +static Property arm_cpu_pauth2_property =
> > + DEFINE_PROP_BOOL("pauth2", ARMCPU, prop_pauth2, false);
> > +static Property arm_cpu_pauth_fpac_property =
> > + DEFINE_PROP_BOOL("pauth-fpac", ARMCPU, prop_pauth_fpac, false);
> > +static Property arm_cpu_pauth_fpac_combine_property =
> > + DEFINE_PROP_BOOL("pauth-fpac-combine", ARMCPU, prop_pauth_fpac_combine, false);
>
> For -cpu max, I would expect these to default on.
> Or perhaps not expose these or epac as properties at all.
I've removed these properties, and epac's as well. It now defaults to
the equivalent of prop_pauth_fpac_combine==true in my previous patch.
> I see that qarma3 does about half the work of qarma5, so it would be
> interesting to measure the relative speed of the 3 implementations on a boot
> of kernel + selftests.
>
> You may want to look a the code generated and play with flatten and noinline
> attributes around pauth_computepac and subroutines. E.g.
>
> static uint64_t __attribute__((flatten, noinline))
> pauth_computepac_qarma5(uint64_t data, uint64_t modifier, ARMPACKey key)
> {
> return pauth_computepac_architected(data, modifier, key, false);
> }
>
> static uint64_t __attribute__((flatten, noinline))
> pauth_computepac_qarma3(uint64_t data, uint64_t modifier, ARMPACKey key)
> {
> return pauth_computepac_architected(data, modifier, key, true);
> }
>
> static uint64_t __attribute__((flatten, noinline))
> pauth_computepac_impdef(uint64_t data, uint64_t modifier, ARMPACKey key)
> {
> return qemu_xxhash64_4(data, modifier, key.lo, key.hi);
> }
>
> static uint64_t pauth_computepac(CPUARMState *env, uint64_t data,
> uint64_t modifier, ARMPACKey key)
> {
> if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
> return pauth_computepac_qarma5(data, modifier, key);
> } else if (cpu_isar_feature(aa64_pauth_arch_qarma3, env_archcpu(env))) {
> return pauth_computepac_qarma3(data, modifier, key);
> } else {
> return pauth_computepac_impdef(data, modifier, key);
> }
> }
I have not played around with this further. Do you feel this is
important to look into prior to merging this patchset (since QARMA3
isn't the default)?
-Aaron
^ permalink raw reply [flat|nested] 22+ messages in thread