[coreboot-gerrit] [M] Change in coreboot[main]: Support CCB at a fixed SPI-flash offset

[Simon Glass (Code Review)]

Simon Glass has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/83293?usp=email )

Change subject: Support CCB at a fixed SPI-flash offset
......................................................................

Support CCB at a fixed SPI-flash offset

Enhance the CCB to allow it to be in an FMAP region, instead of embedded
in the bootblock. This adds quite a bit of logic, particularly in
cbfstool which must now support adjusting the CCB region automatically.

This creates a CCB region in the FMAP containing the CCB. This is added
to the ROM during the build.
BUG=b:172341184, b:262546009, b:249105972
BRANCH=none
TEST=manually test each of the three options using:
make menuconfig; (select option); rm -r build && make -j30
qemu-system-x86_64 -bios build/coreboot.rom -nographic
(see that console appears)
build/util/cbfstool/cbfstool build/coreboot.rom
    configure -n console -V quiet
qemu-system-x86_64 -bios build/coreboot.rom -nographic
(see that console is suppressed, except for first part of bootblock with
CCB_CBFS and CCB_FMAP)

Change-Id: I8abc5ba55d75a3defdea548fffcedba74d4737c2
Signed-off-by: Simon Glass 
---
M Documentation/technotes/ccb.md
M Makefile.mk
M src/Kconfig
M src/commonlib/include/commonlib/ccb.h
M src/commonlib/include/commonlib/region.h
M src/lib/bootblock.c
M src/lib/ccb.c
M util/cbfstool/cbfstool.c
M util/cbfstool/default-x86.fmd
9 files changed, 145 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/83293/1

diff --git a/Documentation/technotes/ccb.md b/Documentation/technotes/ccb.md
index bdb9b18..63cca83 100644
--- a/Documentation/technotes/ccb.md
+++ b/Documentation/technotes/ccb.md
@@ -84,11 +84,22 @@
 the CCB_CBFS option. This applies mostly to AMD, since Intel platforms, do not
 have a signed bootblock.

-## Future extensions
+For boards where it is not possible or desirable to access CBFS files early in
+bootblock, an FMAP region can be used to hold the CCB. Care should be taken that
+this region is in read-only flash, if preventing users from changing it is
+important.

-CCB could be stored in an FMAP region.
+If something goes wrong with the CCB init while the console is off, the
+ccb_check() function can be called to see what happened.
+
+## Future extensions

The CMOS option feature (in `include/option.h`) could be expanded to provide an
 API for CCB. Using strings for options might be too inefficient for bootblock,
 so an enum could be introduced for common option types, with a lookup table used
 to convert between strings and integers.
+
+The initial bootblock output could be suppressed, even with the CCB_CBFS and
+CCB_FMAP options, by just living with the risk that something goes wrong in
+setting up FMAP / CBFS. This would be best handled by enabling the CCB option
+only when the board is working correctly.
diff --git a/Makefile.mk b/Makefile.mk
index 16b6539..077898f 100644
--- a/Makefile.mk
+++ b/Makefile.mk
@@ -485,6 +485,10 @@
 CPPFLAGS_common += -D__BUILD_DIR__="$(obj)"
 CPPFLAGS_common += -D__COREBOOT__

+# Must be >= sizeof(struct ccb)
+CCB_SIZE := 16
+CPPFLAGS_common += -DCCB_SIZE=$(CCB_SIZE)
+
 ifeq ($(BUILD_TIMELESS),1)
 CPPFLAGS_common += -D__TIMELESS__
 endif
@@ -1078,6 +1082,17 @@
 FMAP_SPD_CACHE_ENTRY :=
 endif

+# Align CCB end so that FMAP doesn't end up in a strange place and bootblock
+# won't fit in what remains
+ifeq ($(CONFIG_CCB_FMAP),y)
+FMAP_CCB_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x10)
+FMAP_CCB_SIZE := $(call int-align, $(CCB_SIZE), 0x200)
+FMAP_CCB_ENTRY := CCB@$(FMAP_CCB_BASE) $(FMAP_CCB_SIZE)
+FMAP_CURRENT_BASE := $(call int-add, $(FMAP_CCB_BASE) $(FMAP_CCB_SIZE))
+else
+FMAP_CCB_ENTRY :=
+endif
+
 ifeq ($(CONFIG_VPD),y)
 FMAP_VPD_BASE := $(call int-align, $(FMAP_CURRENT_BASE), 0x4000)
 FMAP_VPD_SIZE := $(CONFIG_VPD_FMAP_SIZE)
@@ -1173,6 +1188,7 @@
        -e "s,##MRC_CACHE_ENTRY##,$(FMAP_MRC_CACHE_ENTRY)," \
        -e "s,##SMMSTORE_ENTRY##,$(FMAP_SMMSTORE_ENTRY)," \
        -e "s,##SPD_CACHE_ENTRY##,$(FMAP_SPD_CACHE_ENTRY)," \
+	    -e "s,##CCB_ENTRY##,$(FMAP_CCB_ENTRY)," \
        -e "s,##VPD_ENTRY##,$(FMAP_VPD_ENTRY)," \
        -e "s,##HSPHY_FW_ENTRY##,$(FMAP_HSPHY_FW_ENTRY)," \
        -e "s,##CBFS_BASE##,$(FMAP_CBFS_BASE)," \
@@ -1203,6 +1219,10 @@
 add_bootblock = $(CBFSTOOL) $(1) write -u -r BOOTBLOCK -f $(2)
 endif

+ifneq ($(CONFIG_ARCH_X86),)
+add_ccb = $(CBFSTOOL) $(1) write -u -r CCB -f $(2)
+endif
+
 # coreboot.pre doesn't follow the standard Make conventions. It gets modified
 # by multiple rules, and thus we can't compute the dependencies correctly.
 $(shell rm -f $(obj)/coreboot.pre)
@@ -1212,6 +1232,12 @@
    $(CBFSTOOL) $@.tmp create -M $(obj)/fmap.fmap -r $(shell cat $(obj)/fmap.desc)
    printf "    BOOTBLOCK\n"
    $(call add_bootblock,$@.tmp,$(objcbfs)/bootblock.bin)
+ifneq ($(CONFIG_CCB_FMAP),)
+	# Force use of shell so these hex values work
+	/usr/bin/printf "\x01\xb0\x43\xc0" >$(objcbfs)/ccb.bin
+	truncate -s $(CCB_SIZE) $(objcbfs)/ccb.bin
+	$(call add_ccb,$@.tmp,$(objcbfs)/ccb.bin)
+endif
    $(prebuild-files) true
    mv $@.tmp $@
 else # ifneq ($(CONFIG_UPDATE_IMAGE),y)
diff --git a/src/Kconfig b/src/Kconfig
index bee8363..3594e60 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -616,6 +616,18 @@

See Documentation/util/cbfstool/ccb.md for more information.

+config CCB_FMAP
+	bool "Read coreboot control block from fixed ROM offset"
+	depends on CCB
+	help
+	  Enable this to read the CCB (coreboot control block) from a fixed ROM
+	  offset. The file is read after CBFS is inited in bootblock.
+
+	  The CCB provides a few simple settings for coreboot which can be
+	  changed using the 'cbfstool set-ccb' command.
+
+	  See Documentation/util/cbfstool/ccb.md for more information.
+
 endchoice

menu "Software Bill Of Materials (SBOM)"
diff --git a/src/commonlib/include/commonlib/ccb.h b/src/commonlib/include/commonlib/ccb.h
index b77a256..26bdb8d 100644
--- a/src/commonlib/include/commonlib/ccb.h
+++ b/src/commonlib/include/commonlib/ccb.h
@@ -25,6 +25,12 @@
 /* Magic number at the top of the CCB and used to detect it in the bootblock */
 #define CCB_MAGIC	0xc043b001

+/* Name of CCB FMAP region, if CONFIG_CCB_FMAP is enabled */
+#define CCB_REGION	"CCB"
+
+/* Assumed maximum size of CCB (can be larger than sizeof(struct ccb) */
+#define CCB_MAX_SIZE	0x10
+
 /**
  * struct ccb - Data in the CCB
  *
diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h
index 25efcc8..a224cb6 100644
--- a/src/commonlib/include/commonlib/region.h
+++ b/src/commonlib/include/commonlib/region.h
@@ -94,6 +94,11 @@
    	},					\
    }

+static inline bool rdev_valid(const struct region_device *rdev)
+{
+	return rdev->ops != NULL;
+}
+
 /* Helper to dynamically initialize region device. */
 void region_device_init(struct region_device *rdev,
    		const struct region_device_ops *ops, size_t offset,
diff --git a/src/lib/bootblock.c b/src/lib/bootblock.c
index 454e538..9284c66 100644
--- a/src/lib/bootblock.c
+++ b/src/lib/bootblock.c
@@ -60,7 +60,7 @@
    	exception_init();
    }

-	/* late init of CCB for when CCB is in CBFS */
+	/* late init of CCB for when CCB is in CBFS or FMAP */
    if (!ENV_HOLDS_CCB) {
    	ccb_init();

diff --git a/src/lib/ccb.c b/src/lib/ccb.c
index c45ea04..0c61a23 100644
--- a/src/lib/ccb.c
+++ b/src/lib/ccb.c
@@ -5,6 +5,7 @@
 #include <cbmem.h>
 #include <commonlib/ccb_api.h>
 #include <console/console.h>
+#include <fmap.h>
 #include <program_loading.h>
 #include <string.h>
 #include <symbols.h>
@@ -16,6 +17,12 @@
 };
 #endif

+_Static_assert(sizeof(struct ccb) <= CCB_SIZE,
+	       "CCB_SIZE must be no smaller than sizeof(struct ccb)");
+
+/* copy of CCB when read from an rdev */
+static struct ccb ccb_holder;
+
 /* holds the global pointer to the coreboot Control Block, NULL if none */
 static const struct ccb *ccb_glob;

@@ -49,7 +56,7 @@
 CBMEM_READY_HOOK(add_ccb_to_cbmem);

/* Get a pointer to the CCB. Returns NULL if not found */
-static struct ccb *locate_ccb(void)
+static struct ccb *locate_ccb(struct region_device *rdev)
 {
    struct ccb *ccb;

@@ -62,6 +69,7 @@
     *    entry so it is accessible through the post-RAM stages
     * 3. (ramstage) The CCB is in CBMEM. It can be accessed directly.
     */
+	memset(rdev, '\0', sizeof(*rdev));
 #if ENV_HOLDS_CCB
    ccb = &ccb_static;
    if (REGION_SIZE(ccb) < sizeof(*ccb))
@@ -79,18 +87,28 @@
    		printk(BIOS_ERR, "CCB: Not found in CBMEM\n");
    } else if (CONFIG(CCB_CBFS)) {
    	struct prog ccb_file = PROG_INIT(PROG_CCB, "ccb");
-		struct region_device rdev;
    	union cbfs_mdata mdata;

-		if (_cbfs_boot_lookup(prog_name(&ccb_file), true, &mdata, &rdev)) {
+		if (_cbfs_boot_lookup(prog_name(&ccb_file), true, &mdata, rdev)) {
    		printk(BIOS_DEBUG, "CCB: No file in CBFS\n");
    		return NULL;
    	}
-		if (region_device_sz(&rdev) != sizeof(struct ccb)) {
+		if (region_device_sz(rdev) != sizeof(struct ccb)) {
    		printk(BIOS_ERR, "CCB: Incorrect file size in CBFS\n");
    		return NULL;
    	}
-		ccb = rdev_mmap_full(&rdev);
+		ccb = rdev_mmap_full(rdev);
+	} else if (CONFIG(CCB_FMAP)) {
+		if (fmap_locate_area_as_rdev(CCB_REGION, rdev)) {
+			printk(BIOS_ERR, "CCB: Not found in FMAP\n");
+			return NULL;
+		}
+		ccb = rdev_mmap_full(rdev);
+		if (!ccb) {
+			printk(BIOS_ERR, "CCB: Cannot map\n");
+			return NULL;
+		}
+		printk(BIOS_DEBUG, "CCB: Found in FMAP\n");
    } else {
    	/* we cannot get here */
    	BUG();
@@ -102,24 +120,35 @@

void ccb_check(void)
 {
+	struct region_device rdev;
    struct ccb *ccb;

-	ccb = locate_ccb();
-	if (ccb)
+	ccb = locate_ccb(&rdev);
+	if (ccb) {
    	printk(BIOS_DEBUG, "CCB: ready\n");
+		if (rdev_valid(&rdev))
+			rdev_munmap(&rdev, ccb);
+	}
 }

void ccb_init(void)
 {
+	struct region_device rdev;
    struct ccb *ccb;

-	ccb = locate_ccb();
+	ccb = locate_ccb(&rdev);
    if (ccb) {
 #if ENV_BOOTBLOCK
    	/* Copy the CCB into the cache for use by romstage. In the event
    	 * that CCB is missing, zero values will be used */
    	memcpy((void *)_ccb, ccb, sizeof(*ccb));
 #endif
+
+		if (rdev_valid(&rdev)) {
+			ccb_holder = *ccb;
+			rdev_munmap(&rdev, ccb);
+			ccb = &ccb_holder;
+		}
    	ccb_glob = ccb;
    }
 }
diff --git a/util/cbfstool/cbfstool.c b/util/cbfstool/cbfstool.c
index 987f71e..57fbc6b6 100644
--- a/util/cbfstool/cbfstool.c
+++ b/util/cbfstool/cbfstool.c
@@ -13,6 +13,7 @@
 #include "cbfs_image.h"
 #include "cbfs_sections.h"
 #include "elfparsing.h"
+#include "fmap.h"
 #include "partitioned_file.h"
 #include "lz4/lib/xxhash.h"
 #include <commonlib/bsd/cbfs_private.h>
@@ -676,13 +677,20 @@
    return ret;
 }

+/* location of CCB, with respect to the image regions (>0 is an error) */
+enum ccb_location {
+	CCB_IN_PRIMARY_CBFS	= 0,
+	CCB_IN_OWN_REGION	= -1,
+	CCB_NOT_FOUND		= 1
+};
+
 /**
  * locate_ccb() - Locate the coreboot Control Block in the CBFS image
  *
  * This is located in the bootblock and has a special signature
  *
  * @ccbp: Returns a pointer to the CCB
- * Return: 0 if OK, 1 on error
+ * Return: 0 if OK, 1 on error, -1 if the CCB region must be written back
  */
 static int locate_ccb(struct buffer *buffer, struct ccb **ccbp)
 {
@@ -695,10 +703,12 @@
    	goto no_bootblock;

if (fmap_find_area(fmap, SECTION_NAME_BOOTBLOCK)) {
+		INFO("Reading bootblock\n");
    	if (!partitioned_file_read_region(buffer, param.image_file,
    					  SECTION_NAME_BOOTBLOCK))
    		goto no_bootblock;
    } else {
+		INFO("Reading CBFS\n");
    	if (!partitioned_file_read_region(buffer, param.image_file,
    					  SECTION_NAME_PRIMARY_CBFS))
    		goto no_bootblock;
@@ -708,22 +718,37 @@
    size = buffer_size(buffer);

for (ptr = (uint32_t *)data, end = (uint32_t *)(data + size); ptr < end;
-	     ptr++) {
+	ptr++) {
    	if (*ptr == CCB_MAGIC) {
    		*ccbp = (struct ccb *)ptr;
-			return 0;
+			INFO("CCB at %p, from size %lx\n", ptr, size);
+			return CCB_IN_PRIMARY_CBFS;
    	}
    }

INFO("CCB not in bootblock\n");

+	/* Now try FMAP */
+	const struct fmap_area *area;
+
+	area = fmap_find_area(fmap, CCB_REGION);
+	if (area) {
+		printk(BIOS_DEBUG, "CCB: Found in FMAP\n");
+		if (!partitioned_file_read_region(buffer, param.image_file,
+						  CCB_REGION))
+			goto no_bootblock;
+
+		*ccbp = (void *)buffer_get(buffer); // + area->offset;
+		return CCB_IN_OWN_REGION;
+	}
+
    struct cbfs_image image;
    struct cbfs_file *ccb_file;
    const char *filename = "ccb";
    struct buffer ccb_buf;

if (cbfs_image_from_buffer(&image, param.image_region, param.headeroffset))
-		return 1;
+		return CCB_NOT_FOUND;

ccb_file = cbfs_get_entry(&image, filename);
    if (!ccb_file) {
@@ -734,7 +759,7 @@
    	INFO("CCB not in CBFS: creating\n");

if (buffer_create(&ccb_buf, sizeof(struct ccb), filename))
-			return 1;
+			return CCB_NOT_FOUND;

header = cbfs_create_file_header(CBFS_TYPE_RAW, ccb_buf.size,
    					 filename);
@@ -748,24 +773,24 @@
    	buffer_delete(&ccb_buf);
    	if (ret) {
    		ERROR("Failed to add '%s' into ROM image.\n", filename);
-			return 1;
+			return CCB_NOT_FOUND;
    	}
    	ccb_file = cbfs_get_entry(&image, filename);
    }

if (!ccb_file) {
    	ERROR("Cannot get file\n");
-		return 1;
+		return CCB_NOT_FOUND;
    }

/* Locate the CCB */
    *ccbp = (void *)ccb_file + be32toh(ccb_file->offset);

-	return 0;
+	return CCB_IN_PRIMARY_CBFS;

no_bootblock:
    ERROR("CCB not in ROM image?!?\n");
-	return 1;
+	return CCB_NOT_FOUND;
 }

/**
@@ -777,11 +802,13 @@
  */
 static int cbfs_ccb_set_value(unused const char *name, unused const char *value)
 {
+	enum ccb_location ccb_loc;
    struct buffer buffer;
    struct ccb *ccb;
    uint val;

-	if (locate_ccb(&buffer, &ccb))
+	ccb_loc = locate_ccb(&buffer, &ccb);
+	if (ccb_loc > 0)
    	return 1;

/*
@@ -803,6 +830,12 @@
    printf("%s=%s\n", name, value);
    ccb->flags = val;

+	if (ccb_loc == CCB_IN_OWN_REGION) {
+		INFO("Performing operation on '%s' region...\n", CCB_REGION);
+		if (!partitioned_file_write_region(param.image_file, &buffer))
+			return 1;
+	}
+
    return 0;
 }

@@ -819,7 +852,7 @@
    struct buffer buffer;
    struct ccb *ccb;

-	if (locate_ccb(&buffer, &ccb))
+	if (locate_ccb(&buffer, &ccb) > 0)
    	return 1;
    /*
     * For now this code is very simple as we only have one setting. We can
diff --git a/util/cbfstool/default-x86.fmd b/util/cbfstool/default-x86.fmd
index f008889..ec57c06 100644
--- a/util/cbfstool/default-x86.fmd
+++ b/util/cbfstool/default-x86.fmd
@@ -15,6 +15,7 @@
    	##SPD_CACHE_ENTRY##
    	##VPD_ENTRY##
    	##HSPHY_FW_ENTRY##
+		##CCB_ENTRY##
    	FMAP@##FMAP_BASE## ##FMAP_SIZE##
    	COREBOOT(CBFS)@##CBFS_BASE## ##CBFS_SIZE##
    }