[PATCH v2] efi/efivars: Create efivars mount point via efivars abstraction

[PATCH v2] efi/efivars: Create efivars mount point via efivars abstraction

[Lee, Chun-Yi]

This patch creates efivars mount point when active efivars abstraction
be set. It is useful for userland to determine the availability of efivars
filesystem.

v2:
Using efivars_kobject() helper instead of checking GetVariable or
GetNextVariable EFI runtime services. Because the efivarfs code could be
instantiated using a different efivars abstraction.

Cc: Matthias Brugger <mbrugger@suse.com>
Cc: Fabian Vogt <fvogt@suse.com>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---
 drivers/firmware/efi/efi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3aa07c3b5136..db483fc68501 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -405,11 +405,13 @@ static int __init efisubsys_init(void)
 	if (error)
 		goto err_remove_group;
 
-	/* and the standard mountpoint for efivarfs */
-	error = sysfs_create_mount_point(efi_kobj, "efivars");
-	if (error) {
-		pr_err("efivars: Subsystem registration failed.\n");
-		goto err_remove_group;
+	if (efivars_kobject()) {
+		/* and the standard mountpoint for efivarfs */
+		error = sysfs_create_mount_point(efi_kobj, "efivars");
+		if (error) {
+			pr_err("efivars: Subsystem registration failed.\n");
+			goto err_remove_group;
+		}
 	}
 
 	if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
-- 
2.16.4

[PATCH v2] efi/efivars: Create efivars mount point via efivars abstraction

[Lee, Chun-Yi]

This patch creates efivars mount point when active efivars abstraction
be set. It is useful for userland to determine the availability of efivars
filesystem.

Cc: Matthias Brugger <mbrugger@suse.com>
Cc: Fabian Vogt <fvogt@suse.com>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
---

v2:
Using efivars_kobject() helper instead of checking GetVariable or
GetNextVariable EFI runtime services. Because the efivarfs code could be
instantiated using a different efivars abstraction.

 drivers/firmware/efi/efi.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3aa07c3b5136..db483fc68501 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -405,11 +405,13 @@ static int __init efisubsys_init(void)
 	if (error)
 		goto err_remove_group;
 
-	/* and the standard mountpoint for efivarfs */
-	error = sysfs_create_mount_point(efi_kobj, "efivars");
-	if (error) {
-		pr_err("efivars: Subsystem registration failed.\n");
-		goto err_remove_group;
+	if (efivars_kobject()) {
+		/* and the standard mountpoint for efivarfs */
+		error = sysfs_create_mount_point(efi_kobj, "efivars");
+		if (error) {
+			pr_err("efivars: Subsystem registration failed.\n");
+			goto err_remove_group;
+		}
 	}
 
 	if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
-- 
2.16.4

Re: [PATCH v2] efi/efivars: Create efivars mount point via efivars abstraction

[Ard Biesheuvel]

On Wed, 26 Aug 2020 at 02:46, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
>
> This patch creates efivars mount point when active efivars abstraction
> be set. It is useful for userland to determine the availability of efivars
> filesystem.
>
> Cc: Matthias Brugger <mbrugger@suse.com>
> Cc: Fabian Vogt <fvogt@suse.com>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>

Apologies for not bringing this up before: while the patch seems fine,
I wonder if we really need this if the purpose is to decide whether
efivars is available or not. We already have the 'efivars' platform
device for this, and so userland can simply check for the existence of

/sys/devices/platform/efivars.0

and so we don't need to make any changes for this.



> ---
>
> v2:
> Using efivars_kobject() helper instead of checking GetVariable or
> GetNextVariable EFI runtime services. Because the efivarfs code could be
> instantiated using a different efivars abstraction.
>
>  drivers/firmware/efi/efi.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 3aa07c3b5136..db483fc68501 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -405,11 +405,13 @@ static int __init efisubsys_init(void)
>         if (error)
>                 goto err_remove_group;
>
> -       /* and the standard mountpoint for efivarfs */
> -       error = sysfs_create_mount_point(efi_kobj, "efivars");
> -       if (error) {
> -               pr_err("efivars: Subsystem registration failed.\n");
> -               goto err_remove_group;
> +       if (efivars_kobject()) {
> +               /* and the standard mountpoint for efivarfs */
> +               error = sysfs_create_mount_point(efi_kobj, "efivars");
> +               if (error) {
> +                       pr_err("efivars: Subsystem registration failed.\n");
> +                       goto err_remove_group;
> +               }
>         }
>
>         if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
> --
> 2.16.4
>

Re: [PATCH v2] efi/efivars: Create efivars mount point via efivars abstraction

[joeyli]

Hi Ard,

On Wed, Aug 26, 2020 at 02:08:18PM +0200, Ard Biesheuvel wrote:
> On Wed, 26 Aug 2020 at 02:46, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> >
> > This patch creates efivars mount point when active efivars abstraction
> > be set. It is useful for userland to determine the availability of efivars
> > filesystem.
> >
> > Cc: Matthias Brugger <mbrugger@suse.com>
> > Cc: Fabian Vogt <fvogt@suse.com>
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> 
> Apologies for not bringing this up before: while the patch seems fine,
> I wonder if we really need this if the purpose is to decide whether
> efivars is available or not. We already have the 'efivars' platform
> device for this, and so userland can simply check for the existence of
> 
> /sys/devices/platform/efivars.0
> 
> and so we don't need to make any changes for this.
> 

The platform device only be registered on generic EFI runtime services
platform. If the efivars abstraction is google gsmi, then userland can
not use efivars.0 to detectmine the availability of efivars filesystem.

If we only consider generic EFI platform, then efivars.0 is good enough.
But if the gsmi implementation joins this game, then my patch should not
blocks the creation of efivars mount point because gsmi_kobj is not
initialized yet.

Maybe the creation of efivars mount point can be moved to
efivars_register(), after the kobject be set. Then gsmi can also create
mount point. How do you think?

On the other hand, the efisubsys_init() does not unregister efivars.0
platform device in err_unregister block. It causes that efivars and
efi-pstore be loaded when no generic_ops.

Let me know if I miss understood any thing, please.

Thanks a lot!
Joey Lee

> 
> 
> > ---
> >
> > v2:
> > Using efivars_kobject() helper instead of checking GetVariable or
> > GetNextVariable EFI runtime services. Because the efivarfs code could be
> > instantiated using a different efivars abstraction.
> >
> >  drivers/firmware/efi/efi.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 3aa07c3b5136..db483fc68501 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -405,11 +405,13 @@ static int __init efisubsys_init(void)
> >         if (error)
> >                 goto err_remove_group;
> >
> > -       /* and the standard mountpoint for efivarfs */
> > -       error = sysfs_create_mount_point(efi_kobj, "efivars");
> > -       if (error) {
> > -               pr_err("efivars: Subsystem registration failed.\n");
> > -               goto err_remove_group;
> > +       if (efivars_kobject()) {
> > +               /* and the standard mountpoint for efivarfs */
> > +               error = sysfs_create_mount_point(efi_kobj, "efivars");
> > +               if (error) {
> > +                       pr_err("efivars: Subsystem registration failed.\n");
> > +                       goto err_remove_group;
> > +               }
> >         }
> >
> >         if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
> > --
> > 2.16.4
> >

Re: [PATCH v2] efi/efivars: Create efivars mount point via efivars abstraction

[joeyli]

On Wed, Aug 26, 2020 at 11:56:33PM +0800, Joey Lee wrote:
> Hi Ard,
> 
> On Wed, Aug 26, 2020 at 02:08:18PM +0200, Ard Biesheuvel wrote:
> > On Wed, 26 Aug 2020 at 02:46, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > >
> > > This patch creates efivars mount point when active efivars abstraction
> > > be set. It is useful for userland to determine the availability of efivars
> > > filesystem.
> > >
> > > Cc: Matthias Brugger <mbrugger@suse.com>
> > > Cc: Fabian Vogt <fvogt@suse.com>
> > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > Cc: Ard Biesheuvel <ardb@kernel.org>
> > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> > 
> > Apologies for not bringing this up before: while the patch seems fine,
> > I wonder if we really need this if the purpose is to decide whether
> > efivars is available or not. We already have the 'efivars' platform
> > device for this, and so userland can simply check for the existence of
> > 
> > /sys/devices/platform/efivars.0
> > 
> > and so we don't need to make any changes for this.
> > 
> 
> The platform device only be registered on generic EFI runtime services
> platform. If the efivars abstraction is google gsmi, then userland can
> not use efivars.0 to detectmine the availability of efivars filesystem.
> 
> If we only consider generic EFI platform, then efivars.0 is good enough.
> But if the gsmi implementation joins this game, then my patch should not
> blocks the creation of efivars mount point because gsmi_kobj is not
> initialized yet.
> 
> Maybe the creation of efivars mount point can be moved to
> efivars_register(), after the kobject be set. Then gsmi can also create
> mount point. How do you think?
> 

Here is the patch for my concept. I simply tested on qemu-OVMF and
qemu-uboot.

From 7c81d3f058a177bb17c4589fe1863b79940f94c3 Mon Sep 17 00:00:00 2001
From: "Lee, Chun-Yi" <jlee@suse.com>
Date: Fri, 28 Aug 2020 00:05:22 +0800
Subject: [PATCH] efi/efivars: Create efivars mount point in the registration
 of efivars abstraction

This patch moved the logic of creating efivars mount point to the registration
of efivars abstraction. It is useful for userland to determine the availability
of efivars filesystem by checking the existence of mount point.

Signed-off-by: Lee, Chun-Yi <jlee@suse.com>
---
 drivers/firmware/efi/efi.c  |  7 -------
 drivers/firmware/efi/vars.c | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 3aa07c3b5136..23c11a2a3f4d 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -405,13 +405,6 @@ static int __init efisubsys_init(void)
 	if (error)
 		goto err_remove_group;
 
-	/* and the standard mountpoint for efivarfs */
-	error = sysfs_create_mount_point(efi_kobj, "efivars");
-	if (error) {
-		pr_err("efivars: Subsystem registration failed.\n");
-		goto err_remove_group;
-	}
-
 	if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
 		efi_debugfs_init();
 
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index 973eef234b36..6fa7f288d635 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -1179,6 +1179,8 @@ int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *kobject)
 {
+	int error;
+
 	if (down_interruptible(&efivars_lock))
 		return -EINTR;
 
@@ -1191,6 +1193,19 @@ int efivars_register(struct efivars *efivars,
 
 	up(&efivars_lock);
 
+	/* and the standard mountpoint for efivarfs */
+	if (efi_kobj) {
+		error = sysfs_create_mount_point(efi_kobj, "efivars");
+		if (error) {
+			if (down_interruptible(&efivars_lock))
+				return -EINTR;
+			__efivars = NULL;
+			up(&efivars_lock);
+			pr_err("efivars: Subsystem registration failed.\n");
+			return error;
+		}
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(efivars_register);
@@ -1222,6 +1237,8 @@ int efivars_unregister(struct efivars *efivars)
 
 	pr_info("Unregistered efivars operations\n");
 	__efivars = NULL;
+	if (efi_kobj)
+		sysfs_remove_mount_point(efi_kobj, "efivars");
 
 	rv = 0;
 out:
-- 
2.16.4

Thanks a lot!
Joey Lee

> On the other hand, the efisubsys_init() does not unregister efivars.0
> platform device in err_unregister block. It causes that efivars and
> efi-pstore be loaded when no generic_ops.
> 
> Let me know if I miss understood any thing, please.
> 
> 
> > 
> > 
> > > ---
> > >
> > > v2:
> > > Using efivars_kobject() helper instead of checking GetVariable or
> > > GetNextVariable EFI runtime services. Because the efivarfs code could be
> > > instantiated using a different efivars abstraction.
> > >
> > >  drivers/firmware/efi/efi.c | 12 +++++++-----
> > >  1 file changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index 3aa07c3b5136..db483fc68501 100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -405,11 +405,13 @@ static int __init efisubsys_init(void)
> > >         if (error)
> > >                 goto err_remove_group;
> > >
> > > -       /* and the standard mountpoint for efivarfs */
> > > -       error = sysfs_create_mount_point(efi_kobj, "efivars");
> > > -       if (error) {
> > > -               pr_err("efivars: Subsystem registration failed.\n");
> > > -               goto err_remove_group;
> > > +       if (efivars_kobject()) {
> > > +               /* and the standard mountpoint for efivarfs */
> > > +               error = sysfs_create_mount_point(efi_kobj, "efivars");
> > > +               if (error) {
> > > +                       pr_err("efivars: Subsystem registration failed.\n");
> > > +                       goto err_remove_group;
> > > +               }
> > >         }
> > >
> > >         if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
> > > --
> > > 2.16.4
> > >