MinnowBoard MRC based support #542

miczyg1 · 2024-07-25T13:09:26Z

This PR adds silicon initialization not covered by native coreboot support but done in FSP. It also contains various fixes, TXE BIOS flow and CBFS_VERIFICATION integration with TXE Secure Boot on Bay Trail platform - Intel MinnowBoard Turbot.

miczyg1 · 2024-07-30T10:57:21Z

Needs this to land too: Dasharo/edk2#156

.github/workflows/build.yml

src/arch/x86/smbios.c

src/device/pciexp_device.c

krystian-hebel · 2024-08-13T12:30:50Z

src/soc/intel/baytrail/Kconfig

-# Put the microcode right after the manifest, so it will be verified by IBB
-# and accessible without walkcbfs ASM routine if CBFS_VERIFICATION is enabled.
-config CPU_MICROCODE_CBFS_LOC
+# These addresses will be used by BSP to locate microcode in bootblock before


Why plural? I can see only one address.

Fixed: https://github.com/Dasharo/coreboot/pull/542/files#diff-a68c92b0f440c50bce825bdbdc037e9897818b52b6545daa9255faf335b5c071R263

Partially fixed, addresses now doesn't match this: https://github.com/Dasharo/coreboot/pull/542/files#diff-a68c92b0f440c50bce825bdbdc037e9897818b52b6545daa9255faf335b5c071R275

https://github.com/Dasharo/coreboot/pull/542/files#r1801179842

src/soc/intel/baytrail/Kconfig

krystian-hebel · 2024-08-13T13:05:04Z

src/soc/intel/baytrail/Makefile.mk

-	dd if=/dev/zero of=$@ bs=5120 count=1 2> /dev/null
-	dd if=$< of=$@ conv=notrunc 2> /dev/null
-else
+key_manifest_pos = $(call int-subtract, $(CONFIG_ROM_SIZE) 0x21000)


Please remove hardcoded size. It should be possible to use something like $(CBFSTOOL) $(1) write -r MANIFESTS -f $(CONFIG_TXE_SB_KEY_MANIFEST_PATH) to get the same result.

I'm not sure I get it. What is wrong with the hardcoded size?

Ohh I get it, you mean to simply put the manifest using cbfstool without defining the location. but this does not guarantee that the Key Manifest will end up in the proper location. The address of Key manifest is fixed. MANIFESTS region may be placed anywhere...

@krystian-hebel so should we leave the hardcoded size?

MANIFESTS region may be placed anywhere...

Then it would be better to assert that this region is properly located, than blindly dd into offset that may be outside of MANIFESTS.

Then it would be better to assert that this region is properly located, than blindly dd into offset that may be outside of MANIFESTS.

Yes, it could be checked somewhere. But blind DD is better than building an image which won't get the CPU out of reset...

Check implemented: a52993b
Also writing the key manifest with cbfstool using MANIFESTS region.

It works, but is there a reason why there are two commas in $(filter)?

4a0e6c1

No reason, probably some leftover from previous attempt to get this logic working...

src/soc/intel/baytrail/txei.c

pietrushnic · 2024-08-13T20:19:09Z

@BeataZdunczyk @macpijan, could anyone fix the code according to Krystian's suggestion while @miczyg1 is on vacation? I also wonder if we shouldn't go upstream with those patches immediately.

BeataZdunczyk · 2024-08-14T06:28:46Z

@pietrushnic, @miczyg1 is working today, so he will address the review and the issues raised from testing RC1. If anything is more challenging to address, we will, of course, assign the task to someone else. Upstream is out of scope for this release, but we can evaluate the effort needed.

miczyg1 · 2024-08-14T13:14:08Z

I have addressed what I could. Not sure how to address the PCIe comments properly yet.

All Intel SOCs use input pullup 20K as not connected GPIO. Also GPIO output mode may be harmful. Signed-off-by: Michał Żygowski <[email protected]>

Fixes a problem where changing ME binary in the config would not result in the desired ME binary be included in the rebuilt coreboot binary. Signed-off-by: Michał Żygowski <[email protected]>

src/soc/intel/baytrail/Kconfig

krystian-hebel · 2024-10-15T14:22:44Z

src/soc/intel/baytrail/Kconfig

 	hex
 	default 0x19800

-endif
+# Ensure the bootblock does not overlap the microcode. We have 0x6400 bytes
+# of space after microcode till the ned of ROM. Make a small gap of 256 bytes


Suggested change

# of space after microcode till the ned of ROM. Make a small gap of 256 bytes

# of space after microcode till the end of ROM. Make a small gap of 256 bytes

src/soc/intel/baytrail/romstage/romstage.c

krystian-hebel · 2024-10-15T15:29:07Z

src/soc/intel/baytrail/Makefile.mk

-	dd if=/dev/zero of=$@ bs=5120 count=1 2> /dev/null
-	dd if=$< of=$@ conv=notrunc 2> /dev/null
-else
+key_manifest_pos = $(call int-subtract, $(CONFIG_ROM_SIZE) 0x21000)


It works, but is there a reason why there are two commas in $(filter)?

krystian-hebel · 2024-10-15T15:54:45Z

src/soc/intel/baytrail/txei.c

+/* get number of full 4-byte slots */
+static size_t bytes_to_slots(size_t bytes)
+{
+	return (bytes + (SLOT_SIZE - 1) + SLOT_SIZE) / SLOT_SIZE;


This is the ceiling of size, which is then increased by one. Is this an error or is it caused by the length not including the header? In any case, this should be commented.

Reworked it to actually do what the comment does: 6a4524e

Used ALIGN_UP like the original source of CSE HECI src/soc/intel/common/block/cse/cse.c does

Also added comments near the loops where the slots are written to explain how it works.

krystian-hebel · 2024-10-15T16:14:28Z

src/soc/intel/baytrail/txei.c

+
+	/* Write the body in whole slots */
+	for (i = 1; i < pend_slots; i++, *p++) {
+		write_slot(i, *p);


This looks like out of bounds read. Either add a check that pend_len is divisible by 4, or add a comment that all the messages come from this file (i.e. there are no non-static functions with message controlled by the caller) and have sizes that are of such sizes and the check isn't necessary.

It may often be out of bound read, depending on the message length. If it is not divisible by 4, then some additional bytes may be sent at the end of message. However, ME/TXE should not care, because the real length of the message is in the message header. But we still must write 32bit slots... Of course we may always check for the message length, reallocate and copy it to a new buffer that is divisible by 4 and appended with zeros if necessary. However, the original source of CSE HECI src/soc/intel/common/block/cse/cse.c does not do it.

6a4524e

This is not exactly what you wanted. Although like I mentioned above, it is based on the CSE HECI from common blocks, which does the same. And TXE/CSE should not care about bytes after the message size.

krystian-hebel · 2024-10-15T16:16:26Z

src/soc/intel/baytrail/txei.c

+	case TXE_FWSTS0_COM_DEBUG:
+	case TXE_FWSTS0_COM_SOFT_TEMP_DISABLE:
+	case TXE_FWSTS0_COM_SECOVER_JMPR:
+		printk(BIOS_INFO, "Current TXE operation mode does not allow"


https://github.com/Dasharo/coreboot/blob/dasharo/Documentation/contributing/coding_style.md#breaking-long-lines-and-strings

However, never break user-visible strings such as printk messages, because that breaks the ability to grep for them.

Signed-off-by: Michał Żygowski <[email protected]>

miczyg1 force-pushed the byt_fsp_parity branch 4 times, most recently from b48cbaf to f8a10ca Compare July 25, 2024 14:06

miczyg1 requested review from krystian-hebel and macpijan July 25, 2024 14:46

miczyg1 force-pushed the byt_fsp_parity branch 2 times, most recently from 4dfd55f to 22e27c3 Compare July 26, 2024 09:59

miczyg1 force-pushed the txe_sb_tool branch from 935b865 to 4706227 Compare July 30, 2024 10:52

Base automatically changed from txe_sb_tool to dasharo July 30, 2024 10:53

miczyg1 force-pushed the byt_fsp_parity branch from 756c855 to 9420aa1 Compare July 30, 2024 10:56

miczyg1 commented Jul 30, 2024

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

miczyg1 force-pushed the byt_fsp_parity branch from 9420aa1 to f18af17 Compare July 30, 2024 13:45

miczyg1 mentioned this pull request Aug 2, 2024

DasharoPayloadPkg/PlatformBootManagerLib: Add missing BlParseLib in the INF file Dasharo/edk2#156

Merged

miczyg1 force-pushed the byt_fsp_parity branch 4 times, most recently from 8f8275c to ac6ae27 Compare August 7, 2024 14:04

krystian-hebel requested changes Aug 13, 2024

View reviewed changes

miczyg1 force-pushed the byt_fsp_parity branch 2 times, most recently from 8d7d3a3 to 5557b1a Compare August 14, 2024 13:12

filipleple force-pushed the byt_fsp_parity branch from bff6f1a to 59d0b38 Compare August 19, 2024 16:23

miczyg1 force-pushed the byt_fsp_parity branch 2 times, most recently from e31753a to ebd381f Compare September 24, 2024 15:06

miczyg1 added 2 commits October 8, 2024 17:16

soc/intel/baytrail: Change NC GPIO to input pull up 20K

abc6d65

All Intel SOCs use input pullup 20K as not connected GPIO. Also GPIO output mode may be harmful. Signed-off-by: Michał Żygowski <[email protected]>

southbridge/intel/common/firmware: Use the ME file path directly

9b626ff

Fixes a problem where changing ME binary in the config would not result in the desired ME binary be included in the rebuilt coreboot binary. Signed-off-by: Michał Żygowski <[email protected]>

miczyg1 temporarily deployed to NovaCustom October 9, 2024 12:20 — with GitHub Actions Inactive

miczyg1 temporarily deployed to NovaCustom October 9, 2024 12:31 — with GitHub Actions Inactive

miczyg1 commented Oct 15, 2024

View reviewed changes

src/soc/intel/baytrail/Kconfig Outdated Show resolved Hide resolved

soc/intel/baytrail/Kconfig: Fix grammar

9e444b9

miczyg1 temporarily deployed to NovaCustom October 15, 2024 13:34 — with GitHub Actions Inactive

krystian-hebel reviewed Oct 15, 2024

View reviewed changes

krystian-hebel requested changes Oct 15, 2024

View reviewed changes

miczyg1 added 4 commits October 17, 2024 10:59

src/soc/intel/baytrail/Makefile.mk: Remove excessive comma

4a0e6c1

Signed-off-by: Michał Żygowski <[email protected]>

src/soc/intel/baytrail/Kconfig: Fix typo

c23fdbe

Signed-off-by: Michał Żygowski <[email protected]>

src/soc/intel/baytrail/txei.c: Do not break printk string

2a5d4c8

Signed-off-by: Michał Żygowski <[email protected]>

src/soc/intel/baytrail/txei.c: Refactor slot calculation

6a4524e

Signed-off-by: Michał Żygowski <[email protected]>

miczyg1 temporarily deployed to NovaCustom October 17, 2024 09:01 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MinnowBoard MRC based support #542

MinnowBoard MRC based support #542

miczyg1 commented Jul 25, 2024

miczyg1 commented Jul 30, 2024

krystian-hebel Aug 13, 2024

miczyg1 Aug 14, 2024

krystian-hebel Oct 15, 2024

miczyg1 Oct 15, 2024

krystian-hebel Aug 13, 2024

miczyg1 Aug 14, 2024

miczyg1 Aug 14, 2024 •

edited

Loading

filipleple Aug 19, 2024

krystian-hebel Oct 4, 2024

miczyg1 Oct 7, 2024

miczyg1 Oct 8, 2024

krystian-hebel Oct 15, 2024

miczyg1 Oct 17, 2024

pietrushnic commented Aug 13, 2024

BeataZdunczyk commented Aug 14, 2024

miczyg1 commented Aug 14, 2024

krystian-hebel Oct 15, 2024

miczyg1 Oct 17, 2024

krystian-hebel Oct 15, 2024

krystian-hebel Oct 15, 2024

miczyg1 Oct 17, 2024

krystian-hebel Oct 15, 2024

miczyg1 Oct 17, 2024

miczyg1 Oct 17, 2024

krystian-hebel Oct 15, 2024

miczyg1 Oct 17, 2024

	# of space after microcode till the ned of ROM. Make a small gap of 256 bytes
	# of space after microcode till the end of ROM. Make a small gap of 256 bytes

MinnowBoard MRC based support #542

Are you sure you want to change the base?

MinnowBoard MRC based support #542

Conversation

miczyg1 commented Jul 25, 2024

miczyg1 commented Jul 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miczyg1 Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietrushnic commented Aug 13, 2024

BeataZdunczyk commented Aug 14, 2024

miczyg1 commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miczyg1 Aug 14, 2024 •

edited

Loading