Change in coreboot[master]: cbfs: Pull handling of the CBFS_CACHE mem_pool into CBFS core

[Julius Werner (Code Review)]

Attention is currently required from: Furquan Shaikh, Aaron Durbin.
Hello Furquan Shaikh, Aaron Durbin,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: cbfs: Pull handling of the CBFS_CACHE mem_pool into CBFS core
......................................................................

cbfs: Pull handling of the CBFS_CACHE mem_pool into CBFS core

This patch pulls control of the memory pool serving allocations from the
CBFS_CACHE memlayout area into cbfs.c and makes it a core part of the
CBFS API. Previously, platforms would independently instantiate this as
part of boot_device_ro() (mostly through cbfs_spi.c). The new cbfs_cache
pool is exported as a global so these platforms can still use it to
directly back rdev_mmap() on their boot device, but the cbfs_cache can
now also use it to directly make allocations itself. This is used to
allow transparent decompression support in cbfs_map().

Signed-off-by: Julius Werner 
Change-Id: I0d52b6a8f582a81a19fd0fd663bb89eab55a49d9
---
M src/arch/x86/memlayout.ld
M src/commonlib/include/commonlib/region.h
M src/commonlib/region.c
M src/drivers/spi/cbfs_spi.c
M src/include/cbfs.h
M src/lib/cbfs.c
M src/mainboard/sifive/hifive-unleashed/media.c
M src/soc/samsung/exynos5250/spi.c
M src/soc/samsung/exynos5420/spi.c
9 files changed, 84 insertions(+), 46 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/49333/1

diff --git a/src/arch/x86/memlayout.ld b/src/arch/x86/memlayout.ld
index 52694fa..6973124 100644
--- a/src/arch/x86/memlayout.ld
+++ b/src/arch/x86/memlayout.ld
@@ -36,6 +36,17 @@
 #elif ENV_POSTCAR
    POSTCAR(32M, 1M)
 #endif
+
+	/*
+	 * x86 devices are unlikely to need this, but we have memory for days so
+	 * might as well provide it. car.ld above may have moved the location
+	 * counter well out of the way, so need to reset it here to bypass
+	 * memlayout assertions. At some point, this script should be rewritten
+	 * to avoid all the conditionals and define a single contiguous layout
+	 * for all stages, in the right order.
+	 */
+	. = 36M;
+	POSTRAM_CBFS_CACHE(36M, 12M)
 }

#if ENV_BOOTBLOCK
diff --git a/src/commonlib/include/commonlib/region.h b/src/commonlib/include/commonlib/region.h
index 4d095b7..0080c44 100644
--- a/src/commonlib/include/commonlib/region.h
+++ b/src/commonlib/include/commonlib/region.h
@@ -196,18 +196,16 @@
    	MEM_REGION_DEV_INIT(base_, size_, &mem_rdev_rw_ops)	\

struct mmap_helper_region_device {
-	struct mem_pool pool;
+	struct mem_pool *pool;
    struct region_device rdev;
 };

-#define MMAP_HELPER_REGION_INIT(ops_, offset_, size_)			\
+#define MMAP_HELPER_DEV_INIT(ops_, offset_, size_, mpool_)		\
    {								\
    	.rdev = REGION_DEV_INIT((ops_), (offset_), (size_)),	\
+		.pool = (mpool_),					\
    }

-void mmap_helper_device_init(struct mmap_helper_region_device *mdev,
-				void *cache, size_t cache_size);
-
 void *mmap_helper_rdev_mmap(const struct region_device *, size_t, size_t);
 int mmap_helper_rdev_munmap(const struct region_device *, void *);

diff --git a/src/commonlib/region.c b/src/commonlib/region.c
index a10702a..55c0679 100644
--- a/src/commonlib/region.c
+++ b/src/commonlib/region.c
@@ -287,12 +287,6 @@
    .eraseat = mdev_eraseat,
 };

-void mmap_helper_device_init(struct mmap_helper_region_device *mdev,
-				void *cache, size_t cache_size)
-{
-	mem_pool_init(&mdev->pool, cache, cache_size);
-}
-
 void *mmap_helper_rdev_mmap(const struct region_device *rd, size_t offset,
    			size_t size)
 {
@@ -301,13 +295,13 @@

mdev = container_of((void *)rd, __typeof__(*mdev), rdev);

-	mapping = mem_pool_alloc(&mdev->pool, size);
+	mapping = mem_pool_alloc(mdev->pool, size);

if (mapping == NULL)
    	return NULL;

if (rd->ops->readat(rd, mapping, offset, size) != size) {
-		mem_pool_free(&mdev->pool, mapping);
+		mem_pool_free(mdev->pool, mapping);
    	return NULL;
    }

@@ -320,7 +314,7 @@

mdev = container_of((void *)rd, __typeof__(*mdev), rdev);

-	mem_pool_free(&mdev->pool, mapping);
+	mem_pool_free(mdev->pool, mapping);

return 0;
 }
diff --git a/src/drivers/spi/cbfs_spi.c b/src/drivers/spi/cbfs_spi.c
index 0034460..7e648a3 100644
--- a/src/drivers/spi/cbfs_spi.c
+++ b/src/drivers/spi/cbfs_spi.c
@@ -7,6 +7,7 @@
  */

#include <boot_device.h>
+#include <cbfs.h>
 #include <console/console.h>
 #include <spi_flash.h>
 #include <symbols.h>
@@ -77,21 +78,7 @@
 };

static struct mmap_helper_region_device mdev =
-	MMAP_HELPER_REGION_INIT(&spi_ops, 0, CONFIG_ROM_SIZE);
-
-static void switch_to_postram_cache(int unused)
-{
-	/*
-	 * Call boot_device_init() to ensure spi_flash is initialized before
-	 * backing mdev with postram cache. This prevents the mdev backing from
-	 * being overwritten if spi_flash was not accessed before dram was up.
-	 */
-	boot_device_init();
-	if (_preram_cbfs_cache != _postram_cbfs_cache)
-		mmap_helper_device_init(&mdev, _postram_cbfs_cache,
-					REGION_SIZE(postram_cbfs_cache));
-}
-ROMSTAGE_CBMEM_INIT_HOOK(switch_to_postram_cache);
+	MMAP_HELPER_DEV_INIT(&spi_ops, 0, CONFIG_ROM_SIZE, &cbfs_cache);

void boot_device_init(void)
 {
@@ -105,8 +92,6 @@
    	return;

spi_flash_init_done = true;
-
-	mmap_helper_device_init(&mdev, _cbfs_cache, REGION_SIZE(cbfs_cache));
 }

/* Return the CBFS boot device. */
diff --git a/src/include/cbfs.h b/src/include/cbfs.h
index e08af56..8aaff39 100644
--- a/src/include/cbfs.h
+++ b/src/include/cbfs.h
@@ -5,6 +5,7 @@

#include <cbmem.h>
 #include <commonlib/cbfs.h>
+#include <commonlib/mem_pool.h>
 #include <program_loading.h>
 #include <types.h>
 #include <vb2_sha.h>
@@ -26,7 +27,7 @@

/* Removes a previously allocated CBFS mapping. Should try to unmap mappings in strict LIFO
    order where possible, since mapping backends often don't support more complicated cases. */
-int cbfs_unmap(void *mapping);
+void cbfs_unmap(void *mapping);

/* Load a file from CBFS into a buffer. Returns amount of loaded bytes on success or 0 on error.
    File will get decompressed as necessary. */
@@ -38,10 +39,15 @@
 /* Load stage into memory filling in prog. Return 0 on success. < 0 on error. */
 int cbfs_prog_stage_load(struct prog *prog);

+
 /**********************************************************************************************
  *                                BOOT DEVICE HELPER APIs                                     *
  **********************************************************************************************/

+/* The shared memory pool for backing mapped CBFS files, and other CBFS allocation needs.
+   On x86 platforms this is only available in post-RAM stages. */
+extern struct mem_pool cbfs_cache;
+
 /*
  * Data structure that represents "a" CBFS boot device, with optional metadata cache. Generally
  * we only have one of these, or two (RO and RW) when CONFIG(VBOOT) is set. The region device
@@ -95,6 +101,7 @@
 size_t cbfs_load_and_decompress(const struct region_device *rdev, size_t offset,
    size_t in_size, void *buffer, size_t buffer_size, uint32_t compression);

+
 /**********************************************************************************************
  *                         INTERNAL HELPERS FOR INLINES, DO NOT USE.                          *
  **********************************************************************************************/
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 77c692d..be1f9fa 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -18,6 +18,20 @@
 #include <symbols.h>
 #include <timestamp.h>

+#define CBFS_CACHE_AVAILABLE ENV_STAGE_HAS_DATA_SECTION
+
+#if CBFS_CACHE_AVAILABLE
+struct mem_pool cbfs_cache = MEM_POOL_INIT(_cbfs_cache, REGION_SIZE(cbfs_cache));
+
+static void switch_to_postram_cache(int unused)
+{
+	if (_preram_cbfs_cache != _postram_cbfs_cache)
+		mem_pool_init(&cbfs_cache, _postram_cbfs_cache,
+			      REGION_SIZE(postram_cbfs_cache));
+}
+ROMSTAGE_CBMEM_INIT_HOOK(switch_to_postram_cache);
+#endif
+
 cb_err_t cbfs_boot_lookup(const char *name, bool force_ro,
    		  union cbfs_mdata *mdata, struct region_device *rdev)
 {
@@ -100,14 +114,47 @@
    if (size_out != NULL)
    	*size_out = region_device_sz(&rdev);

-	return rdev_mmap_full(&rdev);
+	uint32_t compression = CBFS_COMPRESS_NONE;
+	const struct cbfs_file_attr_compression *attr = cbfs_find_attr(&mdata,
+				CBFS_FILE_ATTR_TAG_COMPRESSION, sizeof(*attr));
+	if (attr)
+		compression = be32toh(attr->compression);
+
+	if (compression == CBFS_COMPRESS_NONE)
+		return rdev_mmap_full(&rdev);
+
+	if (!CBFS_CACHE_AVAILABLE) {
+		ERROR("Cannot map compressed file %s in x86 pre-RAM stage\n", mdata.h.filename);
+		return NULL;
+	}
+
+	size_t size = be32toh(attr->decompressed_size);
+	void *buf = mem_pool_alloc(&cbfs_cache, size);
+	if (!buf) {
+		ERROR("CBFS cache out of memory when mapping %s\n", mdata.h.filename);
+		return NULL;
+	}
+
+	size = cbfs_load_and_decompress(&rdev, 0, region_device_sz(&rdev), buf, size,
+					compression);
+	if (!size)
+		return NULL;
+
+	if (size_out != NULL)
+		*size_out = size;
+	return buf;
 }

-int cbfs_unmap(void *mapping)
+void cbfs_unmap(void *mapping)
 {
-	/* This works because munmap() only works on the root rdev and never cares about which
-	   chained subregion something was mapped from. */
-	return rdev_munmap(boot_device_ro(), mapping);
+	/*
+	 * This is save to call with mappings that weren't allocated in the cache (e.g. x86
+	 * direct mappings) -- mem_pool_free() just does nothing for addresses it doesn't
+	 * recognize. This hardcodes the assumption that if platforms implement an rdev_mmap()
+	 * that requires a free() for the boot_device, they need to implement it via the
+	 * cbfs_cache mem_pool.
+	 */
+	mem_pool_free(&cbfs_cache, mapping);
 }

int cbfs_locate_file_in_region(struct cbfsf *fh, const char *region_name,
diff --git a/src/mainboard/sifive/hifive-unleashed/media.c b/src/mainboard/sifive/hifive-unleashed/media.c
index 2ff375a..bad9516 100644
--- a/src/mainboard/sifive/hifive-unleashed/media.c
+++ b/src/mainboard/sifive/hifive-unleashed/media.c
@@ -42,7 +42,7 @@
 };

static struct mmap_helper_region_device sd_mdev =
-	MMAP_HELPER_REGION_INIT(&unleashed_sd_ops, 0, CONFIG_ROM_SIZE);
+	MMAP_HELPER_DEV_INIT(&unleashed_sd_ops, 0, CONFIG_ROM_SIZE, &cbfs_cache);

const struct region_device *boot_device_ro(void)
 {
@@ -79,8 +79,6 @@
    }
    if (MSEL_SPI2SD(m)) {
    	spi_sdcard_init(&card, 2, 0);
-		mmap_helper_device_init(&sd_mdev,
-			_cbfs_cache, REGION_SIZE(cbfs_cache));
    	return;
    }
    die("Wrong configuration of MSEL\n");
diff --git a/src/soc/samsung/exynos5250/spi.c b/src/soc/samsung/exynos5250/spi.c
index 879d04a..aefbfd6 100644
--- a/src/soc/samsung/exynos5250/spi.c
+++ b/src/soc/samsung/exynos5250/spi.c
@@ -3,6 +3,7 @@
 #include <device/mmio.h>
 #include <assert.h>
 #include <boot_device.h>
+#include <cbfs.h>
 #include <console/console.h>
 #include <soc/clk.h>
 #include <soc/gpio.h>
@@ -160,13 +161,11 @@
 };

static struct mmap_helper_region_device mdev =
-	MMAP_HELPER_REGION_INIT(&exynos_spi_ops, 0, CONFIG_ROM_SIZE);
+	MMAP_HELPER_DEV_INIT(&exynos_spi_ops, 0, CONFIG_ROM_SIZE, &cbfs_cache);

void exynos_init_spi_boot_device(void)
 {
    boot_slave_regs = (void *)EXYNOS5_SPI1_BASE;
-
-	mmap_helper_device_init(&mdev, _cbfs_cache, REGION_SIZE(cbfs_cache));
 }

const struct region_device *exynos_spi_boot_device(void)
diff --git a/src/soc/samsung/exynos5420/spi.c b/src/soc/samsung/exynos5420/spi.c
index 0a2f027..2fe9d5c 100644
--- a/src/soc/samsung/exynos5420/spi.c
+++ b/src/soc/samsung/exynos5420/spi.c
@@ -2,6 +2,7 @@

#include <device/mmio.h>
 #include <assert.h>
+#include <cbfs.h>
 #include <console/console.h>
 #include <soc/cpu.h>
 #include <soc/spi.h>
@@ -268,13 +269,11 @@
 };

static struct mmap_helper_region_device mdev =
-	MMAP_HELPER_REGION_INIT(&exynos_spi_ops, 0, CONFIG_ROM_SIZE);
+	MMAP_HELPER_DEV_INIT(&exynos_spi_ops, 0, CONFIG_ROM_SIZE, &cbfs_cache);

void exynos_init_spi_boot_device(void)
 {
    boot_slave = &exynos_spi_slaves[1];
-
-	mmap_helper_device_init(&mdev, _cbfs_cache, REGION_SIZE(cbfs_cache));
 }

const struct region_device *exynos_spi_boot_device(void)