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

[Andrew Jones]

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