The forthcoming pseries consumer of the secvar API wants to expose a
number of config variables. Allowing secvar implementations to provide
their own sysfs attributes makes it easy for consumers to expose what
they need to.
This is not being used by the OPAL secvar implementation at present, and
the config directory will not be created if no attributes are set.
Signed-off-by: Russell Currey <[email protected]>
---
I played around with adding an API call to facilitate a more generic
key/value interface for config variables and it seemed like unnecessary
complexity. I think this is cleaner. If there was ever a secvar
interface other than sysfs we'd have to rework it, though.
arch/powerpc/include/asm/secvar.h | 3 +++
arch/powerpc/kernel/secvar-sysfs.c | 40 ++++++++++++++++++++++++++----
2 files changed, 38 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/secvar.h b/arch/powerpc/include/asm/secvar.h
index 92d2c051918b..250e7066b6da 100644
--- a/arch/powerpc/include/asm/secvar.h
+++ b/arch/powerpc/include/asm/secvar.h
@@ -10,6 +10,7 @@
#include <linux/types.h>
#include <linux/errno.h>
+#include <linux/sysfs.h>
extern const struct secvar_operations *secvar_ops;
@@ -27,10 +28,12 @@ struct secvar_operations {
#ifdef CONFIG_PPC_SECURE_BOOT
extern void set_secvar_ops(const struct secvar_operations *ops);
+extern void set_secvar_config_attrs(const struct attribute **attrs);
#else
static inline void set_secvar_ops(const struct secvar_operations *ops) { }
+static inline void set_secvar_config_attrs(const struct attribute **attrs) { }
#endif
diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
index aa1daec480e1..ad1e1d72d2ae 100644
--- a/arch/powerpc/kernel/secvar-sysfs.c
+++ b/arch/powerpc/kernel/secvar-sysfs.c
@@ -15,9 +15,17 @@
#define NAME_MAX_SIZE 1024
+const struct attribute **secvar_config_attrs __ro_after_init = NULL;
+
static struct kobject *secvar_kobj;
static struct kset *secvar_kset;
+void set_secvar_config_attrs(const struct attribute **attrs)
+{
+ WARN_ON_ONCE(secvar_config_attrs);
+ secvar_config_attrs = attrs;
+}
+
static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
char *buf)
{
@@ -134,6 +142,16 @@ static int update_kobj_size(void)
return 0;
}
+static int secvar_sysfs_config(struct kobject *kobj)
+{
+ struct attribute_group config_group = {
+ .name = "config",
+ .attrs = (struct attribute **)secvar_config_attrs,
+ };
+
+ return sysfs_create_group(kobj, &config_group);
+}
+
static int secvar_sysfs_load(void)
{
char *name;
@@ -196,26 +214,38 @@ static int secvar_sysfs_init(void)
rc = sysfs_create_file(secvar_kobj, &format_attr.attr);
if (rc) {
- kobject_put(secvar_kobj);
- return -ENOMEM;
+ pr_err("secvar: Failed to create format object\n");
+ rc = -ENOMEM;
+ goto err;
}
secvar_kset = kset_create_and_add("vars", NULL, secvar_kobj);
if (!secvar_kset) {
pr_err("secvar: sysfs kobject registration failed.\n");
- kobject_put(secvar_kobj);
- return -ENOMEM;
+ rc = -ENOMEM;
+ goto err;
}
rc = update_kobj_size();
if (rc) {
pr_err("Cannot read the size of the attribute\n");
- return rc;
+ goto err;
+ }
+
+ if (secvar_config_attrs) {
+ rc = secvar_sysfs_config(secvar_kobj);
+ if (rc) {
+ pr_err("secvar: Failed to create config directory\n");
+ goto err;
+ }
}
secvar_sysfs_load();
return 0;
+err:
+ kobject_put(secvar_kobj);
+ return rc;
}
late_initcall(secvar_sysfs_init);
--
2.38.1
On Fri, 2022-12-30 at 15:20 +1100, Russell Currey wrote:
> The forthcoming pseries consumer of the secvar API wants to expose a
> number of config variables. Allowing secvar implementations to
> provide
> their own sysfs attributes makes it easy for consumers to expose what
> they need to.
>
> This is not being used by the OPAL secvar implementation at present,
> and
> the config directory will not be created if no attributes are set.
>
> Signed-off-by: Russell Currey <[email protected]>
Minor comments below, but regardless:
Reviewed-by: Andrew Donnellan <[email protected]>
> ---
> I played around with adding an API call to facilitate a more generic
> key/value interface for config variables and it seemed like
> unnecessary
> complexity. I think this is cleaner. If there was ever a secvar
> interface other than sysfs we'd have to rework it, though.
I concur, this can be dealt with if/when the secvar interface is
exposed by some other means than sysfs.
>
> arch/powerpc/include/asm/secvar.h | 3 +++
> arch/powerpc/kernel/secvar-sysfs.c | 40 ++++++++++++++++++++++++++--
> --
> 2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/secvar.h
> b/arch/powerpc/include/asm/secvar.h
> index 92d2c051918b..250e7066b6da 100644
> --- a/arch/powerpc/include/asm/secvar.h
> +++ b/arch/powerpc/include/asm/secvar.h
> @@ -10,6 +10,7 @@
>
> #include <linux/types.h>
> #include <linux/errno.h>
> +#include <linux/sysfs.h>
>
> extern const struct secvar_operations *secvar_ops;
>
> @@ -27,10 +28,12 @@ struct secvar_operations {
> #ifdef CONFIG_PPC_SECURE_BOOT
>
> extern void set_secvar_ops(const struct secvar_operations *ops);
> +extern void set_secvar_config_attrs(const struct attribute **attrs);
>
> #else
>
> static inline void set_secvar_ops(const struct secvar_operations
> *ops) { }
> +static inline void set_secvar_config_attrs(const struct attribute
> **attrs) { }
>
> #endif
>
> diff --git a/arch/powerpc/kernel/secvar-sysfs.c
> b/arch/powerpc/kernel/secvar-sysfs.c
> index aa1daec480e1..ad1e1d72d2ae 100644
> --- a/arch/powerpc/kernel/secvar-sysfs.c
> +++ b/arch/powerpc/kernel/secvar-sysfs.c
> @@ -15,9 +15,17 @@
>
> #define NAME_MAX_SIZE 1024
>
> +const struct attribute **secvar_config_attrs __ro_after_init = NULL;
> +
> static struct kobject *secvar_kobj;
> static struct kset *secvar_kset;
>
> +void set_secvar_config_attrs(const struct attribute **attrs)
> +{
> + WARN_ON_ONCE(secvar_config_attrs);
> + secvar_config_attrs = attrs;
> +}
> +
> static ssize_t format_show(struct kobject *kobj, struct
> kobj_attribute *attr,
> char *buf)
> {
> @@ -134,6 +142,16 @@ static int update_kobj_size(void)
> return 0;
> }
>
> +static int secvar_sysfs_config(struct kobject *kobj)
> +{
> + struct attribute_group config_group = {
> + .name = "config",
> + .attrs = (struct attribute **)secvar_config_attrs,
> + };
I was slightly concerned that you're putting this on the stack, but it
doesn't appear that sysfs_create_group() keeps any references to the
group around after it creates all the files, so I think this is fine.
> +
> + return sysfs_create_group(kobj, &config_group);
> +}
> +
> static int secvar_sysfs_load(void)
> {
> char *name;
> @@ -196,26 +214,38 @@ static int secvar_sysfs_init(void)
>
> rc = sysfs_create_file(secvar_kobj, &format_attr.attr);
> if (rc) {
> - kobject_put(secvar_kobj);
> - return -ENOMEM;
> + pr_err("secvar: Failed to create format object\n");
This file defines pr_fmt, so the secvar: prefix here can go away,
though I notice that is the case for all the existing prints in this
function too.
> + rc = -ENOMEM;
> + goto err;
> }
>
> secvar_kset = kset_create_and_add("vars", NULL, secvar_kobj);
> if (!secvar_kset) {
> pr_err("secvar: sysfs kobject registration
> failed.\n");
> - kobject_put(secvar_kobj);
> - return -ENOMEM;
> + rc = -ENOMEM;
> + goto err;
> }
>
> rc = update_kobj_size();
> if (rc) {
> pr_err("Cannot read the size of the attribute\n");
> - return rc;
> + goto err;
> + }
> +
> + if (secvar_config_attrs) {
> + rc = secvar_sysfs_config(secvar_kobj);
> + if (rc) {
> + pr_err("secvar: Failed to create config
> directory\n");
Same comment as above
> + goto err;
> + }
> }
>
> secvar_sysfs_load();
>
> return 0;
> +err:
> + kobject_put(secvar_kobj);
> + return rc;
> }
>
> late_initcall(secvar_sysfs_init);
--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited
Russell Currey <[email protected]> writes:
> The forthcoming pseries consumer of the secvar API wants to expose a
> number of config variables. Allowing secvar implementations to provide
> their own sysfs attributes makes it easy for consumers to expose what
> they need to.
>
> This is not being used by the OPAL secvar implementation at present, and
> the config directory will not be created if no attributes are set.
Would it be slightly cleaner if the attributes were just a member of
secvar_operations?
That would avoid the need for some of the separate handling of the ops
and the attributes.
I know "ops" implies it's only methods, but I think that's not a big
problem. The power_pmu struct does something similar, where it combines
ops and attributes.
cheers
> diff --git a/arch/powerpc/include/asm/secvar.h b/arch/powerpc/include/asm/secvar.h
> index 92d2c051918b..250e7066b6da 100644
> --- a/arch/powerpc/include/asm/secvar.h
> +++ b/arch/powerpc/include/asm/secvar.h
> @@ -10,6 +10,7 @@
>
> #include <linux/types.h>
> #include <linux/errno.h>
> +#include <linux/sysfs.h>
>
> extern const struct secvar_operations *secvar_ops;
>
> @@ -27,10 +28,12 @@ struct secvar_operations {
> #ifdef CONFIG_PPC_SECURE_BOOT
>
> extern void set_secvar_ops(const struct secvar_operations *ops);
> +extern void set_secvar_config_attrs(const struct attribute **attrs);
>
> #else
>
> static inline void set_secvar_ops(const struct secvar_operations *ops) { }
> +static inline void set_secvar_config_attrs(const struct attribute **attrs) { }
>
> #endif
>
> diff --git a/arch/powerpc/kernel/secvar-sysfs.c b/arch/powerpc/kernel/secvar-sysfs.c
> index aa1daec480e1..ad1e1d72d2ae 100644
> --- a/arch/powerpc/kernel/secvar-sysfs.c
> +++ b/arch/powerpc/kernel/secvar-sysfs.c
> @@ -15,9 +15,17 @@
>
> #define NAME_MAX_SIZE 1024
>
> +const struct attribute **secvar_config_attrs __ro_after_init = NULL;
> +
> static struct kobject *secvar_kobj;
> static struct kset *secvar_kset;
>
> +void set_secvar_config_attrs(const struct attribute **attrs)
> +{
> + WARN_ON_ONCE(secvar_config_attrs);
> + secvar_config_attrs = attrs;
> +}
> +
> static ssize_t format_show(struct kobject *kobj, struct kobj_attribute *attr,
> char *buf)
> {
> @@ -134,6 +142,16 @@ static int update_kobj_size(void)
> return 0;
> }
>
> +static int secvar_sysfs_config(struct kobject *kobj)
> +{
> + struct attribute_group config_group = {
> + .name = "config",
> + .attrs = (struct attribute **)secvar_config_attrs,
> + };
> +
> + return sysfs_create_group(kobj, &config_group);
> +}
> +
> static int secvar_sysfs_load(void)
> {
> char *name;
> @@ -196,26 +214,38 @@ static int secvar_sysfs_init(void)
>
> rc = sysfs_create_file(secvar_kobj, &format_attr.attr);
> if (rc) {
> - kobject_put(secvar_kobj);
> - return -ENOMEM;
> + pr_err("secvar: Failed to create format object\n");
> + rc = -ENOMEM;
> + goto err;
> }
>
> secvar_kset = kset_create_and_add("vars", NULL, secvar_kobj);
> if (!secvar_kset) {
> pr_err("secvar: sysfs kobject registration failed.\n");
> - kobject_put(secvar_kobj);
> - return -ENOMEM;
> + rc = -ENOMEM;
> + goto err;
> }
>
> rc = update_kobj_size();
> if (rc) {
> pr_err("Cannot read the size of the attribute\n");
> - return rc;
> + goto err;
> + }
> +
> + if (secvar_config_attrs) {
> + rc = secvar_sysfs_config(secvar_kobj);
> + if (rc) {
> + pr_err("secvar: Failed to create config directory\n");
> + goto err;
> + }
> }
>
> secvar_sysfs_load();
>
> return 0;
> +err:
> + kobject_put(secvar_kobj);
> + return rc;
> }
>
> late_initcall(secvar_sysfs_init);
> --
> 2.38.1
On Fri, 2023-01-06 at 15:15 +1100, Michael Ellerman wrote:
> Russell Currey <[email protected]> writes:
> > The forthcoming pseries consumer of the secvar API wants to expose
> > a
> > number of config variables. Allowing secvar implementations to
> > provide
> > their own sysfs attributes makes it easy for consumers to expose
> > what
> > they need to.
> >
> > This is not being used by the OPAL secvar implementation at
> > present, and
> > the config directory will not be created if no attributes are set.
>
> Would it be slightly cleaner if the attributes were just a member of
> secvar_operations?
>
> That would avoid the need for some of the separate handling of the
> ops
> and the attributes.
>
> I know "ops" implies it's only methods, but I think that's not a big
> problem. The power_pmu struct does something similar, where it
> combines
> ops and attributes.
Yeah that does seem easier, thanks for the suggestion.
>
> cheers
>
> > diff --git a/arch/powerpc/include/asm/secvar.h
> > b/arch/powerpc/include/asm/secvar.h
> > index 92d2c051918b..250e7066b6da 100644
> > --- a/arch/powerpc/include/asm/secvar.h
> > +++ b/arch/powerpc/include/asm/secvar.h
> > @@ -10,6 +10,7 @@
> >
> > #include <linux/types.h>
> > #include <linux/errno.h>
> > +#include <linux/sysfs.h>
> >
> > extern const struct secvar_operations *secvar_ops;
> >
> > @@ -27,10 +28,12 @@ struct secvar_operations {
> > #ifdef CONFIG_PPC_SECURE_BOOT
> >
> > extern void set_secvar_ops(const struct secvar_operations *ops);
> > +extern void set_secvar_config_attrs(const struct attribute
> > **attrs);
> >
> > #else
> >
> > static inline void set_secvar_ops(const struct secvar_operations
> > *ops) { }
> > +static inline void set_secvar_config_attrs(const struct attribute
> > **attrs) { }
> >
> > #endif
> >
> > diff --git a/arch/powerpc/kernel/secvar-sysfs.c
> > b/arch/powerpc/kernel/secvar-sysfs.c
> > index aa1daec480e1..ad1e1d72d2ae 100644
> > --- a/arch/powerpc/kernel/secvar-sysfs.c
> > +++ b/arch/powerpc/kernel/secvar-sysfs.c
> > @@ -15,9 +15,17 @@
> >
> > #define NAME_MAX_SIZE 1024
> >
> > +const struct attribute **secvar_config_attrs __ro_after_init =
> > NULL;
> > +
> > static struct kobject *secvar_kobj;
> > static struct kset *secvar_kset;
> >
> > +void set_secvar_config_attrs(const struct attribute **attrs)
> > +{
> > + WARN_ON_ONCE(secvar_config_attrs);
> > + secvar_config_attrs = attrs;
> > +}
> > +
> > static ssize_t format_show(struct kobject *kobj, struct
> > kobj_attribute *attr,
> > char *buf)
> > {
> > @@ -134,6 +142,16 @@ static int update_kobj_size(void)
> > return 0;
> > }
> >
> > +static int secvar_sysfs_config(struct kobject *kobj)
> > +{
> > + struct attribute_group config_group = {
> > + .name = "config",
> > + .attrs = (struct attribute **)secvar_config_attrs,
> > + };
> > +
> > + return sysfs_create_group(kobj, &config_group);
> > +}
> > +
> > static int secvar_sysfs_load(void)
> > {
> > char *name;
> > @@ -196,26 +214,38 @@ static int secvar_sysfs_init(void)
> >
> > rc = sysfs_create_file(secvar_kobj, &format_attr.attr);
> > if (rc) {
> > - kobject_put(secvar_kobj);
> > - return -ENOMEM;
> > + pr_err("secvar: Failed to create format object\n");
> > + rc = -ENOMEM;
> > + goto err;
> > }
> >
> > secvar_kset = kset_create_and_add("vars", NULL,
> > secvar_kobj);
> > if (!secvar_kset) {
> > pr_err("secvar: sysfs kobject registration
> > failed.\n");
> > - kobject_put(secvar_kobj);
> > - return -ENOMEM;
> > + rc = -ENOMEM;
> > + goto err;
> > }
> >
> > rc = update_kobj_size();
> > if (rc) {
> > pr_err("Cannot read the size of the attribute\n");
> > - return rc;
> > + goto err;
> > + }
> > +
> > + if (secvar_config_attrs) {
> > + rc = secvar_sysfs_config(secvar_kobj);
> > + if (rc) {
> > + pr_err("secvar: Failed to create config
> > directory\n");
> > + goto err;
> > + }
> > }
> >
> > secvar_sysfs_load();
> >
> > return 0;
> > +err:
> > + kobject_put(secvar_kobj);
> > + return rc;
> > }
> >
> > late_initcall(secvar_sysfs_init);
> > --
> > 2.38.1
On Thu, 2023-01-05 at 18:28 +1100, Andrew Donnellan wrote:
> On Fri, 2022-12-30 at 15:20 +1100, Russell Currey wrote:
> > The forthcoming pseries consumer of the secvar API wants to expose
> > a
> > number of config variables. Allowing secvar implementations to
> > provide
> > their own sysfs attributes makes it easy for consumers to expose
> > what
> > they need to.
> >
> > This is not being used by the OPAL secvar implementation at
> > present,
> > and
> > the config directory will not be created if no attributes are set.
> >
> > Signed-off-by: Russell Currey <[email protected]>
>
> Minor comments below, but regardless:
>
> Reviewed-by: Andrew Donnellan <[email protected]>
>
> > ---
> > I played around with adding an API call to facilitate a more
> > generic
> > key/value interface for config variables and it seemed like
> > unnecessary
> > complexity. I think this is cleaner. If there was ever a secvar
> > interface other than sysfs we'd have to rework it, though.
>
> I concur, this can be dealt with if/when the secvar interface is
> exposed by some other means than sysfs.
>
> >
> > arch/powerpc/include/asm/secvar.h | 3 +++
> > arch/powerpc/kernel/secvar-sysfs.c | 40
> > ++++++++++++++++++++++++++--
> > --
> > 2 files changed, 38 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/secvar.h
> > b/arch/powerpc/include/asm/secvar.h
> > index 92d2c051918b..250e7066b6da 100644
> > --- a/arch/powerpc/include/asm/secvar.h
> > +++ b/arch/powerpc/include/asm/secvar.h
> > @@ -10,6 +10,7 @@
> >
> > #include <linux/types.h>
> > #include <linux/errno.h>
> > +#include <linux/sysfs.h>
> >
> > extern const struct secvar_operations *secvar_ops;
> >
> > @@ -27,10 +28,12 @@ struct secvar_operations {
> > #ifdef CONFIG_PPC_SECURE_BOOT
> >
> > extern void set_secvar_ops(const struct secvar_operations *ops);
> > +extern void set_secvar_config_attrs(const struct attribute
> > **attrs);
> >
> > #else
> >
> > static inline void set_secvar_ops(const struct secvar_operations
> > *ops) { }
> > +static inline void set_secvar_config_attrs(const struct attribute
> > **attrs) { }
> >
> > #endif
> >
> > diff --git a/arch/powerpc/kernel/secvar-sysfs.c
> > b/arch/powerpc/kernel/secvar-sysfs.c
> > index aa1daec480e1..ad1e1d72d2ae 100644
> > --- a/arch/powerpc/kernel/secvar-sysfs.c
> > +++ b/arch/powerpc/kernel/secvar-sysfs.c
> > @@ -15,9 +15,17 @@
> >
> > #define NAME_MAX_SIZE 1024
> >
> > +const struct attribute **secvar_config_attrs __ro_after_init =
> > NULL;
> > +
> > static struct kobject *secvar_kobj;
> > static struct kset *secvar_kset;
> >
> > +void set_secvar_config_attrs(const struct attribute **attrs)
> > +{
> > + WARN_ON_ONCE(secvar_config_attrs);
> > + secvar_config_attrs = attrs;
> > +}
> > +
> > static ssize_t format_show(struct kobject *kobj, struct
> > kobj_attribute *attr,
> > char *buf)
> > {
> > @@ -134,6 +142,16 @@ static int update_kobj_size(void)
> > return 0;
> > }
> >
> > +static int secvar_sysfs_config(struct kobject *kobj)
> > +{
> > + struct attribute_group config_group = {
> > + .name = "config",
> > + .attrs = (struct attribute **)secvar_config_attrs,
> > + };
>
> I was slightly concerned that you're putting this on the stack, but
> it
> doesn't appear that sysfs_create_group() keeps any references to the
> group around after it creates all the files, so I think this is fine.
>
> > +
> > + return sysfs_create_group(kobj, &config_group);
> > +}
> > +
> > static int secvar_sysfs_load(void)
> > {
> > char *name;
> > @@ -196,26 +214,38 @@ static int secvar_sysfs_init(void)
> >
> > rc = sysfs_create_file(secvar_kobj, &format_attr.attr);
> > if (rc) {
> > - kobject_put(secvar_kobj);
> > - return -ENOMEM;
> > + pr_err("secvar: Failed to create format object\n");
>
> This file defines pr_fmt, so the secvar: prefix here can go away,
> though I notice that is the case for all the existing prints in this
> function too.
Yeah we should fix that for all of them, good catch.
>
> > + rc = -ENOMEM;
> > + goto err;
> > }
> >
> > secvar_kset = kset_create_and_add("vars", NULL,
> > secvar_kobj);
> > if (!secvar_kset) {
> > pr_err("secvar: sysfs kobject registration
> > failed.\n");
> > - kobject_put(secvar_kobj);
> > - return -ENOMEM;
> > + rc = -ENOMEM;
> > + goto err;
> > }
> >
> > rc = update_kobj_size();
> > if (rc) {
> > pr_err("Cannot read the size of the attribute\n");
> > - return rc;
> > + goto err;
> > + }
> > +
> > + if (secvar_config_attrs) {
> > + rc = secvar_sysfs_config(secvar_kobj);
> > + if (rc) {
> > + pr_err("secvar: Failed to create config
> > directory\n");
>
> Same comment as above
>
> > + goto err;
> > + }
> > }
> >
> > secvar_sysfs_load();
> >
> > return 0;
> > +err:
> > + kobject_put(secvar_kobj);
> > + return rc;
> > }
> >
> > late_initcall(secvar_sysfs_init);
>