[coreboot-gerrit] [L] Change in coreboot[main]: soc/intel/common: Refactor BERT generation flow for crashlog

[Pratikkumar V Prajapati (Code Review)]

Attention is currently required from: Pratikkumar Prajapati.

Hello Pratikkumar Prajapati,

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

https://review.coreboot.org/c/coreboot/+/78257?usp=email

to review the following change.

Change subject: soc/intel/common: Refactor BERT generation flow for crashlog
......................................................................

soc/intel/common: Refactor BERT generation flow for crashlog

With earlier flow, a chunk of CBMEM region was allocated for
each SRAM e.g., PUNIT SRAM, SOC PMC SRAM and IOE PMC SRAM.
Then entire SRAM content was copied to dedicated CBMEM
region. Later in acpi_bert.c, the BERT table was getting
created for each chunk of CBMEM. This flow was not considering
creating separate entries for each region of crashlog records.
It resulted in only the first entry getting decoded from each SRAM.

New flow aims to fix this issue. With new flow, a simple singly
linked list is created to store each region of crashlog records
from all SRAMs. The crashlog data is not copied to CBMEM. The
nodes are allocated dynamically and then copied to ACPI BERT table
and then freed. This flow also makes the overall crashlog code
much simpler.

BUG=b:298234592
TEST= TBD.

Change-Id: I43bb61485b77d786647900ca284b7f492f412aee
Signed-off-by: Pratikkumar Prajapati 
---
M src/soc/intel/common/block/acpi/acpi_bert.c
M src/soc/intel/common/block/crashlog/crashlog.c
M src/soc/intel/common/block/include/intelblocks/crashlog.h
3 files changed, 148 insertions(+), 190 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/78257/1

diff --git a/src/soc/intel/common/block/acpi/acpi_bert.c b/src/soc/intel/common/block/acpi/acpi_bert.c
index 350d5d3..310d4ef 100644
--- a/src/soc/intel/common/block/acpi/acpi_bert.c
+++ b/src/soc/intel/common/block/acpi/acpi_bert.c
@@ -7,14 +7,14 @@
 #include <intelblocks/acpi.h>
 #include <intelblocks/crashlog.h>

-static bool boot_error_src_present(void)
+static bool boot_error_src_present(cl_node_t *head)
 {
    if (!discover_crashlog()) {
    	printk(BIOS_SPEW, "Crashlog discovery result: crashlog not found\n");
    	return false;
    }

-	collect_pmc_and_cpu_crashlog_from_srams();
+	collect_pmc_and_cpu_crashlog_from_srams(head);

/* Discovery tables sizes can be larger than the actual valid collected data */
    u32 crashlog_size = cl_get_total_data_size();
@@ -25,11 +25,11 @@
 static enum cb_err record_crashlog_into_bert(void **region, size_t *length)
 {
    acpi_generic_error_status_t *status = NULL;
-	size_t cpu_record_size, pmc_record_size;
    size_t gesb_header_size;
-	void *cl_data = NULL;
+	void *cl_acpi_data = NULL;
+	cl_node_t cl_list_head = {.size = 0, .data = NULL, .next = NULL};

-	if (!boot_error_src_present()) {
+	if (!boot_error_src_present(&cl_list_head)) {
    	return CB_ERR;
    }

@@ -47,67 +47,43 @@
    }

if (cl_get_total_data_size() > bert_storage_remaining()) {
-		printk(BIOS_ERR, "Crashlog entry would exceed "
-				"available region\n");
+		printk(BIOS_ERR, "Crashlog entry would exceed available region\n");
    	return CB_ERR;
    }

-	cpu_record_size = cl_get_cpu_record_size();
-	if (cpu_record_size) {
-		cl_data = new_cper_fw_error_crashlog(status, cpu_record_size);
-		if (!cl_data) {
-			printk(BIOS_ERR, "Crashlog CPU entry(size 0x%zx) "
-				"would exceed available region\n",
-				cpu_record_size);
-			return CB_ERR;
-		}
-		printk(BIOS_DEBUG, "cl_data %p, cpu_record_size 0x%zx\n",
-			cl_data, cpu_record_size);
-		cl_fill_cpu_records(cl_data);
-	}
+	bool multi_entry = false;
+	cl_node_t *cl_node = cl_list_head.next;
+	while (cl_node) {

-	pmc_record_size = cl_get_pmc_record_size();
-	if (pmc_record_size) {
-		/* Allocate new FW ERR structure in case PMC crashlog is present */
-		if (pmc_record_size && !bert_append_fw_err(status)) {
-			printk(BIOS_ERR, "Crashlog PMC entry would "
-				"exceed available region\n");
-			return CB_ERR;
+		if (cl_node->size <= 0) {
+			cl_node = cl_node->next;
+			continue;
    	}

-		cl_data = new_cper_fw_error_crashlog(status, pmc_record_size);
-		if (!cl_data) {
-			printk(BIOS_ERR, "Crashlog PMC entry(size 0x%zx) "
-				"would exceed available region\n",
-				pmc_record_size);
-			return CB_ERR;
+		if (!(cl_node->data)) {
+			cl_node = cl_node->next;
+                        continue;
    	}
-		printk(BIOS_DEBUG, "cl_data %p, pmc_record_size 0x%zx\n",
-			cl_data, pmc_record_size);
-		cl_fill_pmc_records(cl_data);
-	}

-	if (CONFIG(SOC_INTEL_IOE_DIE_SUPPORT)) {
-		size_t ioe_record_size = cl_get_ioe_record_size();
-		if (ioe_record_size) {
-			/*  Allocate new FW ERR structure in case IOE crashlog is present */
-			if (ioe_record_size && !bert_append_fw_err(status)) {
-				printk(BIOS_ERR, "Crashlog IOE entry would "
-						"exceed available region\n");
+		if (multi_entry) {
+			if (!bert_append_fw_err(status)) {
+				printk(BIOS_ERR, "Crashlog entry would exceed available region\n");
    			return CB_ERR;
    		}
-
-			cl_data = new_cper_fw_error_crashlog(status, ioe_record_size);
-			if (!cl_data) {
-				printk(BIOS_ERR, "Crashlog IOE entry(size 0x%zx) "
-						"would exceed available region\n",
-						ioe_record_size);
-				return CB_ERR;
-			}
-			printk(BIOS_DEBUG, "cl_data %p, ioe_record_size 0x%zx\n",
-					cl_data, ioe_record_size);
-			cl_fill_ioe_records(cl_data);
    	}
+
+		cl_acpi_data = new_cper_fw_error_crashlog(status, cl_node->size);
+		if (!cl_acpi_data) {
+			printk(BIOS_ERR, "Crashlog entry(size 0x%x) would exceed available region\n",
+					cl_node->size);
+			return CB_ERR;
+		}
+		memcpy(cl_acpi_data, (void *) cl_node->data, cl_node->size);
+
+		cl_node_t *temp = cl_node;
+		cl_node = cl_node->next;
+		free_cl_node(temp);
+		multi_entry = true;
    }

*length = status->data_length + gesb_header_size;
diff --git a/src/soc/intel/common/block/crashlog/crashlog.c b/src/soc/intel/common/block/crashlog/crashlog.c
index e3d85cd..6735d97 100644
--- a/src/soc/intel/common/block/crashlog/crashlog.c
+++ b/src/soc/intel/common/block/crashlog/crashlog.c
@@ -330,11 +330,36 @@
    return true;
 }

-void __weak cl_get_pmc_sram_data(void)
+cl_node_t *malloc_cl_node(size_t len)
 {
-	u32 *dest = NULL;
+	cl_node_t *node = malloc(sizeof(cl_node_t));
+	if (!node)
+		return NULL;
+
+	node->data = malloc(len * sizeof(u32));
+	if (!(node->data))
+		return NULL;
+
+	node->size = len * sizeof(u32);
+	node->next = NULL;
+
+	return node;
+}
+
+void free_cl_node(cl_node_t *node)
+{
+	if (!node)
+		return;
+	if (node->data)
+		free(node->data);
+	free(node);
+}
+
+void __weak cl_get_pmc_sram_data(cl_node_t *head)
+{
    u32 tmp_bar_addr = cl_get_cpu_tmp_bar();
    u32 pmc_crashLog_size = cl_get_pmc_record_size();
+	cl_node_t *cl_cur = head;

if (!cl_pmc_sram_has_mmio_access() || !tmp_bar_addr)
    	return;
@@ -353,7 +378,7 @@
    		goto pmc_send_re_arm_after_reset;
    	}
    	printk(BIOS_DEBUG, "PMC crashLog size in discovery mode : 0x%X\n",
-		       pmc_crashLog_size);
+				pmc_crashLog_size);
    } else {
    	if (discovery_buf.bits.dis) {
    		printk(BIOS_DEBUG, "PCH crashlog is disabled in legacy mode.\n");
@@ -362,51 +387,65 @@
    	pmc_crashLog_size = (discovery_buf.bits.size != 0) ?
    				discovery_buf.bits.size : 0xC00;
    	printk(BIOS_DEBUG, "PMC crashLog size in legacy mode : 0x%X\n",
-		       pmc_crashLog_size);
+				pmc_crashLog_size);
    }

-	/* allocate mem for the record to be copied */
-	unsigned long pmc_cl_cbmem_addr;
-
-	pmc_cl_cbmem_addr = (unsigned long)cbmem_add(CBMEM_ID_PMC_CRASHLOG,
-						pmc_crashLog_size);
-	if (!pmc_cl_cbmem_addr) {
-		printk(BIOS_ERR, "Unable to allocate CBMEM PMC crashLog entry.\n");
-		return;
-	}
-
-	memset((void *)pmc_cl_cbmem_addr, 0, pmc_crashLog_size);
-	dest = (u32 *)(uintptr_t)pmc_cl_cbmem_addr;
    bool pmc_sram = true;
-	pmc_crashlog_desc_table_t descriptor_table =  cl_get_pmc_descriptor_table();
+	pmc_crashlog_desc_table_t descriptor_table = cl_get_pmc_descriptor_table();
+
+	/* goto tail node */
+	while (cl_cur && cl_cur->next) {
+		cl_cur = cl_cur->next;
+	}
+
    if (discovery_buf.bits.discov_mechanism == 1) {
    	for (int i = 0; i < descriptor_table.numb_regions; i++) {
+
+			cl_node_t *cl_node = malloc_cl_node(descriptor_table.regions[i].bits.size);
+			if (!cl_node) {
+				printk(BIOS_DEBUG, "failed to allocate cl_node [region = %d]\n", i);
+				goto pmc_send_re_arm_after_reset;
+			}
+
    		if (cl_copy_data_from_sram(tmp_bar_addr,
-						  descriptor_table.regions[i].bits.offset,
-						  descriptor_table.regions[i].bits.size,
-						  dest,
-						  i,
-						  pmc_sram)) {
-				dest = (u32 *)((u32)dest +
-						(descriptor_table.regions[i].bits.size
-						* sizeof(u32)));
+						descriptor_table.regions[i].bits.offset,
+						descriptor_table.regions[i].bits.size,
+						cl_node->data,
+						i,
+						pmc_sram)) {
+				cl_cur->next = cl_node;
+				cl_cur = cl_cur->next;
    		} else {
    			pmc_crashLog_size -= descriptor_table.regions[i].bits.size *
    						sizeof(u32);
    			printk(BIOS_DEBUG, "discover mode PMC crashlog size adjusted"
-					" to: 0x%x\n", pmc_crashLog_size);
+						" to: 0x%x\n", pmc_crashLog_size);
+				/* free cl_node */
+				free_cl_node(cl_node);
    		}
    	}
    } else {
-		if (!cl_copy_data_from_sram(tmp_bar_addr,
-					   discovery_buf.bits.base_offset,
-					   discovery_buf.bits.size,
-					   dest,
-					   0,
-					   pmc_sram)) {
+
+		cl_node_t *cl_node = malloc_cl_node(discovery_buf.bits.size);
+		if (!cl_node) {
+			printk(BIOS_DEBUG, "failed to allocate cl_node\n");
+			goto pmc_send_re_arm_after_reset;
+		}
+
+		if (cl_copy_data_from_sram(tmp_bar_addr,
+					discovery_buf.bits.base_offset,
+					discovery_buf.bits.size,
+					cl_node->data,
+					0,
+					pmc_sram)) {
+			cl_cur->next = cl_node;
+			cl_cur = cl_cur->next;
+		} else {
    		pmc_crashLog_size -= discovery_buf.bits.size * sizeof(u32);
    		printk(BIOS_DEBUG, "legacy mode PMC crashlog size adjusted to: 0x%x\n",
-			       pmc_crashLog_size);
+					pmc_crashLog_size);
+			/* free cl_node */
+			free_cl_node(cl_node);
    	}
    }

@@ -422,10 +461,9 @@

}

-void cl_get_cpu_sram_data(void)
+void cl_get_cpu_sram_data(cl_node_t *head)
 {
-	u32 tmp_bar_addr = 0;
-	u32 *dest = NULL;
+	cl_node_t *cl_cur = head;
    u32 m_cpu_crashLog_size = cl_get_cpu_record_size();
    cpu_crashlog_discovery_table_t cpu_cl_disc_tab = cl_get_cpu_discovery_table();

@@ -435,35 +473,41 @@
    }

printk(BIOS_DEBUG, "CPU crash data size: 0x%X bytes in 0x%X region(s).\n",
-	       m_cpu_crashLog_size, cpu_cl_disc_tab.header.fields.count);
+			m_cpu_crashLog_size, cpu_cl_disc_tab.header.fields.count);

-	/* allocate memory buffers for CPU crashog data to be copied */
-	unsigned long cpu_crashlog_cbmem_addr;
-	cpu_crashlog_cbmem_addr = (unsigned long)cbmem_add(CBMEM_ID_CPU_CRASHLOG,
-								m_cpu_crashLog_size);
-	if (!cpu_crashlog_cbmem_addr) {
-		printk(BIOS_ERR, "Failed to add CPU main crashLog entries to CBMEM.\n");
-		return;
+	/* goto tail node */
+	while (cl_cur && cl_cur->next) {
+		cl_cur = cl_cur->next;
    }

-	memset((void *)cpu_crashlog_cbmem_addr, 0, m_cpu_crashLog_size);
-	tmp_bar_addr = cl_get_cpu_bar_addr();
-	dest = (u32 *)(uintptr_t)cpu_crashlog_cbmem_addr;
-	bool pmc_sram = false;
-
    for (int i = 0 ; i < cpu_cl_disc_tab.header.fields.count ; i++) {
-		if (cl_copy_data_from_sram(tmp_bar_addr,
-					   cpu_cl_disc_tab.buffers[i].fields.offset,
-					   cpu_cl_disc_tab.buffers[i].fields.size,
-					   dest,
-					   i,
-					   pmc_sram)) {
-			dest = (u32 *)((u32)dest +
-					(cpu_cl_disc_tab.buffers[i].fields.size * sizeof(u32)));
+
+		u32 cpu_bar_addr = cl_get_cpu_bar_addr();
+		bool pmc_sram = false;
+
+		if (!cpu_cl_disc_tab.buffers[i].fields.size) {
+			continue;
+		}
+
+		cl_node_t *cl_node = malloc_cl_node(cpu_cl_disc_tab.buffers[i].fields.size);
+		if (!cl_node) {
+			printk(BIOS_DEBUG, "failed to allocate cl_node [buffer = %d]\n", i);
+			return;
+		}
+
+		if (cl_copy_data_from_sram(cpu_bar_addr,
+					cpu_cl_disc_tab.buffers[i].fields.offset,
+					cpu_cl_disc_tab.buffers[i].fields.size,
+					cl_node->data,
+					i,
+					pmc_sram)) {
+			cl_cur->next = cl_node;
+			cl_cur = cl_cur->next;
+
    	} else {
    		m_cpu_crashLog_size -= cpu_cl_disc_tab.buffers[i].fields.size
-						* sizeof(u32);
-
+				* sizeof(u32);
+			free_cl_node(cl_node);
    		/* for CPU skip all buffers if the 1st one is not valid */
    		if (i == 0) {
    			m_cpu_crashLog_size = 0;
@@ -482,7 +526,7 @@
    cpu_cl_rearm();
 }

-void collect_pmc_and_cpu_crashlog_from_srams(void)
+void collect_pmc_and_cpu_crashlog_from_srams(cl_node_t *head)
 {
    if (pmc_crashlog_support() && cl_pmc_data_present()
    	&& (cl_get_pmc_record_size() > 0)) {
@@ -490,7 +534,7 @@
    		cl_pmc_en_gen_on_all_reboot();
    		printk(BIOS_DEBUG, "Crashlog collection enabled on every reboot.\n");
    	}
-		cl_get_pmc_sram_data();
+		cl_get_pmc_sram_data(head);
    } else {
    	printk(BIOS_DEBUG, "Skipping PMC crashLog collection. Data not present.\n");
    }
@@ -500,74 +544,8 @@
    if (cpu_crashlog_support() && cl_cpu_data_present()
    	&& (cl_get_cpu_record_size() > 0)) {
    	printk(BIOS_DEBUG, "CPU crashLog present.\n");
-		cl_get_cpu_sram_data();
+		cl_get_cpu_sram_data(head);
    } else {
    	printk(BIOS_DEBUG, "Skipping CPU crashLog collection. Data not present.\n");
    }
 }
-
-bool cl_fill_cpu_records(void *cl_record)
-{
-	void *cl_src_addr = NULL;
-
-	u32 m_cpu_crashLog_size = cl_get_cpu_record_size();
-
-	if (!cl_cpu_data_present() || m_cpu_crashLog_size == 0) {
-		printk(BIOS_DEBUG, "CPU crashLog not present, skipping.\n");
-		return false;
-	}
-
-	printk(BIOS_DEBUG, "CPU crash data collection.\n");
-	cl_src_addr = cbmem_find(CBMEM_ID_CPU_CRASHLOG);
-	if (!cl_src_addr) {
-		printk(BIOS_DEBUG, "CPU crash data, CBMEM not found\n");
-		return false;
-	}
-	memcpy(cl_record, cl_src_addr, m_cpu_crashLog_size);
-
-	return true;
-}
-
-bool cl_fill_pmc_records(void *cl_record)
-{
-	void *cl_src_addr = NULL;
-
-	u32 m_pmc_crashLog_size = cl_get_pmc_record_size();
-
-	if (!cl_pmc_data_present() || m_pmc_crashLog_size == 0) {
-		printk(BIOS_DEBUG, "PMC crashLog not present, skipping.\n");
-		return false;
-	}
-
-	printk(BIOS_DEBUG, "PMC crash data collection.\n");
-	cl_src_addr = cbmem_find(CBMEM_ID_PMC_CRASHLOG);
-	if (!cl_src_addr) {
-		printk(BIOS_DEBUG, "PMC crash data, CBMEM not found\n");
-		return false;
-	}
-	memcpy(cl_record, cl_src_addr, m_pmc_crashLog_size);
-
-	return true;
-}
-
-bool cl_fill_ioe_records(void *cl_record)
-{
-	void *cl_src_addr = NULL;
-
-	u32 m_ioe_crashLog_size = cl_get_ioe_record_size();
-
-	if (!cl_ioe_data_present() || m_ioe_crashLog_size == 0) {
-		printk(BIOS_DEBUG, "IOE crashLog not present, skipping.\n");
-		return false;
-	}
-
-	printk(BIOS_DEBUG, "IOE PMC crash data collection.\n");
-	cl_src_addr = cbmem_find(CBMEM_ID_IOE_CRASHLOG);
-	if (!cl_src_addr) {
-		printk(BIOS_DEBUG, "IOE crash data, CBMEM not found\n");
-		return false;
-	}
-	memcpy(cl_record, cl_src_addr, m_ioe_crashLog_size);
-
-	return true;
-}
diff --git a/src/soc/intel/common/block/include/intelblocks/crashlog.h b/src/soc/intel/common/block/include/intelblocks/crashlog.h
index 38d1a09..8948977 100644
--- a/src/soc/intel/common/block/include/intelblocks/crashlog.h
+++ b/src/soc/intel/common/block/include/intelblocks/crashlog.h
@@ -170,6 +170,14 @@
    cpu_crashlog_buffer_info_t buffers[256];
 } __packed cpu_crashlog_discovery_table_t;

+typedef struct cl_node_t {
+	u32 size;
+	void *data;
+	struct cl_node_t* next;
+} cl_node_t;
+
+cl_node_t *malloc_cl_node(size_t len);
+void free_cl_node(cl_node_t *node);
 int cl_get_cpu_record_size(void);
 int cl_get_pmc_record_size(void);
 int cl_get_ioe_record_size(void);
@@ -184,8 +192,8 @@
 bool cl_cpu_data_present(void);
 bool cl_pmc_data_present(void);
 bool cl_ioe_data_present(void);
-void cl_get_cpu_sram_data(void);
-void cl_get_pmc_sram_data(void);
+void cl_get_cpu_sram_data(cl_node_t *head);
+void cl_get_pmc_sram_data(cl_node_t *head);
 void reset_discovery_buffers(void);
 void update_new_pmc_crashlog_size(u32 *pmc_crash_size);
 void update_new_cpu_crashlog_size(u32 *cpu_crash_size);
@@ -213,11 +221,7 @@
    		u32 *dest_addr,
    		u32 buffer_index,
    		bool pmc_sram);
-void collect_pmc_and_cpu_crashlog_from_srams(void);
-bool cl_fill_cpu_records(void *cl_record);
-bool cl_fill_pmc_records(void *cl_record);
-bool cl_fill_ioe_records(void *cl_record);
-
+void collect_pmc_and_cpu_crashlog_from_srams(cl_node_t *head);
 static const EFI_GUID FW_ERR_SECTION_GUID = {
    0x81212a96, 0x09ed, 0x4996,
    { 0x94, 0x71, 0x8d, 0x72, 0x9c, 0x8e, 0x69, 0xed }