Change in coreboot[master]: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check

[Johnny Lin (Code Review)]

Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

intel/fsp2_0: Add soc_validate_fsp_version for FSP version check

Only need to check this once so check it at romstage where
the console is usually ready.

Change-Id: I628014e05bd567462f50af2633fbf48f3dc412bc
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
2 files changed, 6 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/47559/1

diff --git a/src/drivers/intel/fsp2_0/include/fsp/util.h b/src/drivers/intel/fsp2_0/include/fsp/util.h
index a57b1bb..2d2a58b 100644
--- a/src/drivers/intel/fsp2_0/include/fsp/util.h
+++ b/src/drivers/intel/fsp2_0/include/fsp/util.h
@@ -90,6 +90,7 @@
 void fsp_get_version(char *buf);
 void lb_string_platform_blob_version(struct lb_header *header);
 void report_fspt_output(void);
+void soc_validate_fsp_version(const struct fsp_header *hdr);

/* Fill in header and validate sanity of component within region device. */
 enum cb_err fsp_validate_component(struct fsp_header *hdr,
diff --git a/src/drivers/intel/fsp2_0/util.c b/src/drivers/intel/fsp2_0/util.c
index acc3f4b..dc54d46 100644
--- a/src/drivers/intel/fsp2_0/util.c
+++ b/src/drivers/intel/fsp2_0/util.c
@@ -85,6 +85,9 @@
    	return CB_ERR;
    }

+	if (ENV_ROMSTAGE)
+		soc_validate_fsp_version(hdr);
+
    return CB_SUCCESS;
 }

@@ -243,3 +246,5 @@
    rec->size = ALIGN_UP(sizeof(*rec) + len + 1, 8);
    memcpy(rec->string, fsp_version, len+1);
 }
+
+__weak void soc_validate_fsp_version(const struct fsp_header *hdr) { }

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 1:

This change is ready for review.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 1:

Hi,

So far CPX-SP FSP releases do not have version updated. If the FSP binary and coreboot (in particular FSP header files) are mismatched, serious problems happen, so it makes sense to add version check, and bail out loud and early when mismatch is detected.

We are pushing hard on Intel to version CPX-SP FSP releases appropriately. That being said, CPX-SP FSP engineering is winding down, so we do not have much wiggle room. The current (proposed) design is following:
The build number is the last digit with 8 bit encoding scheme:
|7             6|5 ………………  0|
|Year number % 4| WW number|
So the 6-bit can have 64 work weeks number, enough for a year, and the 2-bit year number can cover 4 year span (with 2020 as the first year).
Note that other fields (spec major/minor, rev major/minor/ver remain constant as 2.1-0.2.1.

Our plan is:
a. Update CPX-SP FSP version string display, for instance from 2.1-0.2.1.45 to 2.1-0.2.1-1.45. In other words, display the year number as well.
b. Add check in soc code [CB:47560] to check minimum/maximum required FSP binary version.
c. Work with Intel to make sure there is a sound design in this aspect for next generation xeon-sp FSP. I am not sure whether utilizing FirmwareVersionInfoHob.h is a good idea.

Your feedback is appreciated. We need to close on this quickly.

Thanks,
Jonathan

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 1:

(1 comment)

This part looks ok. The scheme for this fsp version should be described in to follow-on patch that implements the check.

https://review.coreboot.org/c/coreboot/+/47559/1/src/drivers/intel/fsp2_0/ut... 
File src/drivers/intel/fsp2_0/util.c:

https://review.coreboot.org/c/coreboot/+/47559/1/src/drivers/intel/fsp2_0/ut... 
PS1, Line 250: __weak void soc_validate_fsp_version(const struct fsp_header *hdr) { }
Maybe print a DEBUG __func__ not implemented on this weak call. We should encourage version checks.

[Johnny Lin (Code Review)]

Hello Marc Jones, build bot (Jenkins), Aaron Durbin, Jonathan Zhang, Subrata Banik, Andrey Petrov, Patrick Rudolph, Nathaniel L Desimone, Isaac W Oram,

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

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

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

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

intel/fsp2_0: Add soc_validate_fsp_version for FSP version check

Only need to check this once so check it at romstage where
the console is usually ready. Also define union fsp_revision
to avoid code duplication.

Change-Id: I628014e05bd567462f50af2633fbf48f3dc412bc
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/header_display.c
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
3 files changed, 21 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/47559/2

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47559/1/src/drivers/intel/fsp2_0/ut... 
File src/drivers/intel/fsp2_0/util.c:

https://review.coreboot.org/c/coreboot/+/47559/1/src/drivers/intel/fsp2_0/ut... 
PS1, Line 250: __weak void soc_validate_fsp_version(const struct fsp_header *hdr) { }
> Maybe print a DEBUG __func__ not implemented on this weak call. We should encourage version checks.

Done

[build bot (Jenkins) (Code Review)]

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

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47559/2/src/drivers/intel/fsp2_0/in... 
File src/drivers/intel/fsp2_0/include/fsp/util.h:

https://review.coreboot.org/c/coreboot/+/47559/2/src/drivers/intel/fsp2_0/in... 
PS2, Line 45: union fsp_revision{
missing space after union definition

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47559/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47559/1//COMMIT_MSG@10 
PS1, Line 10: the console is usually ready.
In addition, we need to have weak function of getting FSP version. For instance, for CPX-SP FSP, the FSP version should change from 2.1-0.2.1.45 to 2.1-0.2.1-0.45 (adding a year field, year 2020 is year 0 for CPX-SP FSP).

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Marc Jones, Aaron Durbin, Jonathan Zhang, Stefan Reinauer, Subrata Banik, Andrey Petrov, Patrick Rudolph, Nathaniel L Desimone, Isaac W Oram,

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

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

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

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

intel/fsp2_0: Add soc_validate_fsp_version for FSP version check

Only need to check this once so check it at romstage where
the console is usually ready. Also define union fsp_revision
to avoid code duplication.

Change-Id: I628014e05bd567462f50af2633fbf48f3dc412bc
Signed-off-by: Johnny Lin 
---
M src/drivers/intel/fsp2_0/header_display.c
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
3 files changed, 21 insertions(+), 18 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/47559/3

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47559/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47559/1//COMMIT_MSG@10 
PS1, Line 10: the console is usually ready.
> In addition, we need to have weak function of getting FSP version. […]

There's already a fsp_get_version in util.c, that can be the weak function that implements the general version display format. CPX can override it and have it's own format, but should other functions like fsp_print_header_info do the same? The display format only CPX is different.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47559/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47559/1//COMMIT_MSG@10 
PS1, Line 10: the console is usually ready.
> There's already a fsp_get_version in util. […]

Have about refactoring fsp_print_header_info() to get version string via vsp_get_version(). The current code has revision struct defined in two places, which is not necessary.

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47559/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/47559/1//COMMIT_MSG@10 
PS1, Line 10: the console is usually ready.
> Have about refactoring fsp_print_header_info() to get version string via vsp_get_version(). […]

The usage of fsp_get_version() is to print the version format to a buffer that can be added into coreboot table, while fsp_print_header_info() is meant to print it out on console
I think it's fine to keep fsp_print_header_info() As-is, and CPX can print its own version format in soc_validate_fsp_version().

I make fsp_get_version weak function in CB:47603 and override it in CB:47560.

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 3: Code-Review+1

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 3: Code-Review+1

This looks good to me.

I had a discussion with Nate. As a summary:
* The practice of using 2 bits in bld_num for year, 5 bits in bld_num for work week, aligns with client FSP practice.
* Intead of bld_num, revision should be used to indicated FSP UPD header compatibility. So far revision field has not been maintained for CPX-SP FSP, this however will be addressed by Intel CPX-SP FSP team. When that happens, we will update coreboot code to check revision field to make sure coreboot code is compatible with FSP binary in terms of UPD header.
* For HOBs, the GUID should be updated when there is structure change. So far the HOB GUIDs are not maintained for CPX-SP FSP, this will be addressed by Intel CPX-SP FSP team.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 3: Code-Review+2

[Marc Jones (Code Review)]

Marc Jones has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

intel/fsp2_0: Add soc_validate_fsp_version for FSP version check

Only need to check this once so check it at romstage where
the console is usually ready. Also define union fsp_revision
to avoid code duplication.

Change-Id: I628014e05bd567462f50af2633fbf48f3dc412bc
Signed-off-by: Johnny Lin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47559
Tested-by: build bot (Jenkins) 
Reviewed-by: Marc Jones 
Reviewed-by: Jonathan Zhang 
---
M src/drivers/intel/fsp2_0/header_display.c
M src/drivers/intel/fsp2_0/include/fsp/util.h
M src/drivers/intel/fsp2_0/util.c
3 files changed, 21 insertions(+), 18 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Marc Jones: Looks good to me, but someone else must approve
  Jonathan Zhang: Looks good to me, approved

diff --git a/src/drivers/intel/fsp2_0/header_display.c b/src/drivers/intel/fsp2_0/header_display.c
index 57e13b5..b45b4d8 100644
--- a/src/drivers/intel/fsp2_0/header_display.c
+++ b/src/drivers/intel/fsp2_0/header_display.c
@@ -5,15 +5,7 @@

void fsp_print_header_info(const struct fsp_header *hdr)
 {
-	union {
-		uint32_t val;
-		struct {
-			uint8_t bld_num;
-			uint8_t revision;
-			uint8_t minor;
-			uint8_t major;
-		} rev;
-	} revision;
+	union fsp_revision revision;

revision.val = hdr->fsp_revision;

diff --git a/src/drivers/intel/fsp2_0/include/fsp/util.h b/src/drivers/intel/fsp2_0/include/fsp/util.h
index a57b1bb..315db05 100644
--- a/src/drivers/intel/fsp2_0/include/fsp/util.h
+++ b/src/drivers/intel/fsp2_0/include/fsp/util.h
@@ -42,6 +42,16 @@
    uint64_t length;
 } __packed;

+union fsp_revision {
+	uint32_t val;
+	struct {
+		uint8_t bld_num;
+		uint8_t revision;
+		uint8_t minor;
+		uint8_t major;
+	} rev;
+};
+
 #if CONFIG_UDK_VERSION < CONFIG_UDK_2017_VERSION
 enum resource_type {
    EFI_RESOURCE_SYSTEM_MEMORY		= 0,
@@ -90,6 +100,7 @@
 void fsp_get_version(char *buf);
 void lb_string_platform_blob_version(struct lb_header *header);
 void report_fspt_output(void);
+void soc_validate_fsp_version(const struct fsp_header *hdr);

/* Fill in header and validate sanity of component within region device. */
 enum cb_err fsp_validate_component(struct fsp_header *hdr,
diff --git a/src/drivers/intel/fsp2_0/util.c b/src/drivers/intel/fsp2_0/util.c
index acc3f4b..490816d 100644
--- a/src/drivers/intel/fsp2_0/util.c
+++ b/src/drivers/intel/fsp2_0/util.c
@@ -85,6 +85,9 @@
    	return CB_ERR;
    }

+	if (ENV_ROMSTAGE)
+		soc_validate_fsp_version(hdr);
+
    return CB_SUCCESS;
 }

@@ -213,15 +216,7 @@
 void fsp_get_version(char *buf)
 {
    struct fsp_header *hdr = &fsps_hdr;
-	union {
-		uint32_t val;
-		struct {
-			uint8_t bld_num;
-			uint8_t revision;
-			uint8_t minor;
-			uint8_t major;
-		} rev;
-	} revision;
+	union fsp_revision revision;

revision.val = hdr->fsp_revision;
    snprintf(buf, FSP_VER_LEN, "%u.%u-%u.%u.%u.%u", (hdr->spec_version >> 4),
@@ -243,3 +238,8 @@
    rec->size = ALIGN_UP(sizeof(*rec) + len + 1, 8);
    memcpy(rec->string, fsp_version, len+1);
 }
+
+__weak void soc_validate_fsp_version(const struct fsp_header *hdr)
+{
+	printk(BIOS_DEBUG, "%s not implemented.\n", __func__);
+}

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47559 )

Change subject: intel/fsp2_0: Add soc_validate_fsp_version for FSP version check
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47559/3/src/drivers/intel/fsp2_0/ut... 
File src/drivers/intel/fsp2_0/util.c:

https://review.coreboot.org/c/coreboot/+/47559/3/src/drivers/intel/fsp2_0/ut... 
PS3, Line 244: printk(BIOS_DEBUG, "%s not implemented.\n", __func__);
Printing this on every FSP2.0 target is not a good idea. Having a Kconfig flag to indicate that a platform implements this functionality is better.