Change in ...coreboot[master]: mb/emulation/qemu-440fx: remove mm file listing

[Thomas Heijligen (Code Review)]

Thomas Heijligen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30847

Change subject: mb/emulation/qemu-440fx: remove mm file listing
......................................................................

mb/emulation/qemu-440fx: remove mm file listing

Remove memory maped copy of the file list to use it also in romstage.
fw_cfg_find_file search directly for the file on data port.

Change-Id: Ie97ed3f0c98a5bb18a35ab0eaf8c4777a53e5779
Signed-off-by: Thomas Heijligen 
---
M src/mainboard/emulation/qemu-i440fx/fw_cfg.c
1 file changed, 14 insertions(+), 34 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/30847/1

diff --git a/src/mainboard/emulation/qemu-i440fx/fw_cfg.c b/src/mainboard/emulation/qemu-i440fx/fw_cfg.c
index 957e9d2..c5d6253 100644
--- a/src/mainboard/emulation/qemu-i440fx/fw_cfg.c
+++ b/src/mainboard/emulation/qemu-i440fx/fw_cfg.c
@@ -26,7 +26,7 @@
 #define FW_CFG_PORT_DATA      0x0511

static unsigned char fw_cfg_detected = 0xff;
-static FWCfgFiles *fw_files;
+//static FWCfgFiles *fw_files;

static int fw_cfg_present(void)
 {
@@ -58,49 +58,29 @@
    fw_cfg_read(dst, dstlen);
 }

-static void fw_cfg_init_file(void)
+static int fw_cfg_find_file(FWCfgFile *file, const char *name)
 {
-	u32 i, size, count = 0;
+	uint32_t count = 0;

-	if (fw_files != NULL)
-		return;
+	fw_cfg_select(FW_CFG_FILE_DIR);
+	fw_cfg_read(&count, sizeof(count));

-	fw_cfg_get(FW_CFG_FILE_DIR, &count, sizeof(count));
-	count = swab32(count);
-	size = count * sizeof(FWCfgFile) + sizeof(count);
-	printk(BIOS_DEBUG, "QEMU: %d files in fw_cfg\n", count);
-	fw_files = malloc(size);
-	fw_cfg_get(FW_CFG_FILE_DIR, fw_files, size);
-	fw_files->count = swab32(fw_files->count);
-	for (i = 0; i < count; i++) {
-		fw_files->f[i].size   = swab32(fw_files->f[i].size);
-		fw_files->f[i].select = swab16(fw_files->f[i].select);
-		printk(BIOS_DEBUG, "QEMU:     %s [size=%d]\n",
-		       fw_files->f[i].name, fw_files->f[i].size);
+	for (int i = 0; i < count; i++) {
+		fw_cfg_read(file, sizeof(*file));
+		if (strcmp(file->name, name) == 0) {
+			file->size = be32_to_cpu(file->size);
+			file->select = be16_to_cpu(file->select);
+			return 0;
+		}
    }
-}
-
-static FWCfgFile *fw_cfg_find_file(const char *name)
-{
-	int i;
-
-	fw_cfg_init_file();
-	for (i = 0; i < fw_files->count; i++)
-		if (strcmp(fw_files->f[i].name, name) == 0)
-			return fw_files->f + i;
-	return NULL;
+	return -1;
 }

int fw_cfg_check_file(FWCfgFile *file, const char *name)
 {
-	FWCfgFile *f;
    if (!fw_cfg_present())
    	return -1;
-	f = fw_cfg_find_file(name);
-	if (!f)
-		return -1;
-	*file = *f;
-	return 0;
+	return fw_cfg_find_file(file, name);
 }

int fw_cfg_max_cpus(void)

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-440fx: remove mm file listing
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/30847/1/src/mainboard/emulation/qemu-i440fx/... 
File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:

https://review.coreboot.org/#/c/30847/1/src/mainboard/emulation/qemu-i440fx/... 
PS1, Line 29: //static FWCfgFiles *fw_files;
please don't comment code, remove it

https://review.coreboot.org/#/c/30847/1/src/mainboard/emulation/qemu-i440fx/... 
PS1, Line 66: 	fw_cfg_read(&count, sizeof(count));
you could use a static buffer on heap instead of malloc. There are usually less than 16 files, so you don't need that much heap.

[Thomas Heijligen (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: mb/emulation/qemu-440fx: remove mm file listing
......................................................................

mb/emulation/qemu-440fx: remove mm file listing

Remove memory maped copy of the file list to use it also in romstage.
fw_cfg_find_file search directly for the file on data port.

Change-Id: Ie97ed3f0c98a5bb18a35ab0eaf8c4777a53e5779
Signed-off-by: Thomas Heijligen 
---
M src/mainboard/emulation/qemu-i440fx/fw_cfg.c
1 file changed, 13 insertions(+), 34 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/30847/3

[Thomas Heijligen (Code Review)]

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-440fx: remove mm file listing
......................................................................

Patch Set 3:

(2 comments)
> Patch Set 1:
>
> (2 comments)

https://review.coreboot.org/#/c/30847/1/src/mainboard/emulation/qemu-i440fx/... 
File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:

https://review.coreboot.org/#/c/30847/1/src/mainboard/emulation/qemu-i440fx/... 
PS1, Line 29: //static FWCfgFiles *fw_files;
> please don't comment code, remove it

Done

https://review.coreboot.org/#/c/30847/1/src/mainboard/emulation/qemu-i440fx/... 
PS1, Line 66: 	fw_cfg_read(&count, sizeof(count));
> you could use a static buffer on heap instead of malloc. […]

We don't know if there more files in future and without a static buffer it should work then out of the box

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-440fx: remove mm file listing
......................................................................

Patch Set 4: Code-Review+2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-440fx: remove mm file listing
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/30847/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30847/4//COMMIT_MSG@7 
PS4, Line 7: mb/emulation/qemu-440fx: remove mm file listing
nit: missing i

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-440fx: remove mm file listing
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/#/c/30847/4//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/30847/4//COMMIT_MSG@9 
PS4, Line 9: maped
mapped

https://review.coreboot.org/#/c/30847/4//COMMIT_MSG@10 
PS4, Line 10: search
searches?

https://review.coreboot.org/#/c/30847/4/src/mainboard/emulation/qemu-i440fx/... 
File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:

https://review.coreboot.org/#/c/30847/4/src/mainboard/emulation/qemu-i440fx/... 
PS4, Line 62: uint32_t
size_t?

[Thomas Heijligen (Code Review)]

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-440fx: remove mm file listing
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/30847/4/src/mainboard/emulation/qemu-i440fx/... 
File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:

https://review.coreboot.org/#/c/30847/4/src/mainboard/emulation/qemu-i440fx/... 
PS4, Line 62: uint32_t
> size_t?

count refers to FWCfgFiles count (fw_cfg_if.h) which is an uint32_t. Reading more or less than 32 bit leads to problems.

Change in ...coreboot[master]: mb/emulation/qemu-i440fx: remove mm file listing

[Thomas Heijligen (Code Review)]

Hello Patrick Rudolph, Paul Menzel, build bot (Jenkins),

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

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

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

Change subject: mb/emulation/qemu-i440fx: remove mm file listing
......................................................................

mb/emulation/qemu-i440fx: remove mm file listing

Remove memory mapped copy of the file list to use it also in romstage.
fw_cfg_find_file searches directly for the file on data port.

Change-Id: Ie97ed3f0c98a5bb18a35ab0eaf8c4777a53e5779
Signed-off-by: Thomas Heijligen 
---
M src/mainboard/emulation/qemu-i440fx/fw_cfg.c
1 file changed, 13 insertions(+), 34 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/30847/5

Change in ...coreboot[master]: mb/emulation/qemu-i440fx: remove mm file listing

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-i440fx: remove mm file listing
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/30847/7/src/mainboard/emulation/qemu-i440fx/... 
File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:

https://review.coreboot.org/#/c/30847/7/src/mainboard/emulation/qemu-i440fx/... 
PS7, Line 65: 	fw_cfg_read(&count, sizeof(count));
looks like be32_to_cpu(count) is missing

Change in ...coreboot[master]: mb/emulation/qemu-i440fx: remove mm file listing

[Thomas Heijligen (Code Review)]

Hello Patrick Rudolph, Paul Menzel, build bot (Jenkins),

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

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

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

Change subject: mb/emulation/qemu-i440fx: remove mm file listing
......................................................................

mb/emulation/qemu-i440fx: remove mm file listing

Remove memory mapped copy of the file list to use it also in romstage.
fw_cfg_find_file searches directly for the file on data port.

Change-Id: Ie97ed3f0c98a5bb18a35ab0eaf8c4777a53e5779
Signed-off-by: Thomas Heijligen 
---
M src/mainboard/emulation/qemu-i440fx/fw_cfg.c
1 file changed, 14 insertions(+), 34 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/30847/8

Change in ...coreboot[master]: mb/emulation/qemu-i440fx: remove mm file listing

[Thomas Heijligen (Code Review)]

Thomas Heijligen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-i440fx: remove mm file listing
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/#/c/30847/7/src/mainboard/emulation/qemu-i440fx/... 
File src/mainboard/emulation/qemu-i440fx/fw_cfg.c:

https://review.coreboot.org/#/c/30847/7/src/mainboard/emulation/qemu-i440fx/... 
PS7, Line 65: 	fw_cfg_read(&count, sizeof(count));
> looks like be32_to_cpu(count) is missing

You were right

Change in ...coreboot[master]: mb/emulation/qemu-i440fx: remove mm file listing

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-i440fx: remove mm file listing
......................................................................

Patch Set 8: Code-Review+2

Change in ...coreboot[master]: mb/emulation/qemu-i440fx: remove mm file listing

[Nico Huber (Code Review)]

Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30847 )

Change subject: mb/emulation/qemu-i440fx: remove mm file listing
......................................................................

mb/emulation/qemu-i440fx: remove mm file listing

Remove memory mapped copy of the file list to use it also in romstage.
fw_cfg_find_file searches directly for the file on data port.

Change-Id: Ie97ed3f0c98a5bb18a35ab0eaf8c4777a53e5779
Signed-off-by: Thomas Heijligen 
Reviewed-on: https://review.coreboot.org/c/30847
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
---
M src/mainboard/emulation/qemu-i440fx/fw_cfg.c
1 file changed, 14 insertions(+), 34 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/mainboard/emulation/qemu-i440fx/fw_cfg.c b/src/mainboard/emulation/qemu-i440fx/fw_cfg.c
index 364221e..b859bdb 100644
--- a/src/mainboard/emulation/qemu-i440fx/fw_cfg.c
+++ b/src/mainboard/emulation/qemu-i440fx/fw_cfg.c
@@ -26,7 +26,6 @@
 #define FW_CFG_PORT_DATA      0x0511

static unsigned char fw_cfg_detected = 0xff;
-static FWCfgFiles *fw_files;

static int fw_cfg_present(void)
 {
@@ -58,49 +57,30 @@
    fw_cfg_read(dst, dstlen);
 }

-static void fw_cfg_init_file(void)
+static int fw_cfg_find_file(FWCfgFile *file, const char *name)
 {
-	u32 i, size, count = 0;
+	uint32_t count = 0;

-	if (fw_files != NULL)
-		return;
+	fw_cfg_select(FW_CFG_FILE_DIR);
+	fw_cfg_read(&count, sizeof(count));
+	count = be32_to_cpu(count);

-	fw_cfg_get(FW_CFG_FILE_DIR, &count, sizeof(count));
-	count = swab32(count);
-	size = count * sizeof(FWCfgFile) + sizeof(count);
-	printk(BIOS_DEBUG, "QEMU: %d files in fw_cfg\n", count);
-	fw_files = malloc(size);
-	fw_cfg_get(FW_CFG_FILE_DIR, fw_files, size);
-	fw_files->count = swab32(fw_files->count);
-	for (i = 0; i < count; i++) {
-		fw_files->f[i].size   = swab32(fw_files->f[i].size);
-		fw_files->f[i].select = swab16(fw_files->f[i].select);
-		printk(BIOS_DEBUG, "QEMU:     %s [size=%d]\n",
-		       fw_files->f[i].name, fw_files->f[i].size);
+	for (int i = 0; i < count; i++) {
+		fw_cfg_read(file, sizeof(*file));
+		if (strcmp(file->name, name) == 0) {
+			file->size = be32_to_cpu(file->size);
+			file->select = be16_to_cpu(file->select);
+			return 0;
+		}
    }
-}
-
-static FWCfgFile *fw_cfg_find_file(const char *name)
-{
-	int i;
-
-	fw_cfg_init_file();
-	for (i = 0; i < fw_files->count; i++)
-		if (strcmp(fw_files->f[i].name, name) == 0)
-			return fw_files->f + i;
-	return NULL;
+	return -1;
 }

int fw_cfg_check_file(FWCfgFile *file, const char *name)
 {
-	FWCfgFile *f;
    if (!fw_cfg_present())
    	return -1;
-	f = fw_cfg_find_file(name);
-	if (!f)
-		return -1;
-	*file = *f;
-	return 0;
+	return fw_cfg_find_file(file, name);
 }

int fw_cfg_max_cpus(void)