| Message ID | 20200812065339.2030527-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement an IMPDEF pauth algorithm | expand |
On Tue, Aug 11, 2020 at 11:53:38PM -0700, Richard Henderson wrote: > The crypto overhead of emulating pauth can be significant for > some workloads. Add an enumeration property that allows the > feature to be turned off, on with the architected algorithm, > or on with an implementation defined algorithm. > > The architected algorithm is quite expensive to emulate; > using another algorithm may allow hardware acceleration. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu64.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 64 insertions(+) > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > index dd696183df..3181d0e2f8 100644 > --- a/target/arm/cpu64.c > +++ b/target/arm/cpu64.c > @@ -572,6 +572,69 @@ void aarch64_add_sve_properties(Object *obj) > } > } > > +static const char * const pauth_names[] = { > + "off", "impdef", "arch" > +}; Hi Richard, Please add three boolean properties, rather than one enum: pauth: enable support of the pauth feature pauth-fast: enable QEMU's fast non-cryptographic hash for pauth (pauth must be enabled) pauth-arch: enable the architected algorithm for pauth (pauth must be enabled) These booleans can then be added to the cpu feature probing list used by qmp_query_cpu_model_expansion() > + > +static const QEnumLookup pauth_lookup = { > + .array = pauth_names, > + .size = ARRAY_SIZE(pauth_names) > +}; > + > +static int cpu_arm_get_pauth(Object *obj, Error **errp) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + int value; > + > + /* We will always set GPA+APA and GPI+API to the same value. */ > + if (FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, APA)) { > + value = 2; > + } else if (FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, API)) { > + value = 1; > + } else { > + value = 0; > + } > + return value; > +} > + > +static void cpu_arm_set_pauth(Object *obj, int value, Error **errp) > +{ > + ARMCPU *cpu = ARM_CPU(obj); > + uint64_t t; > + > + /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */ > + t = cpu->isar.id_aa64isar1; > + switch (value) { > + case 0: > + t = FIELD_DP64(t, ID_AA64ISAR1, APA, 0); > + t = FIELD_DP64(t, ID_AA64ISAR1, API, 0); > + t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 0); > + t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0); > + break; > + case 1: > + t = FIELD_DP64(t, ID_AA64ISAR1, APA, 0); > + t = FIELD_DP64(t, ID_AA64ISAR1, API, 1); > + t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 0); > + t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 1); > + break; > + case 2: > + t = FIELD_DP64(t, ID_AA64ISAR1, APA, 1); > + t = FIELD_DP64(t, ID_AA64ISAR1, API, 0); > + t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 1); > + t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0); > + break; > + default: > + g_assert_not_reached(); > + } > + cpu->isar.id_aa64isar1 = t; > +} > + > +static void aarch64_add_pauth_properties(Object *obj) > +{ > + object_property_add_enum(obj, "pauth", "ARMPauthKind", &pauth_lookup, > + cpu_arm_get_pauth, cpu_arm_set_pauth); > +} > + > /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host); > * otherwise, a CPU with as many features enabled as our emulation supports. > * The version of '-cpu max' for qemu-system-arm is defined in cpu.c; > @@ -720,6 +783,7 @@ static void aarch64_max_initfn(Object *obj) > #endif > } > > + aarch64_add_pauth_properties(obj); We don't yet support enabling pauth on KVM accelerated guests, so this call should be in the !kvm_enabled() part of this function. > aarch64_add_sve_properties(obj); > object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq, > cpu_max_set_sve_max_vq, NULL, NULL); > -- > 2.25.1 > > Thanks, drew
On 8/12/20 4:00 AM, Andrew Jones wrote: > On Tue, Aug 11, 2020 at 11:53:38PM -0700, Richard Henderson wrote: >> The crypto overhead of emulating pauth can be significant for >> some workloads. Add an enumeration property that allows the >> feature to be turned off, on with the architected algorithm, >> or on with an implementation defined algorithm. >> >> The architected algorithm is quite expensive to emulate; >> using another algorithm may allow hardware acceleration. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/arm/cpu64.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 64 insertions(+) >> >> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c >> index dd696183df..3181d0e2f8 100644 >> --- a/target/arm/cpu64.c >> +++ b/target/arm/cpu64.c >> @@ -572,6 +572,69 @@ void aarch64_add_sve_properties(Object *obj) >> } >> } >> >> +static const char * const pauth_names[] = { >> + "off", "impdef", "arch" >> +}; > > Hi Richard, > > Please add three boolean properties, rather than one enum: > > pauth: enable support of the pauth feature > pauth-fast: enable QEMU's fast non-cryptographic hash for pauth > (pauth must be enabled) > pauth-arch: enable the architected algorithm for pauth > (pauth must be enabled) > > These booleans can then be added to the cpu feature probing list used by > qmp_query_cpu_model_expansion() Why are 3 booleans better than one enum? I'd forgotten about qmp_query_cpu_model_expansion(); can it not take anything but booleans? r~
On Wed, Aug 12, 2020 at 08:10:47AM -0700, Richard Henderson wrote: > On 8/12/20 4:00 AM, Andrew Jones wrote: > > On Tue, Aug 11, 2020 at 11:53:38PM -0700, Richard Henderson wrote: > >> The crypto overhead of emulating pauth can be significant for > >> some workloads. Add an enumeration property that allows the > >> feature to be turned off, on with the architected algorithm, > >> or on with an implementation defined algorithm. > >> > >> The architected algorithm is quite expensive to emulate; > >> using another algorithm may allow hardware acceleration. > >> > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >> --- > >> target/arm/cpu64.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 64 insertions(+) > >> > >> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > >> index dd696183df..3181d0e2f8 100644 > >> --- a/target/arm/cpu64.c > >> +++ b/target/arm/cpu64.c > >> @@ -572,6 +572,69 @@ void aarch64_add_sve_properties(Object *obj) > >> } > >> } > >> > >> +static const char * const pauth_names[] = { > >> + "off", "impdef", "arch" > >> +}; > > > > Hi Richard, > > > > Please add three boolean properties, rather than one enum: > > > > pauth: enable support of the pauth feature > > pauth-fast: enable QEMU's fast non-cryptographic hash for pauth > > (pauth must be enabled) > > pauth-arch: enable the architected algorithm for pauth > > (pauth must be enabled) > > > > These booleans can then be added to the cpu feature probing list used by > > qmp_query_cpu_model_expansion() > > Why are 3 booleans better than one enum? > > I'd forgotten about qmp_query_cpu_model_expansion(); can it not take anything > but booleans? Right. The probing works by getting a list of possible CPU features, which are all boolean properties. That way the prober can try enabling/disabling them without having to know about each property's set of valid values. We could implement each as an enum and a second level of probing, but that would complicate the probing, and I'm not sure enums gain us much over multiple properties. In this case, since pauth-fast and pauth-arch are mutually exclusive and we want a pauth=on/off too, then we'll need a finalize function like SVE has in order to support the following selections: # Default (pauth-arch), explicitly selected or not -cpu max[,pauth=on] -cpu max[,pauth=on][,pauth-fast=off],pauth-arch=on # Select pauth-fast -cpu max[,pauth=on][,pauth-arch=off],pauth-fast=on # Disable -cpu max,pauth=off -cpu max[,pauth=off],pauth-arch=off,pauth-fast=off # Mutual exclusion errors -cpu max,pauth=off,pauth-{arch,fast}=on -cpu max,pauth=on,pauth-arch=off,pauth-fast=off -cpu max[,pauth=on],pauth-arch=on,pauth-fast=on # Errors because we don't want to guess what the user means -cpu max[,pauth=on],pauth-arch=off -cpu max[,pauth=on],pauth-fast=off Thanks, drew
On Wed, Aug 12, 2020 at 06:31:11PM +0200, Andrew Jones wrote: > On Wed, Aug 12, 2020 at 08:10:47AM -0700, Richard Henderson wrote: > > On 8/12/20 4:00 AM, Andrew Jones wrote: > > > On Tue, Aug 11, 2020 at 11:53:38PM -0700, Richard Henderson wrote: > > >> The crypto overhead of emulating pauth can be significant for > > >> some workloads. Add an enumeration property that allows the > > >> feature to be turned off, on with the architected algorithm, > > >> or on with an implementation defined algorithm. > > >> > > >> The architected algorithm is quite expensive to emulate; > > >> using another algorithm may allow hardware acceleration. > > >> > > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > >> --- > > >> target/arm/cpu64.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++ > > >> 1 file changed, 64 insertions(+) > > >> > > >> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > > >> index dd696183df..3181d0e2f8 100644 > > >> --- a/target/arm/cpu64.c > > >> +++ b/target/arm/cpu64.c > > >> @@ -572,6 +572,69 @@ void aarch64_add_sve_properties(Object *obj) > > >> } > > >> } > > >> > > >> +static const char * const pauth_names[] = { > > >> + "off", "impdef", "arch" > > >> +}; > > > > > > Hi Richard, > > > > > > Please add three boolean properties, rather than one enum: > > > > > > pauth: enable support of the pauth feature > > > pauth-fast: enable QEMU's fast non-cryptographic hash for pauth > > > (pauth must be enabled) > > > pauth-arch: enable the architected algorithm for pauth > > > (pauth must be enabled) > > > > > > These booleans can then be added to the cpu feature probing list used by > > > qmp_query_cpu_model_expansion() > > > > Why are 3 booleans better than one enum? > > > > I'd forgotten about qmp_query_cpu_model_expansion(); can it not take anything > > but booleans? > > Right. The probing works by getting a list of possible CPU features, which > are all boolean properties. That way the prober can try enabling/disabling > them without having to know about each property's set of valid values. We > could implement each as an enum and a second level of probing, but that > would complicate the probing, and I'm not sure enums gain us much over > multiple properties. > > In this case, since pauth-fast and pauth-arch are mutually exclusive and > we want a pauth=on/off too, then we'll need a finalize function like SVE > has in order to support the following selections: > > # Default (pauth-arch), explicitly selected or not > -cpu max[,pauth=on] > -cpu max[,pauth=on][,pauth-fast=off],pauth-arch=on > > # Select pauth-fast > -cpu max[,pauth=on][,pauth-arch=off],pauth-fast=on > > # Disable > -cpu max,pauth=off > -cpu max[,pauth=off],pauth-arch=off,pauth-fast=off > > # Mutual exclusion errors > -cpu max,pauth=off,pauth-{arch,fast}=on > -cpu max,pauth=on,pauth-arch=off,pauth-fast=off > -cpu max[,pauth=on],pauth-arch=on,pauth-fast=on > > # Errors because we don't want to guess what the user means > -cpu max[,pauth=on],pauth-arch=off > -cpu max[,pauth=on],pauth-fast=off Thinking about this some more, maybe we don't need pauth-arch. If we don't, then it simplifies nicely to # Default (enabled with architected algorithm) -cpu max[,pauth=on][,pauth-fast=off] # Select pauth-fast -cpu max[,pauth=on],pauth-fast=on # Disable -cpu max[,pauth-fast=off],pauth=off # Mutual exclusion errors -cpu max,pauth=off,pauth-fast=on Thanks, drew
On Thu, Aug 13, 2020 at 08:03:21AM +0200, Andrew Jones wrote: > Thinking about this some more, maybe we don't need pauth-arch. > If we don't, then it simplifies nicely to > > # Default (enabled with architected algorithm) > -cpu max[,pauth=on][,pauth-fast=off] > > # Select pauth-fast > -cpu max[,pauth=on],pauth-fast=on One reason that users may wish to choose the IMP-DEF algorithm is for functional testing regardless of speed (since APA+GPA / API+GPI depend on whether the algo is architected or imp-def). Given that, I think that "impdef" is a better option name than "pauth-fast", and speed is a benefit but not the only reason to use the option. How about hacing a 'pauth-algo' option which defaults to architected, e.g. | -cpu max,pauth={on,off},pauth-algo={impdef,architected} ... then in future the 'pauth={on,off}' bit could de extended to cover FPAC version etc independently of the algorithm, but for now that can be a boolean. Thanks, Mark.
On Thu, Aug 13, 2020 at 10:05:04AM +0100, Mark Rutland wrote: > On Thu, Aug 13, 2020 at 08:03:21AM +0200, Andrew Jones wrote: > > Thinking about this some more, maybe we don't need pauth-arch. > > If we don't, then it simplifies nicely to > > > > # Default (enabled with architected algorithm) > > -cpu max[,pauth=on][,pauth-fast=off] > > > > # Select pauth-fast > > -cpu max[,pauth=on],pauth-fast=on > > One reason that users may wish to choose the IMP-DEF algorithm is for > functional testing regardless of speed (since APA+GPA / API+GPI depend > on whether the algo is architected or imp-def). > > Given that, I think that "impdef" is a better option name than > "pauth-fast", and speed is a benefit but not the only reason to use the > option. I was going with pauth-fast because in this case Richard identified a need for a fast version. If we identify another need later, then we may want to add another impdef version, e.g. pauth-foo. Maybe all the impdef versions should have impdef in their name to make that more explicit? pauth-impdef-fast pauth-impdef-foo ... That seems a bit verbose, though, and we can document that pauth-* are impdefs and that the default is architected. > > How about hacing a 'pauth-algo' option which defaults to architected, > e.g. > > | -cpu max,pauth={on,off},pauth-algo={impdef,architected} > > ... then in future the 'pauth={on,off}' bit could de extended to cover > FPAC version etc independently of the algorithm, but for now that can be > a boolean. > Keeping pauth a boolean, but creating a pauth-algo enum doesn't help us much, because probing will only be possible for pauth. The prober could only decide to enable pauth with the default algo or not. We could create a pauth-specific probe, similar to the gic-specific probe, but, IMO, it's really not necessary. If we need multiple pauth-* properties, then we can have them all. It just adds a few more lines of logic to the pauth finalize function. Thanks, drew
On Thu, Aug 13, 2020 at 11:49:07AM +0200, Andrew Jones wrote: > On Thu, Aug 13, 2020 at 10:05:04AM +0100, Mark Rutland wrote: > > On Thu, Aug 13, 2020 at 08:03:21AM +0200, Andrew Jones wrote: > > > Thinking about this some more, maybe we don't need pauth-arch. > > > If we don't, then it simplifies nicely to > > > > > > # Default (enabled with architected algorithm) > > > -cpu max[,pauth=on][,pauth-fast=off] > > > > > > # Select pauth-fast > > > -cpu max[,pauth=on],pauth-fast=on > > > > One reason that users may wish to choose the IMP-DEF algorithm is for > > functional testing regardless of speed (since APA+GPA / API+GPI depend > > on whether the algo is architected or imp-def). > > > > Given that, I think that "impdef" is a better option name than > > "pauth-fast", and speed is a benefit but not the only reason to use the > > option. > > I was going with pauth-fast because in this case Richard identified a > need for a fast version. If we identify another need later, then we may > want to add another impdef version, e.g. pauth-foo. Maybe all the impdef > versions should have impdef in their name to make that more explicit? > > pauth-impdef-fast > pauth-impdef-foo Something like that is fine by me. > > How about hacing a 'pauth-algo' option which defaults to architected, > > e.g. > > > > | -cpu max,pauth={on,off},pauth-algo={impdef,architected} > > > > ... then in future the 'pauth={on,off}' bit could de extended to cover > > FPAC version etc independently of the algorithm, but for now that can be > > a boolean. > > Keeping pauth a boolean, but creating a pauth-algo enum doesn't help us > much, because probing will only be possible for pauth. The prober could > only decide to enable pauth with the default algo or not. We could create > a pauth-specific probe, similar to the gic-specific probe, but, IMO, it's > really not necessary. If we need multiple pauth-* properties, then we can > have them all. It just adds a few more lines of logic to the pauth > finalize function. I suspect that it will be necessary in future handle multiple options to enumerate things like FPAC/EPAC separately from the algorithm used, which is why I suggested the algo being its own option now. I can live with whatever you folk decide on. Thanks, Mark.
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index dd696183df..3181d0e2f8 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -572,6 +572,69 @@ void aarch64_add_sve_properties(Object *obj) } } +static const char * const pauth_names[] = { + "off", "impdef", "arch" +}; + +static const QEnumLookup pauth_lookup = { + .array = pauth_names, + .size = ARRAY_SIZE(pauth_names) +}; + +static int cpu_arm_get_pauth(Object *obj, Error **errp) +{ + ARMCPU *cpu = ARM_CPU(obj); + int value; + + /* We will always set GPA+APA and GPI+API to the same value. */ + if (FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, APA)) { + value = 2; + } else if (FIELD_EX64(cpu->isar.id_aa64isar1, ID_AA64ISAR1, API)) { + value = 1; + } else { + value = 0; + } + return value; +} + +static void cpu_arm_set_pauth(Object *obj, int value, Error **errp) +{ + ARMCPU *cpu = ARM_CPU(obj); + uint64_t t; + + /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */ + t = cpu->isar.id_aa64isar1; + switch (value) { + case 0: + t = FIELD_DP64(t, ID_AA64ISAR1, APA, 0); + t = FIELD_DP64(t, ID_AA64ISAR1, API, 0); + t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 0); + t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0); + break; + case 1: + t = FIELD_DP64(t, ID_AA64ISAR1, APA, 0); + t = FIELD_DP64(t, ID_AA64ISAR1, API, 1); + t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 0); + t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 1); + break; + case 2: + t = FIELD_DP64(t, ID_AA64ISAR1, APA, 1); + t = FIELD_DP64(t, ID_AA64ISAR1, API, 0); + t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 1); + t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0); + break; + default: + g_assert_not_reached(); + } + cpu->isar.id_aa64isar1 = t; +} + +static void aarch64_add_pauth_properties(Object *obj) +{ + object_property_add_enum(obj, "pauth", "ARMPauthKind", &pauth_lookup, + cpu_arm_get_pauth, cpu_arm_set_pauth); +} + /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host); * otherwise, a CPU with as many features enabled as our emulation supports. * The version of '-cpu max' for qemu-system-arm is defined in cpu.c; @@ -720,6 +783,7 @@ static void aarch64_max_initfn(Object *obj) #endif } + aarch64_add_pauth_properties(obj); aarch64_add_sve_properties(obj); object_property_add(obj, "sve-max-vq", "uint32", cpu_max_get_sve_max_vq, cpu_max_set_sve_max_vq, NULL, NULL);
The crypto overhead of emulating pauth can be significant for some workloads. Add an enumeration property that allows the feature to be turned off, on with the architected algorithm, or on with an implementation defined algorithm. The architected algorithm is quite expensive to emulate; using another algorithm may allow hardware acceleration. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/cpu64.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)