Change in ...coreboot[master]: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31608

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist

This patch adds timestamp for "end of romstage" with postcar if platform
has selected postcar as dedicated stage.

If postcar stage doesn't exist then "end of romstage" timestamp will get
call while starting of ramstage as exist today.

TEST=Its been observed that "end of romstage" timestamp doesn't appear
in "cbmem -t" log when ramstage is not getting executed. As part of this fix
"end of romstage" timestamp is showing in "cbmem -t" log on IA platform
where POSTCAR is a dedicated stage.

Change-Id: I17fd89296354b66a5538f85737c79145232593d3
Signed-off-by: Subrata Banik 
---
M src/arch/x86/postcar_loader.c
M src/lib/prog_loaders.c
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/31608/1

diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c
index 141e8d2..8d1cab4 100644
--- a/src/arch/x86/postcar_loader.c
+++ b/src/arch/x86/postcar_loader.c
@@ -23,6 +23,7 @@
 #include <rmodule.h>
 #include <romstage_handoff.h>
 #include <stage_cache.h>
+#include <timestamp.h>

static inline void stack_push(struct postcar_frame *pcf, uint32_t val)
 {
@@ -159,6 +160,9 @@
    struct prog prog =
    	PROG_INIT(PROG_POSTCAR, CONFIG_CBFS_PREFIX "/postcar");

+	/* As postcar exist, its end of romstage here */
+	timestamp_add_now(TS_END_ROMSTAGE);
+
    postcar_commit_mtrrs(pcf);

if (!IS_ENABLED(CONFIG_NO_STAGE_CACHE) &&
diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c
index 70ea7ef..5baeb8b 100644
--- a/src/lib/prog_loaders.c
+++ b/src/lib/prog_loaders.c
@@ -128,7 +128,9 @@
    struct prog ramstage =
    	PROG_INIT(PROG_RAMSTAGE, CONFIG_CBFS_PREFIX "/ramstage");

-	timestamp_add_now(TS_END_ROMSTAGE);
+	/* Call "end of romstage" here if postcar stage doesn't exist */
+	if (!IS_ENABLED(CONFIG_ARCH_POSTCAR_X86_32))
+		timestamp_add_now(TS_END_ROMSTAGE);

/*
     * Only x86 systems using ramstage stage cache currently take the same

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 1:

(2 comments)

Subrata, please wait for other reviewers to comment before reacting to my comments.

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c@164 
PS1, Line 164: 	timestamp_add_now(TS_END_ROMSTAGE);
I might move this just before prog_run() line below. There this timestamp would remain almost unchanged in comparison to previous build.

Then again, we probably want to be consistent with whether time spent loading or decompressing stage N+1 is included in between start..end of stage N.

https://review.coreboot.org/#/c/31608/1/src/lib/prog_loaders.c 
File src/lib/prog_loaders.c:

https://review.coreboot.org/#/c/31608/1/src/lib/prog_loaders.c@132 
PS1, Line 132: 	if (!IS_ENABLED(CONFIG_ARCH_POSTCAR_X86_32))
Maybe:

if (!ENV_POSTCAR)

[Paul Menzel (Code Review)]

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

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 1:

(3 comments)

https://review.coreboot.org/#/c/31608/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31608/1//COMMIT_MSG@15 
PS1, Line 15: Its
It’s

https://review.coreboot.org/#/c/31608/1//COMMIT_MSG@17 
PS1, Line 17: IA
What does this stand for?

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c@163 
PS1, Line 163: its
it’s

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 1:

(5 comments)

https://review.coreboot.org/#/c/31608/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/#/c/31608/1//COMMIT_MSG@15 
PS1, Line 15: Its
> It’s

Done

https://review.coreboot.org/#/c/31608/1//COMMIT_MSG@17 
PS1, Line 17: IA
> What does this stand for?

Intel Architecture

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c@163 
PS1, Line 163: its
> it’s

Done

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c@164 
PS1, Line 164: 	timestamp_add_now(TS_END_ROMSTAGE);
> I might move this just before prog_run() line below. […]

If I move this just before prog_run() then I will be adding postcar stage loading and caching time as well for "end of romstage", isn't ? if you see in ramstage, there were another 2 timestamp to know ramstage loading and hashing time.

we can also something like that as postcar loading start and end

https://review.coreboot.org/#/c/31608/1/src/lib/prog_loaders.c 
File src/lib/prog_loaders.c:

https://review.coreboot.org/#/c/31608/1/src/lib/prog_loaders.c@132 
PS1, Line 132: 	if (!IS_ENABLED(CONFIG_ARCH_POSTCAR_X86_32))
> Maybe: […]

Done

[Subrata Banik (Code Review)]

Hello Aaron Durbin, Ron Minnich, Aamir Bohra, Duncan Laurie, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist

This patch adds timestamp for "end of romstage" with postcar if platform
has selected postcar as dedicated stage.

If postcar stage doesn't exist then "end of romstage" timestamp will get
call while starting of ramstage as exist today.

TEST=It's been observed that "end of romstage" timestamp doesn't appear
in "cbmem -t" log when ramstage is not getting executed. As part of this fix
"end of romstage" timestamp is showing in "cbmem -t" log on Intel platform
where POSTCAR is a dedicated stage.

Change-Id: I17fd89296354b66a5538f85737c79145232593d3
Signed-off-by: Subrata Banik 
---
M src/arch/x86/postcar_loader.c
M src/lib/prog_loaders.c
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/31608/2

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 2:
> (2 comments)
>
> Subrata, please wait for other reviewers to comment before reacting
> to my comments.

any further comment ?

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c@164 
PS1, Line 164: 	timestamp_add_now(TS_END_ROMSTAGE);
> If I move this just before prog_run() then I will be adding postcar stage loading and caching time a […]

Well patchset #2 is removing load_postcar_cbfs() from TS_END_ROMSTAGE. I'd rather keep it in, you can provide a followup that adds individual stamps for postcar loader like the ones you refer to used in run_ramstage().

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c@164 
PS1, Line 164: 	timestamp_add_now(TS_END_ROMSTAGE);
> Well patchset #2 is removing load_postcar_cbfs() from
> TS_END_ROMSTAGE.

[Subrata] I didn't get you. i don;t see any change in this file between patchset #1 and #2

https://review.coreboot.org/#/c/31608/1..2/src/arch/x86/postcar_loader.c
> I'd rather keep it in, you can provide a followup
> that adds individual stamps for postcar loader like the ones you
> refer to used in run_ramstage().

[Subrata] We can do that as well, i will push separate CL for that.

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c@164 
PS1, Line 164: 	timestamp_add_now(TS_END_ROMSTAGE);
> > Well patchset #2 is removing load_postcar_cbfs() from
> > TS_END_ROMSTAGE.
>
> [Subrata] I didn't get you. i don;t see any change in this file between patchset #1 and #2

I meant a difference to base. It used to be such that loading postcar happens before TS_END_ROMSTAGE, both #1 and #2 would change that.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c@164 
PS1, Line 164: 	timestamp_add_now(TS_END_ROMSTAGE);
> > > Well patchset #2 is removing load_postcar_cbfs() from
> > > TS_END_ROMSTAGE.
> >
> > [Subrata] I didn't get you. i don;t see any change in this file
>
> between patchset #1 and #2
>
> I meant a difference to base. It used to be such that loading
> postcar happens before TS_END_ROMSTAGE, both #1 and #2 would change
> that.

[Subrata] in that case, run_ramstage also need to modify to make TS_END_ROMSTAGE happen before ramstage loading ?  https://github.com/coreboot/coreboot/blob/master/src/lib/prog_loaders.c

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/1/src/arch/x86/postcar_loader.c@164 
PS1, Line 164: 	timestamp_add_now(TS_END_ROMSTAGE);
> > Well patchset #2 is removing load_postcar_cbfs() from […]

https://review.coreboot.org/#/c/coreboot/+/31762/

[Aaron Durbin (Code Review)]

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

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31608/3/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/3/src/arch/x86/postcar_loader.c@177 
PS3, Line 177: 
END_ROMSTAGE should go here.

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/#/c/31608/3/src/arch/x86/postcar_loader.c 
File src/arch/x86/postcar_loader.c:

https://review.coreboot.org/#/c/31608/3/src/arch/x86/postcar_loader.c@177 
PS3, Line 177:
> END_ROMSTAGE should go here.

Done

[Subrata Banik (Code Review)]

Hello Aaron Durbin, Ron Minnich, Aamir Bohra, Duncan Laurie, build bot (Jenkins), Patrick Georgi, Furquan Shaikh,

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

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

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

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist

This patch adds timestamp for "end of romstage" with postcar if platform
has selected postcar as dedicated stage.

If postcar stage doesn't exist then "end of romstage" timestamp will get
call while starting of ramstage as exist today.

TEST=It's been observed that "end of romstage" timestamp doesn't appear
in "cbmem -t" log when ramstage is not getting executed. As part of this fix
"end of romstage" timestamp is showing in "cbmem -t" log on Intel platform
where POSTCAR is a dedicated stage.

Change-Id: I17fd89296354b66a5538f85737c79145232593d3
Signed-off-by: Subrata Banik 
---
M src/arch/x86/postcar_loader.c
M src/lib/prog_loaders.c
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/31608/4

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 4: Code-Review+2

[Aaron Durbin (Code Review)]

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

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/31608/4/src/lib/prog_loaders.c 
File src/lib/prog_loaders.c:

https://review.coreboot.org/#/c/31608/4/src/lib/prog_loaders.c@132 
PS4, Line 132: !ENV_POSTCAR
Just make this ENV_ROMSTAGE -- make it explicit.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 4: -Code-Review

[Subrata Banik (Code Review)]

Hello Aaron Durbin, Ron Minnich, Aamir Bohra, Duncan Laurie, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,

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

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

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

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist

This patch adds timestamp for "end of romstage" with postcar if platform
has selected postcar as dedicated stage.

If postcar stage doesn't exist then "end of romstage" timestamp will get
call while starting of ramstage as exist today.

TEST=It's been observed that "end of romstage" timestamp doesn't appear
in "cbmem -t" log when ramstage is not getting executed. As part of this fix
"end of romstage" timestamp is showing in "cbmem -t" log on Intel platform
where POSTCAR is a dedicated stage.

Change-Id: I17fd89296354b66a5538f85737c79145232593d3
Signed-off-by: Subrata Banik 
---
M src/arch/x86/postcar_loader.c
M src/lib/prog_loaders.c
2 files changed, 7 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/31608/5

[Aaron Durbin (Code Review)]

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

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

Patch Set 6: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31608 )

Change subject: prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist
......................................................................

prog_loader: Associate TS_END_ROMSTAGE timestamp with postcar if exist

This patch adds timestamp for "end of romstage" with postcar if platform
has selected postcar as dedicated stage.

If postcar stage doesn't exist then "end of romstage" timestamp will get
call while starting of ramstage as exist today.

TEST=It's been observed that "end of romstage" timestamp doesn't appear
in "cbmem -t" log when ramstage is not getting executed. As part of this fix
"end of romstage" timestamp is showing in "cbmem -t" log on Intel platform
where POSTCAR is a dedicated stage.

Change-Id: I17fd89296354b66a5538f85737c79145232593d3
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31608
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
---
M src/arch/x86/postcar_loader.c
M src/lib/prog_loaders.c
2 files changed, 7 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved

diff --git a/src/arch/x86/postcar_loader.c b/src/arch/x86/postcar_loader.c
index 141e8d2..a36b900 100644
--- a/src/arch/x86/postcar_loader.c
+++ b/src/arch/x86/postcar_loader.c
@@ -23,6 +23,7 @@
 #include <rmodule.h>
 #include <romstage_handoff.h>
 #include <stage_cache.h>
+#include <timestamp.h>

static inline void stack_push(struct postcar_frame *pcf, uint32_t val)
 {
@@ -171,5 +172,8 @@
    } else
    	load_postcar_cbfs(&prog, pcf);

+	/* As postcar exist, it's end of romstage here */
+	timestamp_add_now(TS_END_ROMSTAGE);
+
    prog_run(&prog);
 }
diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c
index da021f1..7319811 100644
--- a/src/lib/prog_loaders.c
+++ b/src/lib/prog_loaders.c
@@ -131,7 +131,9 @@
    if (ENV_POSTCAR)
    	timestamp_add_now(TS_END_POSTCAR);

-	timestamp_add_now(TS_END_ROMSTAGE);
+	/* Call "end of romstage" here if postcar stage doesn't exist */
+	if (ENV_ROMSTAGE)
+		timestamp_add_now(TS_END_ROMSTAGE);

/*
     * Only x86 systems using ramstage stage cache currently take the same