Change in coreboot[master]: acpi: add a function to read a table and verify it

[ron minnich (Code Review)]

ron minnich has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

acpi: add a function to read a table and verify it

coreboot can currently read some ACPI tables from cbfs.
There is a fair amount of repeated code. Add a function
which can take a file name, and a table name, which
reads the file in, verifies its size, and its name,
returns a pointer to it if it is OK, NULL otherwise.

Show one use of the function: if a madt is found in
cbfs, use it instead of generating tables.

Over time, this code might replace other instances
of the copied-pasted code use for, e.g., the SLIT.

One such use of this code is on platforms where an
ACPI table has been supplied and it it not yet possible
to generate one.

Tested-by: Ronald G. Minnich

Change-Id: Id5e82eb0a82555b7763107e2a1e2a529a0c1b11f
Signed-off-by: Ronald G. Minnich 
---
M src/acpi/acpi.c
1 file changed, 36 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/41388/1

diff --git a/src/acpi/acpi.c b/src/acpi/acpi.c
index 8bf4b49..f1d2e78 100644
--- a/src/acpi/acpi.c
+++ b/src/acpi/acpi.c
@@ -39,6 +39,34 @@
 }

/**
+ * Read in a named table, and verify its properties
+ */
+acpi_header_t *acpi_read_table(const char *filename, const char *tablename)
+{
+	acpi_header_t *file;
+	size_t table_size;
+
+	file = cbfs_boot_map_with_leak(filename, CBFS_TYPE_RAW, &table_size);
+	if (!file) {
+		printk(BIOS_ERR, "No %s file for table %s\n", filename, tablename);
+		return NULL;
+	}
+	if (file->length > table_size) {
+		printk(BIOS_ERR, "Invalid %s file: file length(%d) > table_size(%ld)\n", filename, file->length, table_size);
+		return NULL;
+	}
+	if (file->length < sizeof(acpi_header_t)) {
+		printk(BIOS_ERR, "Invalid %s file: file length(%d) < table_size(%ld)\n", filename, file->length, table_size);
+		return NULL;
+	}
+	if (memcmp(file->signature, tablename, 4) != 0) {
+		printk(BIOS_ERR, "Invalid %s file, signature(%s) does not match %s\n", filename, file->signature, tablename);
+		return NULL;
+	}
+	return file;
+}
+
+/**
  * Add an ACPI table to the RSDT (and XSDT) structure, recalculate length
  * and checksum.
  */
@@ -231,9 +259,16 @@

void acpi_create_madt(acpi_madt_t *madt)
 {
-	acpi_header_t *header = &(madt->header);
+	acpi_header_t *header, *file;
    unsigned long current = (unsigned long)madt + sizeof(acpi_madt_t);

+	file = acpi_read_table(CONFIG_CBFS_PREFIX "/madt.aml", "APIC");
+	if (file) {
+		memmove(madt, file, file->length);
+		return;
+	}
+
+	header = &(madt->header);
    memset((void *)madt, 0, sizeof(acpi_madt_t));

if (!header)

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/c/coreboot/+/41388/1/src/acpi/acpi.c 
File src/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/41388/1/src/acpi/acpi.c@55 
PS1, Line 55: 		printk(BIOS_ERR, "Invalid %s file: file length(%d) > table_size(%ld)\n", filename, file->length, table_size);
line over 96 characters

https://review.coreboot.org/c/coreboot/+/41388/1/src/acpi/acpi.c@59 
PS1, Line 59: 		printk(BIOS_ERR, "Invalid %s file: file length(%d) < table_size(%ld)\n", filename, file->length, table_size);
line over 96 characters

https://review.coreboot.org/c/coreboot/+/41388/1/src/acpi/acpi.c@63 
PS1, Line 63: 		printk(BIOS_ERR, "Invalid %s file, signature(%s) does not match %s\n", filename, file->signature, tablename);
line over 96 characters

[ron minnich (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Jonathan Neuschäfer, Stefan Reinauer, Stefan Reinauer,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41388

to look at the new patch set (#2).

Change subject: acpi: add a function to read a table and verify it
......................................................................

acpi: add a function to read a table and verify it

coreboot can currently read some ACPI tables from cbfs.
There is a fair amount of repeated code. Add a function
which can take a file name, and a table name, which
reads the file in, verifies its size, and its name,
returns a pointer to it if it is OK, NULL otherwise.

Show one use of the function: if a madt is found in
cbfs, use it instead of generating tables.

Over time, this code might replace other instances
of the copied-pasted code use for, e.g., the SLIT.

One such use of this code is on platforms where an
ACPI table has been supplied and it it not yet possible
to generate one.

Tested-by: Ronald G. Minnich

Change-Id: Id5e82eb0a82555b7763107e2a1e2a529a0c1b11f
Signed-off-by: Ronald G. Minnich 
---
M src/acpi/acpi.c
1 file changed, 39 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/41388/2

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c 
File src/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c@55 
PS2, Line 55: 		printk(BIOS_ERR, "Invalid %s file: file length(%d) > table_size(%ld)\n", filename,
line over 96 characters

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c@60 
PS2, Line 60: 		printk(BIOS_ERR, "Invalid %s file: file length(%d) < table_size(%ld)\n", filename,
line over 96 characters

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c 
File src/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c@268 
PS2, Line 268: 	file = acpi_read_table(CONFIG_CBFS_PREFIX "/madt.aml", "APIC");
We do not want an unconditional path here of using cbfs. This should be guarded by a Kconfig.

[ron minnich (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, David Hendricks, Jonathan Neuschäfer, Stefan Reinauer, Stefan Reinauer,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41388

to look at the new patch set (#3).

Change subject: acpi: add a function to read a table and verify it
......................................................................

acpi: add a function to read a table and verify it

coreboot can currently read some ACPI tables from cbfs.
There is a fair amount of repeated code. Add a function
which can take a file name, and a table name, which
reads the file in, verifies its size, and its name,
returns a pointer to it if it is OK, NULL otherwise.

Show one use of the function: if a madt is found in
cbfs, use it instead of generating tables.

Over time, this code might replace other instances
of the copied-pasted code use for, e.g., the SLIT.

One such use of this code is on platforms where an
ACPI table has been supplied and it it not yet possible
to generate one.

Tested-by: Ronald G. Minnich

Change-Id: Id5e82eb0a82555b7763107e2a1e2a529a0c1b11f
Signed-off-by: Ronald G. Minnich 
---
M src/acpi/acpi.c
1 file changed, 39 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/88/41388/3

[ron minnich (Code Review)]

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c 
File src/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c@268 
PS2, Line 268: 	file = acpi_read_table(CONFIG_CBFS_PREFIX "/madt.aml", "APIC");
> We do not want an unconditional path here of using cbfs. This should be guarded by a Kconfig.

which one? I mean which Kconfig?

This is the exact same code pattern that's elsewhere in the file, for dsdt and slic, not guarded either.

I don't see the problem. If the file is not there, this code won't have any effect. What am I missing?

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c 
File src/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c@268 
PS2, Line 268: 	file = acpi_read_table(CONFIG_CBFS_PREFIX "/madt.aml", "APIC");
> which one? I mean which Kconfig?  […]

I didn't know we were doing that for dsdt and slic. Seems completely unnecessary when one knows we're not taking that path. SLIC is not always used. It's unfortunate we're unconditionally trying that. dsdt, granted, is inherently packaged in cbfs by the nature of how we build.

I'd prefer we don't try slic and madt unless instructed. Likewise, we shouldn't be using cbfs_boot_map_with_leak() as it makes quite a few assumptions about the boot media.

[ron minnich (Code Review)]

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c 
File src/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c@268 
PS2, Line 268: 	file = acpi_read_table(CONFIG_CBFS_PREFIX "/madt.aml", "APIC");
> I didn't know we were doing that for dsdt and slic. […]

so, for now, I'd rather not take on fixing all those problems, but if I take it as an action to fix them *along with this one* can we proceed?

What's the recommended function to use here, btw? I'm happy to make that fix everywhere it's used in this file.

What's your favorite new config variable name? I'm not a huge fan of continuing to add more config variables but if it's seen to be needed I guess we can do it.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c 
File src/acpi/acpi.c:

https://review.coreboot.org/c/coreboot/+/41388/2/src/acpi/acpi.c@268 
PS2, Line 268: 	file = acpi_read_table(CONFIG_CBFS_PREFIX "/madt.aml", "APIC");
> so, for now, I'd rather not take on fixing all those problems, but if I take it as an action to fix them *along with this one* can we proceed?

Which problems?
> What's the recommended function to use here, btw? I'm happy to make that fix everywhere it's used in this file.

You should do a rdev_mmap() -> rdev_read() -> rdev_unmap() in the acpi_read_table() function. That function should contain the memmove() itself instead of separating the functionality. The semantics that you want are "try and fill in this table from cbfs file". And this function should be used for all the tables accessing cbfs.
> What's your favorite new config variable name? I'm not a huge fan of continuing to add more config variables but if it's seen to be needed I guess we can do it.

It helps cull the bloat and surface are. Not having Kconfigs definitely allows ease of use by just adding files to cbfs, but it does bring that surface area. This is my opinion, of course.

[ron minnich (Code Review)]

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Patch Set 3: Code-Review+2

[ron minnich (Code Review)]

ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Patch Set 3: Code-Review-2

not for merge anyway

[ron minnich (Code Review)]

ron minnich has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41388 )

Change subject: acpi: add a function to read a table and verify it
......................................................................

Abandoned