mbox series

[v8,0/4] Introduce mini-dump support for remoteproc

Message ID 1605819935-10726-1-git-send-email-sidgup@codeaurora.org
Headers show
Series Introduce mini-dump support for remoteproc | expand

Message

Siddharth Gupta Nov. 19, 2020, 9:05 p.m. UTC
Sometimes firmware sizes can be in tens of MB's and reading all the memory
during coredump can consume lot of time and memory.

Introducing support for mini-dumps. Mini-dump contains smallest amount of
useful information, that could help to debug subsystem crashes.

During bootup memory is allocated in SMEM (Shared memory) in the form of a
table that contains the physical addresses and sizes of the regions that
are supposed to be collected during coredump. This memory is shared amongst
all processors in a Qualcomm platform, so all remoteprocs fill in their
entry in the global table once they are out of reset.

This patch series adds support for parsing the global minidump table and
uses the current coredump frameork to expose this memory to userspace
during remoteproc's recovery.

This patch series also integrates the patch:
https://patchwork.kernel.org/patch/11695541/ sent by Siddharth.

Changelog:
v7 -> v8:
- Addressed all comments from Bjorn:
    * Renamed set_section_name to elf_strtbl_add.
    * Renamed rproc_minidump to rproc_coredump_using_sections.
    * Removed qcom_minidump header and moved structures to qcom_common source files.
    * Moved minidump specific functions to qcom_common source files.
    * Other minor fixes.

v6 -> v7:
- The STR_TAB size is calculated dynamically now instead of a predefined size.
- Added comments to indicate details about the reserved null section header. More
  details can be found at https://refspecs.linuxfoundation.org/elf/elf.pdf.

v5 -> v6:
- Removed priv_cleanup operation from rproc_ops. The dump_segments list is
  updated and cleaned up each time minidump is invoked.
- Split patch #2 into 2 parts - one that adds the rproc_minidump function, and
  the other that uses the new function in the qcom_q6v5_pas driver.
- Updated structs in qcom_minidump to explicitly indicate the endianness of the
  data stored in SMEM, also updated member names.
- Read the global table of contents in SMEM each time adsp_minidump is invoked.

v4 -> v5:
- Fixed adsp_add_minidump_segments to read IO memory using appropriate functions.

v3 -> v4:
- Made adsp_priv_cleanup a static function.

v2 -> v3:
- Refactored code to remove dependency on Qualcomm configs.
- Renamed do_rproc_minidump to rproc_minidump and marked as exported
  symbol.

v1 -> v2:
- 3 kernel test robot warnings have been resolved.
- Introduced priv_cleanup op in order to making the cleaning of
  private elements used by the remoteproc more readable.
- Removed rproc_cleanup_priv as it is no longer needed.
- Switched to if/else format for rproc_alloc in order to keep 
  the static const decalaration of adsp_minidump_ops.

Siddharth Gupta (4):
  remoteproc: core: Add ops to enable custom coredump functionality
  remoteproc: coredump: Add minidump functionality
  remoteproc: qcom: Add capability to collect minidumps
  remoteproc: qcom: Add minidump id for sm8150 modem

 drivers/remoteproc/qcom_common.c            | 147 ++++++++++++++++++++++++++++
 drivers/remoteproc/qcom_common.h            |   2 +
 drivers/remoteproc/qcom_q6v5_pas.c          |  28 +++++-
 drivers/remoteproc/remoteproc_core.c        |   6 +-
 drivers/remoteproc/remoteproc_coredump.c    | 140 ++++++++++++++++++++++++++
 drivers/remoteproc/remoteproc_elf_helpers.h |  26 +++++
 include/linux/remoteproc.h                  |   3 +
 7 files changed, 349 insertions(+), 3 deletions(-)

Comments

Bjorn Andersson Nov. 21, 2020, 4:03 a.m. UTC | #1
On Thu 19 Nov 15:05 CST 2020, Siddharth Gupta wrote:

> This change adds a new kind of core dump mechanism which instead of dumping

> entire program segments of the firmware, dumps sections of the remoteproc

> memory which are sufficient to allow debugging the firmware. This function

> thus uses section headers instead of program headers during creation of the

> core dump elf.

> 

> Co-developed-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>

> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>

> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


Regards,
Bjorn

> ---

>  drivers/remoteproc/remoteproc_coredump.c    | 140 ++++++++++++++++++++++++++++

>  drivers/remoteproc/remoteproc_elf_helpers.h |  26 ++++++

>  include/linux/remoteproc.h                  |   1 +

>  3 files changed, 167 insertions(+)

> 

> diff --git a/drivers/remoteproc/remoteproc_coredump.c b/drivers/remoteproc/remoteproc_coredump.c

> index 34530dc..81ec154 100644

> --- a/drivers/remoteproc/remoteproc_coredump.c

> +++ b/drivers/remoteproc/remoteproc_coredump.c

> @@ -323,3 +323,143 @@ void rproc_coredump(struct rproc *rproc)

>  	 */

>  	wait_for_completion(&dump_state.dump_done);

>  }

> +

> +/**

> + * rproc_coredump_using_sections() - perform coredump using section headers

> + * @rproc:	rproc handle

> + *

> + * This function will generate an ELF header for the registered sections of

> + * segments and create a devcoredump device associated with rproc. Based on

> + * the coredump configuration this function will directly copy the segments

> + * from device memory to userspace or copy segments from device memory to

> + * a separate buffer, which can then be read by userspace.

> + * The first approach avoids using extra vmalloc memory. But it will stall

> + * recovery flow until dump is read by userspace.

> + */

> +void rproc_coredump_using_sections(struct rproc *rproc)

> +{

> +	struct rproc_dump_segment *segment;

> +	void *shdr;

> +	void *ehdr;

> +	size_t data_size;

> +	size_t strtbl_size = 0;

> +	size_t strtbl_index = 1;

> +	size_t offset;

> +	void *data;

> +	u8 class = rproc->elf_class;

> +	int shnum;

> +	struct rproc_coredump_state dump_state;

> +	unsigned int dump_conf = rproc->dump_conf;

> +	char *str_tbl = "STR_TBL";

> +

> +	if (list_empty(&rproc->dump_segments) ||

> +	    dump_conf == RPROC_COREDUMP_DISABLED)

> +		return;

> +

> +	if (class == ELFCLASSNONE) {

> +		dev_err(&rproc->dev, "Elf class is not set\n");

> +		return;

> +	}

> +

> +	/*

> +	 * We allocate two extra section headers. The first one is null.

> +	 * Second section header is for the string table. Also space is

> +	 * allocated for string table.

> +	 */

> +	data_size = elf_size_of_hdr(class) + 2 * elf_size_of_shdr(class);

> +	shnum = 2;

> +

> +	/* the extra byte is for the null character at index 0 */

> +	strtbl_size += strlen(str_tbl) + 2;

> +

> +	list_for_each_entry(segment, &rproc->dump_segments, node) {

> +		data_size += elf_size_of_shdr(class);

> +		strtbl_size += strlen(segment->priv) + 1;

> +		if (dump_conf == RPROC_COREDUMP_ENABLED)

> +			data_size += segment->size;

> +		shnum++;

> +	}

> +

> +	data_size += strtbl_size;

> +

> +	data = vmalloc(data_size);

> +	if (!data)

> +		return;

> +

> +	ehdr = data;

> +	memset(ehdr, 0, elf_size_of_hdr(class));

> +	/* e_ident field is common for both elf32 and elf64 */

> +	elf_hdr_init_ident(ehdr, class);

> +

> +	elf_hdr_set_e_type(class, ehdr, ET_CORE);

> +	elf_hdr_set_e_machine(class, ehdr, rproc->elf_machine);

> +	elf_hdr_set_e_version(class, ehdr, EV_CURRENT);

> +	elf_hdr_set_e_entry(class, ehdr, rproc->bootaddr);

> +	elf_hdr_set_e_shoff(class, ehdr, elf_size_of_hdr(class));

> +	elf_hdr_set_e_ehsize(class, ehdr, elf_size_of_hdr(class));

> +	elf_hdr_set_e_shentsize(class, ehdr, elf_size_of_shdr(class));

> +	elf_hdr_set_e_shnum(class, ehdr, shnum);

> +	elf_hdr_set_e_shstrndx(class, ehdr, 1);

> +

> +	/*

> +	 * The zeroth index of the section header is reserved and is rarely used.

> +	 * Set the section header as null (SHN_UNDEF) and move to the next one.

> +	 */

> +	shdr = data + elf_hdr_get_e_shoff(class, ehdr);

> +	memset(shdr, 0, elf_size_of_shdr(class));

> +	shdr += elf_size_of_shdr(class);

> +

> +	/* Initialize the string table. */

> +	offset = elf_hdr_get_e_shoff(class, ehdr) +

> +		 elf_size_of_shdr(class) * elf_hdr_get_e_shnum(class, ehdr);

> +	memset(data + offset, 0, strtbl_size);

> +

> +	/* Fill in the string table section header. */

> +	memset(shdr, 0, elf_size_of_shdr(class));

> +	elf_shdr_set_sh_type(class, shdr, SHT_STRTAB);

> +	elf_shdr_set_sh_offset(class, shdr, offset);

> +	elf_shdr_set_sh_size(class, shdr, strtbl_size);

> +	elf_shdr_set_sh_entsize(class, shdr, 0);

> +	elf_shdr_set_sh_flags(class, shdr, 0);

> +	elf_shdr_set_sh_name(class, shdr, elf_strtbl_add(str_tbl, ehdr, class, &strtbl_index));

> +	offset += elf_shdr_get_sh_size(class, shdr);

> +	shdr += elf_size_of_shdr(class);

> +

> +	list_for_each_entry(segment, &rproc->dump_segments, node) {

> +		memset(shdr, 0, elf_size_of_shdr(class));

> +		elf_shdr_set_sh_type(class, shdr, SHT_PROGBITS);

> +		elf_shdr_set_sh_offset(class, shdr, offset);

> +		elf_shdr_set_sh_addr(class, shdr, segment->da);

> +		elf_shdr_set_sh_size(class, shdr, segment->size);

> +		elf_shdr_set_sh_entsize(class, shdr, 0);

> +		elf_shdr_set_sh_flags(class, shdr, SHF_WRITE);

> +		elf_shdr_set_sh_name(class, shdr,

> +				     elf_strtbl_add(segment->priv, ehdr, class, &strtbl_index));

> +

> +		/* No need to copy segments for inline dumps */

> +		if (dump_conf == RPROC_COREDUMP_ENABLED)

> +			rproc_copy_segment(rproc, data + offset, segment, 0,

> +					   segment->size);

> +		offset += elf_shdr_get_sh_size(class, shdr);

> +		shdr += elf_size_of_shdr(class);

> +	}

> +

> +	if (dump_conf == RPROC_COREDUMP_ENABLED) {

> +		dev_coredumpv(&rproc->dev, data, data_size, GFP_KERNEL);

> +		return;

> +	}

> +

> +	/* Initialize the dump state struct to be used by rproc_coredump_read */

> +	dump_state.rproc = rproc;

> +	dump_state.header = data;

> +	init_completion(&dump_state.dump_done);

> +

> +	dev_coredumpm(&rproc->dev, NULL, &dump_state, data_size, GFP_KERNEL,

> +		      rproc_coredump_read, rproc_coredump_free);

> +

> +	/* Wait until the dump is read and free is called. Data is freed

> +	 * by devcoredump framework automatically after 5 minutes.

> +	 */

> +	wait_for_completion(&dump_state.dump_done);

> +}

> +EXPORT_SYMBOL(rproc_coredump_using_sections);

> diff --git a/drivers/remoteproc/remoteproc_elf_helpers.h b/drivers/remoteproc/remoteproc_elf_helpers.h

> index 4b6be7b..26404e6 100644

> --- a/drivers/remoteproc/remoteproc_elf_helpers.h

> +++ b/drivers/remoteproc/remoteproc_elf_helpers.h

> @@ -65,6 +65,7 @@ ELF_GEN_FIELD_GET_SET(hdr, e_type, u16)

>  ELF_GEN_FIELD_GET_SET(hdr, e_version, u32)

>  ELF_GEN_FIELD_GET_SET(hdr, e_ehsize, u32)

>  ELF_GEN_FIELD_GET_SET(hdr, e_phentsize, u16)

> +ELF_GEN_FIELD_GET_SET(hdr, e_shentsize, u16)

>  

>  ELF_GEN_FIELD_GET_SET(phdr, p_paddr, u64)

>  ELF_GEN_FIELD_GET_SET(phdr, p_vaddr, u64)

> @@ -75,6 +76,9 @@ ELF_GEN_FIELD_GET_SET(phdr, p_offset, u64)

>  ELF_GEN_FIELD_GET_SET(phdr, p_flags, u32)

>  ELF_GEN_FIELD_GET_SET(phdr, p_align, u64)

>  

> +ELF_GEN_FIELD_GET_SET(shdr, sh_type, u32)

> +ELF_GEN_FIELD_GET_SET(shdr, sh_flags, u32)

> +ELF_GEN_FIELD_GET_SET(shdr, sh_entsize, u16)

>  ELF_GEN_FIELD_GET_SET(shdr, sh_size, u64)

>  ELF_GEN_FIELD_GET_SET(shdr, sh_offset, u64)

>  ELF_GEN_FIELD_GET_SET(shdr, sh_name, u32)

> @@ -93,4 +97,26 @@ ELF_STRUCT_SIZE(shdr)

>  ELF_STRUCT_SIZE(phdr)

>  ELF_STRUCT_SIZE(hdr)

>  

> +static inline unsigned int elf_strtbl_add(const char *name, void *ehdr, u8 class, size_t *index)

> +{

> +	u16 shstrndx = elf_hdr_get_e_shstrndx(class, ehdr);

> +	void *shdr;

> +	char *strtab;

> +	size_t idx, ret;

> +

> +	shdr = ehdr + elf_size_of_hdr(class) + shstrndx * elf_size_of_shdr(class);

> +	strtab = ehdr + elf_shdr_get_sh_offset(class, shdr);

> +	idx = index ? *index : 0;

> +	if (!strtab || !name)

> +		return 0;

> +

> +	ret = idx;

> +	strcpy((strtab + idx), name);

> +	idx += strlen(name) + 1;

> +	if (index)

> +		*index = idx;

> +

> +	return ret;

> +}

> +

>  #endif /* REMOTEPROC_ELF_LOADER_H */

> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h

> index a419878..cdf2722 100644

> --- a/include/linux/remoteproc.h

> +++ b/include/linux/remoteproc.h

> @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,

>  int rproc_boot(struct rproc *rproc);

>  void rproc_shutdown(struct rproc *rproc);

>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);

> +void rproc_coredump_using_sections(struct rproc *rproc);

>  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);

>  int rproc_coredump_add_custom_segment(struct rproc *rproc,

>  				      dma_addr_t da, size_t size,

> -- 

> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,

> a Linux Foundation Collaborative Project

>
Bjorn Andersson Nov. 21, 2020, 4:05 a.m. UTC | #2
On Thu 19 Nov 15:05 CST 2020, Siddharth Gupta wrote:

> Add minidump id for modem in sm8150 chipset so that the regions to be

> included in the coredump generated upon a crash is based on the minidump

> tables in SMEM instead of those in the ELF header.

> 

> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>


When reposting patches without modifications, please add any
Acked-by, Reviewed-by or Tested-by that you received previously.


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>


Regards,
Bjorn

> ---

>  drivers/remoteproc/qcom_q6v5_pas.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c

> index ca05c2ef..e61ef88 100644

> --- a/drivers/remoteproc/qcom_q6v5_pas.c

> +++ b/drivers/remoteproc/qcom_q6v5_pas.c

> @@ -630,6 +630,7 @@ static const struct adsp_data mpss_resource_init = {

>  	.crash_reason_smem = 421,

>  	.firmware_name = "modem.mdt",

>  	.pas_id = 4,

> +	.minidump_id = 3,

>  	.has_aggre2_clk = false,

>  	.auto_boot = false,

>  	.active_pd_names = (char*[]){

> -- 

> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,

> a Linux Foundation Collaborative Project

>