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@xxxxxxxxx> 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@xxxxxxxx>
> > Cc: Fabian Vogt <fvogt@xxxxxxxx>
> > Cc: Ilias Apalodimas <ilias.apalodimas@xxxxxxxxxx>
> > Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > Signed-off-by: "Lee, Chun-Yi" <jlee@xxxxxxxx>
> 
> 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
> >