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

Hi Simon,

On Sun, Apr 26, 2020 at 11:13 PM Simon Glass sjg@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@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

    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

    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

•   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@gmail.com

Regards, Bin