[PATCH v4 6/9] x86: Move coreboot-table detection into common code

[Bin Meng]

Hi Simon,

On Thu, Apr 30, 2020 at 5:33 PM Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Sun, Apr 26, 2020 at 11:13 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > To support detecting booting from coreboot, move the code which locates
> > the coreboot tables into a common place. Adjust the algorithm slightly to
> > use a word comparison instead of string, since it is faster.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v4:
> > - Add new patch to move coreboot-table detection into common code
> >
> > Changes in v3: None
> > Changes in v2: None
> >
> >  arch/x86/cpu/coreboot/tables.c         | 24 +++++++++---------------
> >  arch/x86/cpu/i386/cpu.c                | 25 +++++++++++++++++++++++++
> >  arch/x86/include/asm/coreboot_tables.h |  7 +++++++
> >  3 files changed, 41 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/cpu/coreboot/tables.c b/arch/x86/cpu/coreboot/tables.c
> > index 37e0424b5e..0f04c4f8e9 100644
> > --- a/arch/x86/cpu/coreboot/tables.c
> > +++ b/arch/x86/cpu/coreboot/tables.c
> > @@ -115,20 +115,11 @@ __weak void cb_parse_unhandled(u32 tag, unsigned char *ptr)
> >
> >  static int cb_parse_header(void *addr, int len, struct sysinfo_t *info)
> >  {
> > +       unsigned char *ptr = addr;
>
> This variable is not necessary

No, I was wrong.

>
> >         struct cb_header *header;
> > -       unsigned char *ptr = (unsigned char *)addr;
> >         int i;
> >
> > -       for (i = 0; i < len; i += 16, ptr += 16) {
> > -               header = (struct cb_header *)ptr;
> > -               if (!strncmp((const char *)header->signature, "LBIO", 4))
> > -                       break;
> > -       }
> > -
> > -       /* We walked the entire space and didn't find anything. */
> > -       if (i >= len)
> > -               return -1;
> > -
> > +       header = (struct cb_header *)ptr;
>
> and assign addr to header directly

Ignore this.

>
> >         if (!header->table_bytes)
> >                 return 0;
> >
> > @@ -231,10 +222,13 @@ static int cb_parse_header(void *addr, int len, struct sysinfo_t *info)
> >
> >  int get_coreboot_info(struct sysinfo_t *info)
> >  {
> > -       int ret = cb_parse_header((void *)0x00000000, 0x1000, info);
> > +       long addr;
> > +       int ret;
> >
> > -       if (ret != 1)
> > -               ret = cb_parse_header((void *)0x000f0000, 0x1000, info);
> > +       addr = locate_coreboot_table();
> > +       if (addr < 0)
> > +               return addr;
> > +       ret = cb_parse_header((void *)addr, 0x1000, info);
> >
> > -       return (ret == 1) ? 0 : -1;
> > +       return ret == 1 ? 0 : -ENOENT;
> >  }
> > diff --git a/arch/x86/cpu/i386/cpu.c b/arch/x86/cpu/i386/cpu.c
> > index c8da7f10e9..e52fd686d9 100644
> > --- a/arch/x86/cpu/i386/cpu.c
> > +++ b/arch/x86/cpu/i386/cpu.c
> > @@ -447,6 +447,31 @@ int x86_cpu_init_f(void)
> >         return 0;
> >  }
> >
> > +long detect_coreboot_table_at(ulong start, ulong size)
> > +{
> > +       u32 *ptr, *end;
> > +
> > +       size /= 4;
> > +       for (ptr = (void *)start, end = ptr + size; ptr < end; ptr += 4) {
> > +               if (*ptr == 0x4f49424c) /* "LBIO" */
> > +                       return (long)ptr;
> > +       }
> > +
> > +       return -ENOENT;
> > +}
> > +
> > +long locate_coreboot_table(void)
> > +{
> > +       long addr;
> > +
> > +       /* We look for LBIO in the first 4K of RAM and again at 60KB */
>
> It should be 960KB

I can fix this comments when applying.

>
> > +       addr = detect_coreboot_table_at(0x0, 0x1000);
> > +       if (addr < 0)
> > +               addr = detect_coreboot_table_at(0xf0000, 0x1000);
> > +
> > +       return addr;
> > +}
> > +
>
> Looks good otherwise:
>
> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>

Regards,
Bin