diff mbox series

[1/2] target/arm: Add cpu property to control pauth

Message ID 20200812065339.2030527-2-richard.henderson@linaro.org
State New
Headers show
Series target/arm: Implement an IMPDEF pauth algorithm | expand

Commit Message

Richard Henderson Aug. 12, 2020, 6:53 a.m. UTC
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(+)

-- 
2.25.1

Comments

Andrew Jones Aug. 12, 2020, 11 a.m. UTC | #1
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
Richard Henderson Aug. 12, 2020, 3:10 p.m. UTC | #2
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~
Andrew Jones Aug. 12, 2020, 4:31 p.m. UTC | #3
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
Andrew Jones Aug. 13, 2020, 6:03 a.m. UTC | #4
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
Mark Rutland Aug. 13, 2020, 9:05 a.m. UTC | #5
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.
Andrew Jones Aug. 13, 2020, 9:49 a.m. UTC | #6
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
Mark Rutland Aug. 13, 2020, 11:10 a.m. UTC | #7
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 mbox series

Patch

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);