Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200216182334.8121-1-ardb@kernel.org> <20200216182334.8121-13-ardb@kernel.org>
 <20200216191219.GA589207@rani.riverdale.lan>
In-Reply-To: <20200216191219.GA589207@rani.riverdale.lan>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Mon, 17 Feb 2020 09:32:39 +0100
Message-ID: <CAKv+Gu_NEuPAnEjFtk1MOs6xqcH-WNK2+6uP_EQ203vhjHzDaw@mail.gmail.com>
Subject: Re: [PATCH 12/18] efi: clean up config_parse_tables()
To:     Arvind Sankar <nivedita@alum.mit.edu>
Cc:     linux-efi <linux-efi@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "the arch/x86 maintainers" <x86@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 16 Feb 2020 at 20:12, Arvind Sankar <nivedita@alum.mit.edu> wrote:
>
> On Sun, Feb 16, 2020 at 07:23:28PM +0100, Ard Biesheuvel wrote:
> > config_parse_tables() is a jumble of pointer arithmetic, due to the
> > fact that on x86, we may be dealing with firmware whose native word
> > size differs from the kernel's.
> >
> > This is not a concern on other architectures, and doesn't quite
> > justify the state of the code, so let's clean it up by adding a
> > non-x86 code path, constifying statically allocated tables and
> > replacing preprocessor conditionals with IS_ENABLED() checks.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/ia64/kernel/efi.c          |  3 +-
> >  arch/x86/platform/efi/efi.c     |  6 +--
> >  drivers/firmware/efi/arm-init.c |  5 +--
> >  drivers/firmware/efi/efi.c      | 47 ++++++++++----------
> >  include/linux/efi.h             |  5 ++-
> >  5 files changed, 32 insertions(+), 34 deletions(-)
> >
...
       if (!retval)
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 2bfd6c0806ce..db1fe765380f 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
...
> > @@ -498,39 +498,38 @@ static __init int match_config_table(efi_guid_t *guid,
> >       return 0;
> >  }
> >
> > -int __init efi_config_parse_tables(void *config_tables, int count, int sz,
> > -                                efi_config_table_type_t *arch_tables)
> > +int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
> > +                                int count,
> > +                                const efi_config_table_type_t *arch_tables)
> >  {
> > -     void *tablep;
> > +     const efi_config_table_64_t *tbl64 = (void *)config_tables;
> > +     const efi_config_table_32_t *tbl32 = (void *)config_tables;
> > +     const efi_guid_t *guid;
> > +     unsigned long table;
> >       int i;
> >
> > -     tablep = config_tables;
> >       pr_info("");
> >       for (i = 0; i < count; i++) {
> > -             efi_guid_t guid;
> > -             unsigned long table;
> > -
> > -             if (efi_enabled(EFI_64BIT)) {
> > -                     u64 table64;
> > -                     guid = ((efi_config_table_64_t *)tablep)->guid;
> > -                     table64 = ((efi_config_table_64_t *)tablep)->table;
> > -                     table = table64;
> > -#ifndef CONFIG_64BIT
> > -                     if (table64 >> 32) {
> > +             if (!IS_ENABLED(CONFIG_X86)) {
> > +                     guid = &config_tables[i].guid;
> > +                     table = (unsigned long)config_tables[i].table;
> > +             } else if (efi_enabled(EFI_64BIT)) {
> > +                     guid = &tbl64[i].guid;
> > +                     table = tbl64[i].table;
> > +
> > +                     if (IS_ENABLED(CONFIG_X64_32) &&
>                                               ^^^ typo, should be X86
>

Noted, thanks for spotting that.