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~
