[PATCH v3a 1/2] lib: generic accessor functions for arch keystore

[Christophe Leroy]

Le 08/08/2022 à 17:43, gjoyce at linux.vnet.ibm.com a écrit :
> From: Greg Joyce <gjoyce at linux.vnet.ibm.com>
> 
> Generic kernel subsystems may rely on platform specific persistent
> KeyStore to store objects containing sensitive key material. In such case,
> they need to access architecture specific functions to perform read/write
> operations on these variables.
> 
> Define the generic variable read/write prototypes to be implemented by
> architecture specific versions. The default(weak) implementations of
> these prototypes return -EOPNOTSUPP unless overridden by architecture
> versions.
> 
> Signed-off-by: Greg Joyce <gjoyce at linux.vnet.ibm.com>
> ---
>   include/linux/arch_vars.h | 23 +++++++++++++++++++++++
>   lib/Makefile              |  2 +-
>   lib/arch_vars.c           | 25 +++++++++++++++++++++++++
>   3 files changed, 49 insertions(+), 1 deletion(-)
>   create mode 100644 include/linux/arch_vars.h
>   create mode 100644 lib/arch_vars.c
> 
> diff --git a/include/linux/arch_vars.h b/include/linux/arch_vars.h
> new file mode 100644
> index 000000000000..9c280ff9432e
> --- /dev/null
> +++ b/include/linux/arch_vars.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Platform variable opearations.

Is it platform specific or architecture specific ?

> + *
> + * Copyright (C) 2022 IBM Corporation
> + *
> + * These are the accessor functions (read/write) for architecture specific
> + * variables. Specific architectures can provide overrides.

"variables" is a very generic word which I think doesn't match what you 
want to do.

For me "variables" are local variables and global variables in a C file. 
Here it seems to be something completely different hence the name is 
really meaningfull and misleading.

> + *
> + */
> +
> +#include <linux/kernel.h>
> +
> +enum arch_variable_type {

arch_variable_type ? What's that ? variable types are char, short, long, 
long long, etc ...

> +	ARCH_VAR_OPAL_KEY      = 0,     /* SED Opal Authentication Key */
> +	ARCH_VAR_OTHER         = 1,     /* Other type of variable */
> +	ARCH_VAR_MAX           = 1,     /* Maximum type value */
> +};

Why the hell do you need an enum for two values only ?

> +
> +int arch_read_variable(enum arch_variable_type type, char *varname,
> +		       void *varbuf, u_int *varlen);
> +int arch_write_variable(enum arch_variable_type type, char *varname,
> +			void *varbuf, u_int varlen);
> diff --git a/lib/Makefile b/lib/Makefile
> index f99bf61f8bbc..b90c4cb0dbbb 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -48,7 +48,7 @@ obj-y += bcd.o sort.o parser.o debug_locks.o random32.o \
>   	 bsearch.o find_bit.o llist.o memweight.o kfifo.o \
>   	 percpu-refcount.o rhashtable.o \
>   	 once.o refcount.o usercopy.o errseq.o bucket_locks.o \
> -	 generic-radix-tree.o
> +	 generic-radix-tree.o arch_vars.o
>   obj-$(CONFIG_STRING_SELFTEST) += test_string.o
>   obj-y += string_helpers.o
>   obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
> diff --git a/lib/arch_vars.c b/lib/arch_vars.c
> new file mode 100644
> index 000000000000..e6f16d7d09c1
> --- /dev/null
> +++ b/lib/arch_vars.c

The name is meaningless, too generic.


> @@ -0,0 +1,25 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Platform variable operations.

platform versus architecture ?

> + *
> + * Copyright (C) 2022 IBM Corporation
> + *
> + * These are the accessor functions (read/write) for architecture specific
> + * variables. Specific architectures can provide overrides.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/arch_vars.h>
> +
> +int __weak arch_read_variable(enum arch_variable_type type, char *varname,
> +			      void *varbuf, u_int *varlen)

Sorry, to read a variable, I use READ_ONCE or I read it directly.

> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +int __weak arch_write_variable(enum arch_variable_type type, char *varname,
> +			       void *varbuf, u_int varlen)
> +{
> +	return -EOPNOTSUPP;
> +}