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/+/49331
to review the following change.
Change subject: cbfs: Move trivial wrappers to static inlines
......................................................................
cbfs: Move trivial wrappers to static inlines
The new CBFS API contains a couple of trivial wrappers that all just
call the same base functions with slightly different predetermined
arguments, and I'm planning to add several more of them as well. This
patch changes these functions to become static inlines, and reorganizes
the cbfs.h header a bit for better readability while I'm at it.
Signed-off-by: Julius Werner <jwerner(a)chromium.org>
Change-Id: If0170401b2a70c158691b6eb56c7e312553afad1
---
M src/include/cbfs.h
M src/lib/cbfs.c
2 files changed, 68 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/49331/1
diff --git a/src/include/cbfs.h b/src/include/cbfs.h
index 431f6e5..e08af56 100644
--- a/src/include/cbfs.h
+++ b/src/include/cbfs.h
@@ -9,50 +9,39 @@
#include <types.h>
#include <vb2_sha.h>
-/***********************************************
- * Perform CBFS operations on the boot device. *
- ***********************************************/
-/* Return mapping of option ROM found in boot device. NULL on error. */
-void *cbfs_boot_map_optionrom(uint16_t vendor, uint16_t device);
-/* Return mapping of option ROM with revision number. Returns NULL on error. */
-void *cbfs_boot_map_optionrom_revision(uint16_t vendor, uint16_t device, uint8_t rev);
-
-/* Locate file by name and optional type. Return 0 on success. < 0 on error. */
-int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type);
-/* Locate file in a specific region of fmap. Return 0 on success. < 0 on error*/
-int cbfs_locate_file_in_region(struct cbfsf *fh, const char *region_name,
- const char *name, uint32_t *type);
+/**********************************************************************************************
+ * CBFS FILE ACCESS APIs *
+ **********************************************************************************************/
/* Map file into memory, returning a pointer to the mapping or NULL on error. If |size_out| is
not NULL, it will pass out the size of the mapped file.
NOTE: Since this may return a direct pointer to memory-mapped hardware, compressed files are
NOT transparently decompressed (unlike cbfs_load()). */
-void *cbfs_map(const char *name, size_t *size_out);
+static inline void *cbfs_map(const char *name, size_t *size_out);
+
/* Like cbfs_map(), except that it will always read from the read-only CBFS (the "COREBOOT" FMAP
region), even when CONFIG(VBOOT) is enabled. */
-void *cbfs_ro_map(const char *name, size_t *size_out);
+static inline void *cbfs_ro_map(const char *name, size_t *size_out);
+
/* 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);
/* 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. */
-size_t cbfs_load(const char *name, void *buf, size_t buf_size);
+static inline size_t cbfs_load(const char *name, void *buf, size_t buf_size);
/* Like cbfs_load(), except that it will always read from the read-only CBFS (the "COREBOOT"
FMAP region), even when CONFIG(VBOOT) is enabled. */
-size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size);
-
-/* Load |in_size| bytes from |rdev| at |offset| to the |buffer_size| bytes large |buffer|,
- decompressing it according to |compression| in the process. Returns the decompressed file
- size, or 0 on error. LZMA files will be mapped for decompression. LZ4 files will be
- decompressed in-place with the buffer size requirements outlined in compression.h. */
-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);
+static inline size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size);
/* 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 *
+ **********************************************************************************************/
+
/*
* 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
@@ -83,4 +72,57 @@
cb_err_t cbfs_init_boot_device(const struct cbfs_boot_device *cbd,
struct vb2_hash *metadata_hash);
+
+/**********************************************************************************************
+ * LEGACY APIs, TO BE DEPRECATED/REPLACED *
+ **********************************************************************************************/
+
+/* Locate file by name and optional type. Return 0 on success. < 0 on error. */
+int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type);
+/* Locate file in a specific region of fmap. Return 0 on success. < 0 on error*/
+int cbfs_locate_file_in_region(struct cbfsf *fh, const char *region_name,
+ const char *name, uint32_t *type);
+
+/* Return mapping of option ROM found in boot device. NULL on error. */
+void *cbfs_boot_map_optionrom(uint16_t vendor, uint16_t device);
+/* Return mapping of option ROM with revision number. Returns NULL on error. */
+void *cbfs_boot_map_optionrom_revision(uint16_t vendor, uint16_t device, uint8_t rev);
+
+/* Load |in_size| bytes from |rdev| at |offset| to the |buffer_size| bytes large |buffer|,
+ decompressing it according to |compression| in the process. Returns the decompressed file
+ size, or 0 on error. LZMA files will be mapped for decompression. LZ4 files will be
+ decompressed in-place with the buffer size requirements outlined in compression.h. */
+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. *
+ **********************************************************************************************/
+size_t _cbfs_load(const char *name, void *buf, size_t buf_size, bool force_ro);
+void *_cbfs_map(const char *name, size_t *size_out, bool force_ro);
+
+
+/**********************************************************************************************
+ * INLINE IMPLEMENTATIONS *
+ **********************************************************************************************/
+static inline void *cbfs_map(const char *name, size_t *size_out)
+{
+ return _cbfs_map(name, size_out, false);
+}
+
+static inline void *cbfs_ro_map(const char *name, size_t *size_out)
+{
+ return _cbfs_map(name, size_out, true);
+}
+
+static inline size_t cbfs_load(const char *name, void *buf, size_t buf_size)
+{
+ return _cbfs_load(name, buf, buf_size, false);
+}
+
+static inline size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size)
+{
+ return _cbfs_load(name, buf, buf_size, true);
+}
+
#endif
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index d4effbf..77c692d 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -89,7 +89,7 @@
return 0;
}
-static void *_cbfs_map(const char *name, size_t *size_out, bool force_ro)
+void *_cbfs_map(const char *name, size_t *size_out, bool force_ro)
{
struct region_device rdev;
union cbfs_mdata mdata;
@@ -103,16 +103,6 @@
return rdev_mmap_full(&rdev);
}
-void *cbfs_map(const char *name, size_t *size_out)
-{
- return _cbfs_map(name, size_out, false);
-}
-
-void *cbfs_ro_map(const char *name, size_t *size_out)
-{
- return _cbfs_map(name, size_out, true);
-}
-
int cbfs_unmap(void *mapping)
{
/* This works because munmap() only works on the root rdev and never cares about which
@@ -306,7 +296,7 @@
return cbfs_map(name, NULL);
}
-static size_t _cbfs_load(const char *name, void *buf, size_t buf_size, bool force_ro)
+size_t _cbfs_load(const char *name, void *buf, size_t buf_size, bool force_ro)
{
struct region_device rdev;
union cbfs_mdata mdata;
@@ -327,16 +317,6 @@
buf, buf_size, compression);
}
-size_t cbfs_load(const char *name, void *buf, size_t buf_size)
-{
- return _cbfs_load(name, buf, buf_size, false);
-}
-
-size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size)
-{
- return _cbfs_load(name, buf, buf_size, true);
-}
-
int cbfs_prog_stage_load(struct prog *pstage)
{
struct cbfs_stage stage;
--
To view, visit https://review.coreboot.org/c/coreboot/+/49331
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: If0170401b2a70c158691b6eb56c7e312553afad1
Gerrit-Change-Number: 49331
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-MessageType: newchange
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/+/49330
to review the following change.
Change subject: cbfs: Reflow cbfs.c and cbfs.h to 96-character line lengths
......................................................................
cbfs: Reflow cbfs.c and cbfs.h to 96-character line lengths
Doing this all in one go keeps the files consistent and should make
future refactoring easier.
Signed-off-by: Julius Werner <jwerner(a)chromium.org>
Change-Id: I4a701d24fc9ccd68dce8789aab15fd21964a55f9
---
M src/include/cbfs.h
M src/lib/cbfs.c
2 files changed, 76 insertions(+), 90 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/49330/1
diff --git a/src/include/cbfs.h b/src/include/cbfs.h
index cad01c6..431f6e5 100644
--- a/src/include/cbfs.h
+++ b/src/include/cbfs.h
@@ -17,35 +17,36 @@
void *cbfs_boot_map_optionrom(uint16_t vendor, uint16_t device);
/* Return mapping of option ROM with revision number. Returns NULL on error. */
void *cbfs_boot_map_optionrom_revision(uint16_t vendor, uint16_t device, uint8_t rev);
+
/* Locate file by name and optional type. Return 0 on success. < 0 on error. */
int cbfs_boot_locate(struct cbfsf *fh, const char *name, uint32_t *type);
-/* Map file into memory, returning a pointer to the mapping or NULL on error.
- If |size_out| is not NULL, it will pass out the size of the mapped file.
- NOTE: Since this may return a direct pointer to memory-mapped hardware,
- compressed files are NOT transparently decompressed (unlike cbfs_load()). */
-void *cbfs_map(const char *name, size_t *size_out);
-/* Like cbfs_map(), except that it will always read from the read-only CBFS
- ("COREBOOT" FMAP region), even when CONFIG(VBOOT) is enabled. */
-void *cbfs_ro_map(const char *name, size_t *size_out);
-/* 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);
/* Locate file in a specific region of fmap. Return 0 on success. < 0 on error*/
int cbfs_locate_file_in_region(struct cbfsf *fh, const char *region_name,
const char *name, uint32_t *type);
-/* 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. Same
- decompression requirements as cbfs_load_and_decompress(). */
+
+/* Map file into memory, returning a pointer to the mapping or NULL on error. If |size_out| is
+ not NULL, it will pass out the size of the mapped file.
+ NOTE: Since this may return a direct pointer to memory-mapped hardware, compressed files are
+ NOT transparently decompressed (unlike cbfs_load()). */
+void *cbfs_map(const char *name, size_t *size_out);
+/* Like cbfs_map(), except that it will always read from the read-only CBFS (the "COREBOOT" FMAP
+ region), even when CONFIG(VBOOT) is enabled. */
+void *cbfs_ro_map(const char *name, size_t *size_out);
+/* 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);
+
+/* 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. */
size_t cbfs_load(const char *name, void *buf, size_t buf_size);
-/* Like cbfs_load(), except that it will always read from the read-only CBFS
- ("COREBOOT" FMAP region), even when CONFIG(VBOOT) is enabled. */
+/* Like cbfs_load(), except that it will always read from the read-only CBFS (the "COREBOOT"
+ FMAP region), even when CONFIG(VBOOT) is enabled. */
size_t cbfs_ro_load(const char *name, void *buf, size_t buf_size);
-/* Load |in_size| bytes from |rdev| at |offset| to the |buffer_size| bytes
- * large |buffer|, decompressing it according to |compression| in the process.
- * Returns the decompressed file size, or 0 on error.
- * LZMA files will be mapped for decompression. LZ4 files will be decompressed
- * in-place with the buffer size requirements outlined in compression.h. */
+
+/* Load |in_size| bytes from |rdev| at |offset| to the |buffer_size| bytes large |buffer|,
+ decompressing it according to |compression| in the process. Returns the decompressed file
+ size, or 0 on error. LZMA files will be mapped for decompression. LZ4 files will be
+ decompressed in-place with the buffer size requirements outlined in compression.h. */
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);
@@ -53,10 +54,9 @@
int cbfs_prog_stage_load(struct prog *prog);
/*
- * 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 stored here must always be a
- * subregion of boot_device_ro().
+ * 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
+ * stored here must always be a subregion of boot_device_ro().
*/
struct cbfs_boot_device {
struct region_device rdev;
@@ -68,18 +68,17 @@
void cbfs_boot_device_find_mcache(struct cbfs_boot_device *cbd, uint32_t id);
/*
- * Retrieves the currently active CBFS boot device. If |force_ro| is set, will
- * always return the read-only CBFS instead (this only makes a difference when
- * CONFIG(VBOOT) is enabled). May perform certain CBFS initialization tasks.
- * Returns NULL on error (e.g. boot device IO error).
+ * Retrieves the currently active CBFS boot device. If |force_ro| is set, will always return the
+ * read-only CBFS instead (this only makes a difference when CONFIG(VBOOT) is enabled). May
+ * perform certain CBFS initialization tasks. Returns NULL on error (e.g. boot device IO error).
*/
const struct cbfs_boot_device *cbfs_get_boot_device(bool force_ro);
/*
- * Builds the mcache (if |cbd->mcache| is set) and verifies |metadata_hash| (if
- * it is not NULL). If CB_CBFS_CACHE_FULL is returned, the mcache is incomplete
- * but still valid and the metadata hash was still verified. Should be called
- * once per *boot* (not once per stage) before the first CBFS access.
+ * Builds the mcache (if |cbd->mcache| is set) and verifies |metadata_hash| (if it is not NULL).
+ * If CB_CBFS_CACHE_FULL is returned, the mcache is incomplete but still valid and the metadata
+ * hash was still verified. Should be called once per *boot* (not once per stage) before the
+ * first CBFS access.
*/
cb_err_t cbfs_init_boot_device(const struct cbfs_boot_device *cbd,
struct vb2_hash *metadata_hash);
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c
index 60dc15a..d4effbf 100644
--- a/src/lib/cbfs.c
+++ b/src/lib/cbfs.c
@@ -29,7 +29,7 @@
cb_err_t err = CB_CBFS_CACHE_FULL;
if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM && cbd->mcache_size)
err = cbfs_mcache_lookup(cbd->mcache, cbd->mcache_size,
- name, mdata, &data_offset);
+ name, mdata, &data_offset);
if (err == CB_CBFS_CACHE_FULL) {
struct vb2_hash *metadata_hash = NULL;
if (CONFIG(TOCTOU_SAFETY)) {
@@ -37,21 +37,17 @@
dead_code();
if (!cbd->mcache_size)
die("Cannot access CBFS TOCTOU-safely in " ENV_STRING " before CBMEM init!\n");
- /* We can only reach this for the RW CBFS -- an mcache
- overflow in the RO CBFS would have been caught when
- building the mcache in cbfs_get_boot_device().
- (Note that TOCTOU_SAFETY implies !NO_CBFS_MCACHE.) */
+ /* We can only reach this for the RW CBFS -- an mcache overflow in the
+ RO CBFS would have been caught when building the mcache in cbfs_get
+ boot_device(). (Note that TOCTOU_SAFETY implies !NO_CBFS_MCACHE.) */
assert(cbd == vboot_get_cbfs_boot_device());
/* TODO: set metadata_hash to RW metadata hash here. */
}
- err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset,
- metadata_hash);
+ err = cbfs_lookup(&cbd->rdev, name, mdata, &data_offset, metadata_hash);
}
- if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && !force_ro &&
- err == CB_CBFS_NOT_FOUND) {
- printk(BIOS_INFO, "CBFS: Fall back to RO region for %s\n",
- name);
+ if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && !force_ro && err == CB_CBFS_NOT_FOUND) {
+ printk(BIOS_INFO, "CBFS: Fall back to RO region for %s\n", name);
return cbfs_boot_lookup(name, true, mdata, rdev);
}
if (err) {
@@ -60,8 +56,7 @@
else if (err == CB_CBFS_HASH_MISMATCH)
printk(BIOS_ERR, "CBFS ERROR: metadata hash mismatch!\n");
else
- printk(BIOS_ERR,
- "CBFS ERROR: error %d when looking up '%s'\n",
+ printk(BIOS_ERR, "CBFS ERROR: error %d when looking up '%s'\n",
err, name);
return err;
}
@@ -81,8 +76,7 @@
return -1;
size_t msize = be32toh(fh->mdata.h.offset);
- if (rdev_chain(&fh->metadata, &addrspace_32bit.rdev,
- (uintptr_t)&fh->mdata, msize))
+ if (rdev_chain(&fh->metadata, &addrspace_32bit.rdev, (uintptr_t)&fh->mdata, msize))
return -1;
if (type) {
@@ -121,8 +115,8 @@
int 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. */
+ /* 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);
}
@@ -132,8 +126,7 @@
struct region_device rdev;
int ret = 0;
if (fmap_locate_area_as_rdev(region_name, &rdev)) {
- LOG("%s region not found while looking for %s\n",
- region_name, name);
+ LOG("%s region not found while looking for %s\n", region_name, name);
return -1;
}
@@ -188,14 +181,13 @@
return false;
if (ENV_ROMSTAGE && CONFIG(POSTCAR_STAGE))
return false;
- if ((ENV_ROMSTAGE || ENV_POSTCAR)
- && !CONFIG(COMPRESS_RAMSTAGE))
+ if ((ENV_ROMSTAGE || ENV_POSTCAR) && !CONFIG(COMPRESS_RAMSTAGE))
return false;
return true;
}
-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)
+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)
{
size_t out_size;
void *map;
@@ -212,8 +204,8 @@
if (!cbfs_lz4_enabled())
return 0;
- /* cbfs_stage_load_and_decompress() takes care of in-place
- lz4 decompression by setting up the rdev to be in memory. */
+ /* cbfs_stage_load_and_decompress() takes care of in-place LZ4 decompression by
+ setting up the rdev to be in memory. */
map = rdev_mmap(rdev, offset, in_size);
if (map == NULL)
return 0;
@@ -247,33 +239,31 @@
}
}
-static size_t cbfs_stage_load_and_decompress(const struct region_device *rdev,
- size_t offset, size_t in_size, void *buffer, size_t buffer_size,
- uint32_t compression)
+static size_t cbfs_stage_load_and_decompress(const struct region_device *rdev, size_t offset,
+ size_t in_size, void *buffer, size_t buffer_size, uint32_t compression)
{
struct region_device rdev_src;
if (compression == CBFS_COMPRESS_LZ4) {
if (!cbfs_lz4_enabled())
return 0;
- /* Load the compressed image to the end of the available memory
- * area for in-place decompression. It is the responsibility of
- * the caller to ensure that buffer_size is large enough
- * (see compression.h, guaranteed by cbfstool for stages). */
+ /* Load the compressed image to the end of the available memory area for
+ in-place decompression. It is the responsibility of the caller to ensure that
+ buffer_size is large enough (see compression.h, guaranteed by cbfstool for
+ stages). */
void *compr_start = buffer + buffer_size - in_size;
if (rdev_readat(rdev, compr_start, offset, in_size) != in_size)
return 0;
/* Create a region device backed by memory. */
- rdev_chain(&rdev_src, &addrspace_32bit.rdev,
- (uintptr_t)compr_start, in_size);
+ rdev_chain(&rdev_src, &addrspace_32bit.rdev, (uintptr_t)compr_start, in_size);
- return cbfs_load_and_decompress(&rdev_src, 0, in_size, buffer,
- buffer_size, compression);
+ return cbfs_load_and_decompress(&rdev_src, 0, in_size, buffer, buffer_size,
+ compression);
}
/* All other algorithms can use the generic implementation. */
- return cbfs_load_and_decompress(rdev, offset, in_size, buffer,
- buffer_size, compression);
+ return cbfs_load_and_decompress(rdev, offset, in_size, buffer, buffer_size,
+ compression);
}
static inline int tohex4(unsigned int c)
@@ -316,8 +306,7 @@
return cbfs_map(name, NULL);
}
-static size_t _cbfs_load(const char *name, void *buf, size_t buf_size,
- bool force_ro)
+static size_t _cbfs_load(const char *name, void *buf, size_t buf_size, bool force_ro)
{
struct region_device rdev;
union cbfs_mdata mdata;
@@ -389,7 +378,7 @@
}
fsize = cbfs_stage_load_and_decompress(fh, foffset, fsize, load,
- stage.memlen, stage.compression);
+ stage.memlen, stage.compression);
if (!fsize)
return -1;
@@ -421,8 +410,7 @@
} else if (ENV_ROMSTAGE_OR_BEFORE) {
u8 *boundary = _ecbfs_mcache - REGION_SIZE(cbfs_mcache) *
CONFIG_CBFS_MCACHE_RW_PERCENTAGE / 100;
- boundary = (u8 *)ALIGN_DOWN((uintptr_t)boundary,
- CBFS_MCACHE_ALIGNMENT);
+ boundary = (u8 *)ALIGN_DOWN((uintptr_t)boundary, CBFS_MCACHE_ALIGNMENT);
if (id == CBMEM_ID_CBFS_RO_MCACHE) {
cbd->mcache = _cbfs_mcache;
cbd->mcache_size = boundary - _cbfs_mcache;
@@ -434,20 +422,19 @@
}
cb_err_t cbfs_init_boot_device(const struct cbfs_boot_device *cbd,
- struct vb2_hash *metadata_hash)
+ struct vb2_hash *mdata_hash)
{
/* If we have an mcache, mcache_build() will also check mdata hash. */
if (!CONFIG(NO_CBFS_MCACHE) && !ENV_SMM && cbd->mcache_size > 0)
- return cbfs_mcache_build(&cbd->rdev, cbd->mcache,
- cbd->mcache_size, metadata_hash);
+ return cbfs_mcache_build(&cbd->rdev, cbd->mcache, cbd->mcache_size, mdata_hash);
/* No mcache and no verification means we have nothing special to do. */
- if (!CONFIG(CBFS_VERIFICATION) || !metadata_hash)
+ if (!CONFIG(CBFS_VERIFICATION) || !mdata_hash)
return CB_SUCCESS;
- /* Verification only: use cbfs_walk() without a walker() function to
- just run through the CBFS once, will return NOT_FOUND by default. */
- cb_err_t err = cbfs_walk(&cbd->rdev, NULL, NULL, metadata_hash, 0);
+ /* Verification only: use cbfs_walk() without a walker() function to just run through
+ the CBFS once, will return NOT_FOUND by default. */
+ cb_err_t err = cbfs_walk(&cbd->rdev, NULL, NULL, mdata_hash, 0);
if (err == CB_CBFS_NOT_FOUND)
err = CB_SUCCESS;
return err;
@@ -457,22 +444,22 @@
{
static struct cbfs_boot_device ro;
- /* Ensure we always init RO mcache, even if first file is from RW.
+ /* Ensure we always init RO mcache, even if the first file is from the RW CBFS.
Otherwise it may not be available when needed in later stages. */
if (ENV_INITIAL_STAGE && !force_ro && !region_device_sz(&ro.rdev))
cbfs_get_boot_device(true);
if (!force_ro) {
const struct cbfs_boot_device *rw = vboot_get_cbfs_boot_device();
- /* This will return NULL if vboot isn't enabled, didn't run yet
- or decided to boot into recovery mode. */
+ /* This will return NULL if vboot isn't enabled, didn't run yet or decided to
+ boot into recovery mode. */
if (rw)
return rw;
}
- /* In rare cases post-RAM stages may run this before cbmem_initialize(),
- so we can't lock in the result of find_mcache() on the first try and
- should keep trying every time until an mcache is found. */
+ /* In rare cases post-RAM stages may run this before cbmem_initialize(), so we can't
+ lock in the result of find_mcache() on the first try and should keep trying every
+ time until an mcache is found. */
cbfs_boot_device_find_mcache(&ro, CBMEM_ID_CBFS_RO_MCACHE);
if (region_device_sz(&ro.rdev))
--
To view, visit https://review.coreboot.org/c/coreboot/+/49330
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I4a701d24fc9ccd68dce8789aab15fd21964a55f9
Gerrit-Change-Number: 49330
Gerrit-PatchSet: 1
Gerrit-Owner: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Furquan Shaikh <furquan(a)google.com>
Gerrit-Attention: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-MessageType: newchange
Attention is currently required from: Nico Huber.
Hello Nico Huber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/49320
to review the following change.
Change subject: nb/intel/sandybridge: Clarify command timing calculation
......................................................................
nb/intel/sandybridge: Clarify command timing calculation
Command timing is the absolute value of the most negative `pi_coding`
value across all ranks, or zero if there are no negative values. Use the
MAX() macro to ease proving that `cmd_delay` can never be negative, and
then drop the always-false underflow check.
The variable type for `cmd_delay` still needs to be signed because of
the comparisons with `pi_coding`, which is a signed value. Using an
unsigned type would result in undefined and also undesired behavior.
Change-Id: I714d3cf57d0f62376a1107af63bcd761f952bc3a
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
---
M src/northbridge/intel/sandybridge/raminit_common.c
1 file changed, 3 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/20/49320/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c
index f766867..cef0131 100644
--- a/src/northbridge/intel/sandybridge/raminit_common.c
+++ b/src/northbridge/intel/sandybridge/raminit_common.c
@@ -938,16 +938,11 @@
u32 clk_logic_dly = 0;
/*
- * Apply command delay if desired setting is negative. Find the
- * most negative value: 'cmd_delay' will be the absolute value.
+ * Compute command timing as abs() of the most negative PI code
+ * across all ranks. Use zero if none of the values is negative.
*/
FOR_ALL_POPULATED_RANKS {
- if (cmd_delay < -ctrl->timings[channel][slotrank].pi_coding)
- cmd_delay = -ctrl->timings[channel][slotrank].pi_coding;
- }
- if (cmd_delay < 0) {
- printk(BIOS_ERR, "C%d command delay underflow: %d\n", channel, cmd_delay);
- cmd_delay = 0;
+ cmd_delay = MAX(cmd_delay, -ctrl->timings[channel][slotrank].pi_coding);
}
if (cmd_delay > CCC_MAX_PI) {
printk(BIOS_ERR, "C%d command delay overflow: %d\n", channel, cmd_delay);
--
To view, visit https://review.coreboot.org/c/coreboot/+/49320
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I714d3cf57d0f62376a1107af63bcd761f952bc3a
Gerrit-Change-Number: 49320
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: newchange
Attention is currently required from: Nico Huber.
Hello Nico Huber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/49319
to review the following change.
Change subject: nb/intel/sandybridge: Fix handling of clock timing
......................................................................
nb/intel/sandybridge: Fix handling of clock timing
Clock is a differential signal and propagates faster than command and
control, therefore its timing needs to be offset with `pi_code_offset`.
It is also a periodic signal, so it can safely wrap around.
To avoid potential undefined behavior, make `clk_delay` signed. It makes
no difference with valid values, because the initial value can be proven
to never be negative and `pi_code_offset` is always positive. With this
change, it is possible to add an underflow check, for additional sanity.
Change-Id: I375adf84142079f341b060fba5e79ce4dcb002be
Signed-off-by: Angel Pons <th3fanbus(a)gmail.com>
---
M src/northbridge/intel/sandybridge/raminit_common.c
1 file changed, 14 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/19/49319/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c
index 092e34d..f766867 100644
--- a/src/northbridge/intel/sandybridge/raminit_common.c
+++ b/src/northbridge/intel/sandybridge/raminit_common.c
@@ -977,14 +977,24 @@
}
}
FOR_ALL_POPULATED_RANKS {
- u32 clk_delay = ctrl->timings[channel][slotrank].pi_coding + cmd_delay;
+ int clk_delay = ctrl->timings[channel][slotrank].pi_coding + cmd_delay;
- if (clk_delay > CCC_MAX_PI) {
- printk(BIOS_ERR, "C%dR%d clock delay overflow: %d\n",
+ /*
+ * Clock is a differential signal, whereas command and control are not.
+ * This affects its timing, and it is also why it needs a magic offset.
+ */
+ clk_delay += ctrl->pi_code_offset;
+
+ /* Can never happen with valid values */
+ if (clk_delay < 0) {
+ printk(BIOS_ERR, "C%dR%d clock delay underflow: %d\n",
channel, slotrank, clk_delay);
- clk_delay = CCC_MAX_PI;
+ clk_delay = 0;
}
+ /* Clock can safely wrap around because it is a periodic signal */
+ clk_delay %= CCC_MAX_PI + 1;
+
clk_pi_coding |= (clk_delay % QCLK_PI) << (6 * slotrank);
clk_logic_dly |= (clk_delay / QCLK_PI) << slotrank;
}
--
To view, visit https://review.coreboot.org/c/coreboot/+/49319
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I375adf84142079f341b060fba5e79ce4dcb002be
Gerrit-Change-Number: 49319
Gerrit-PatchSet: 1
Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-MessageType: newchange
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49243 )
Change subject: vendorcode/intel/fsp: Update Tiger Lake v3444 FSP Headers
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://review.coreboot.org/c/coreboot/+/49243
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I68b32730293fc83b5088074f71fa215220574748
Gerrit-Change-Number: 49243
Gerrit-PatchSet: 4
Gerrit-Owner: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com>
Gerrit-Reviewer: Dossym Nurmukhanov <dossym(a)google.com>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: John Zhao <john.zhao(a)intel.com>
Gerrit-Reviewer: John Zhao <john.zhao(a)intel.corp-partner.google.com>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)chromium.org>
Gerrit-Reviewer: Nick Vaccaro <nvaccaro(a)google.com>
Gerrit-Reviewer: Patrick Georgi <pgeorgi(a)google.com>
Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org>
Gerrit-Reviewer: Raj Astekar <raj.astekar(a)intel.com>
Gerrit-Reviewer: Srinidhi N Kaushik <srinidhi.n.kaushik(a)intel.com>
Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Tue, 12 Jan 2021 00:53:38 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment