| Message ID | 20200426151302.93418-6-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | 33139a0bc7645f73f6e7dd152336e1ee00c9d40e |
| Headers | show |
| Series | x86: Improve support for chain-loading U-Boot | expand |
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 > 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 at gmail.com> Regards, Bin
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
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; 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; 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 */ + addr = detect_coreboot_table_at(0x0, 0x1000); + if (addr < 0) + addr = detect_coreboot_table_at(0xf0000, 0x1000); + + return addr; +} + int x86_cpu_reinit_f(void) { setup_identity(); diff --git a/arch/x86/include/asm/coreboot_tables.h b/arch/x86/include/asm/coreboot_tables.h index 61de0077d7..268284f43c 100644 --- a/arch/x86/include/asm/coreboot_tables.h +++ b/arch/x86/include/asm/coreboot_tables.h @@ -343,4 +343,11 @@ void *high_table_malloc(size_t bytes); */ void write_coreboot_table(u32 addr, struct memory_area *cfg_tables); +/** + * locate_coreboot_table() - Try to find coreboot tables at standard locations + * + * @return address of table that was found, or -ve error number + */ +long locate_coreboot_table(void); + #endif
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(-)