| Message ID | 20230222193544.3392713-8-aaron@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement Most ARMv8.3 Pointer Authentication Features | expand |
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~
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
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
On 3/22/23 13:36, Aaron Lindsay wrote: > 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)? No, a mere curiosity. r~
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); } }
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(-)