[Skiboot] [PATCH v7 12/22] hdata: Add result table property to "dump" node

[Michael Neuling]

On Sat, 2019-04-13 at 14:45 +0530, Vasant Hegde wrote:
> During MPIPL hostboot updates MDRT table with details of memory movement
> (source, destination address, size, etc). Convert these details to fadump
> structure format and pass it to kernel via 'result-table' property.
> 
> Device tree property:
>   /ibm,opal/dump/result-table - follows fadump structure format
> 
> Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
> ---
>  hdata/spira.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hdata/spira.h |  4 +++
>  2 files changed, 97 insertions(+)
> 
> diff --git a/hdata/spira.c b/hdata/spira.c
> index 34e3034ce..fa6149322 100644
> --- a/hdata/spira.c
> +++ b/hdata/spira.c
> @@ -26,6 +26,8 @@
>  #include <fsp-attn.h>
>  #include <fsp-leds.h>
>  #include <skiboot.h>
> +#include <mem_region-malloc.h>
> +#include <opal-dump.h>
>  
>  #include "hdata.h"
>  #include "hostservices.h"
> @@ -1055,6 +1057,94 @@ static void dt_init_secureboot_node(const struct iplparams_sysparams *sysparams)
>  	dt_add_property_cells(node, "hw-key-hash-size", hw_key_hash_size);
>  }
>  
> +static void opal_dump_add_result_table(const struct iplparams_iplparams *p)
> +{
> +	int i, j = 0;
> +	u32 mdrt_cnt = spira.ntuples.mdump_res.act_cnt;
> +	u32 mdrt_max_cnt = MDRT_TABLE_SIZE / sizeof(struct mdrt_table);
> +	size_t prop_size;
> +	struct mdrt_table *mdrt = (void *)(MDRT_TABLE_BASE);
> +	struct fadump_section *fadump_section;
> +	struct fadump *result_table;
> +	struct dt_node *dump_node;
> +
> +	dump_node = dt_find_by_path(opal_node, "dump");
> +	if (!dump_node)
> +		return;
> +
> +	/* Check boot params to detect MPIPL boot or not */
> +	if (p->cec_ipl_maj_type != IPLPARAMS_MAJ_TYPE_REIPL)
> +		return;
> +	/*
> +	 * On FSP system we get minor type as post dump IPL and on BMC system
> +	 * we get platform reboot. Hence lets check for both values
> +	 */
> +	if (p->cec_ipl_min_type != IPLPARAMS_MIN_TYPE_POST_DUMP &&
> +	    p->cec_ipl_min_type != IPLPARAMS_MIN_TYPE_PLAT_REBOOT) {
> +		prlog(PR_DEBUG, "DUMP: Non MPIPL reboot "
> +		      "[minor type = 0x%x]\n", p->cec_ipl_min_type);
> +		return;
> +	}
> +	if (p->cec_ipl_attrib != IPLPARAMS_ATTRIB_MEM_PRESERVE) {
> +		prlog(PR_DEBUG, "DUMP: Memory not preserved\n");
> +		return;
> +	}
> +
> +	if (mdrt_cnt == 0 || mdrt_cnt >= mdrt_max_cnt) {
> +		prlog(PR_DEBUG, "DUMP: Invalid MDRT count : %x\n", mdrt_cnt);
> +		return;
> +	}
> +
> +	prlog(PR_NOTICE, "DUMP: Dump found, MDRT count = 0x%x\n", mdrt_cnt);
> +
> +	/* Calculcate property size */
> +	prop_size = sizeof(struct fadump) +
> +		(mdrt_cnt * sizeof(struct fadump_section));
> +	result_table = zalloc(prop_size);
> +	if (!result_table) {
> +		prlog(PR_ERR, "DUMP: Failed to allocate memory\n");
> +		return;
> +	}
> +
> +	/* Copy MDRT entries to result-table */
> +	result_table->fadump_section_size = sizeof(struct fadump_section);
> +	for (i = 0; i < mdrt_cnt; i++) {
> +		/* Skip garbage entry */
> +		if ((mdrt->dest_addr == 0) || (mdrt->size == 0)) {
> +			mdrt++;
> +			continue;
> +		}
> +
> +		if (mdrt->dump_type != DUMP_TYPE_FADUMP) {
> +			mdrt++;
> +			continue;
> +		}
> +
> +		fadump_section = &(result_table->section[j++]);
> +		fadump_section->source_type = mdrt->data_region;
> +		/* Clear top bit */
> +		fadump_section->source_addr =
> +			cpu_to_be64(mdrt->src_addr & ~(HRMOR_BIT));
> +		fadump_section->dest_addr =
> +			cpu_to_be64(mdrt->dest_addr & ~(HRMOR_BIT));
> +		fadump_section->source_size = cpu_to_be64(mdrt->size);
> +		fadump_section->dest_size = cpu_to_be64(mdrt->size);
> +		mdrt++;
> +	}
> +
> +	if (j == 0) {
> +		prlog(PR_DEBUG, "DUMP: MDRT table is empty\n");
> +		free(result_table);
> +		return;
> +	}
> +
> +	result_table->section_count = cpu_to_be16(j);
> +	/* Actual property size */
> +	prop_size = sizeof(struct fadump) + (j * sizeof(struct fadump_section));
> +	dt_add_property(dump_node, "result-table", result_table, prop_size);

This concerns me, especially since it's an API.

How is this structure communicated to the kernel? This seems to be a large flat
structure passed from opal to the kernel here. 

What happens if we need to make a change to this format in future?  How are we
going to communicate that to the kernel?  Do we need a compat or version
property here?

Why don't we represent the structure more in the device tree, rather than
flattening it here?

Mikey

> +	free(result_table);
> +}
> +
>  static void add_opal_dump_node(void)
>  {
>  	u64 fw_load_area[4];
> @@ -1218,6 +1308,9 @@ static void add_iplparams_ipl_params(const void *iplp, struct dt_node *node)
>  	else
>  		dt_add_property_strings(led_node, DT_PROPERTY_LED_MODE,
>  					LED_MODE_GUIDING_LIGHT);
> +
> +	/* Populate opal dump result table */
> +	opal_dump_add_result_table(p);
>  }
>  
>  static void add_iplparams_serials(const void *iplp, struct dt_node *node)
> diff --git a/hdata/spira.h b/hdata/spira.h
> index 80e75d220..a8b68327d 100644
> --- a/hdata/spira.h
> +++ b/hdata/spira.h
> @@ -401,8 +401,12 @@ struct iplparams_iplparams {
>  #define IPLPARAMS_FSP_FW_IPL_SIDE_TEMP	0x01
>  	uint8_t		ipl_speed;
>  	__be16		cec_ipl_attrib;
> +#define IPLPARAMS_ATTRIB_MEM_PRESERVE	PPC_BIT16(2)
>  	uint8_t		cec_ipl_maj_type;
> +#define IPLPARAMS_MAJ_TYPE_REIPL	0x1
>  	uint8_t		cec_ipl_min_type;
> +#define IPLPARAMS_MIN_TYPE_POST_DUMP	0xc
> +#define IPLPARAMS_MIN_TYPE_PLAT_REBOOT	0xd
>  	uint8_t		os_ipl_mode;
>  	uint8_t		keylock_pos;
>  	uint8_t		lmb_size;