Re: [REBASE PATCH v5 11/17] qcom_minidump: Register ramoops region with minidump

[Kees Cook <keescook@chromium.org>]

On Wed, Sep 13, 2023 at 04:30:04PM -0700, Kees Cook wrote:
> On Mon, Sep 11, 2023 at 04:23:53PM +0530, Mukesh Ojha wrote:
> > Register all the pstore frontend with minidump, so that they can
> > be dumped as default Linux minidump region to be collected on
> > SoC where minidump is enabled.
> > 
> > Helper functions is written in separate file and built along with
> > the minidump driver, since it is client of minidump and also call
> > it at appropriate place from minidump probe so that they always
> > get registered.
> > 
> > While at it also rename the out minidump module object name during
> > build as qcom_apss_minidump which basically depicts as Qualcomm
> > Application processor subsystem minidump.
> > 
> > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> > ---
> >  drivers/soc/qcom/Kconfig                 |  1 +
> >  drivers/soc/qcom/Makefile                |  3 +-
> >  drivers/soc/qcom/qcom_minidump.c         |  4 ++
> >  drivers/soc/qcom/qcom_ramoops_minidump.c | 88 ++++++++++++++++++++++++++++++++
> >  drivers/soc/qcom/qcom_ramoops_minidump.h | 10 ++++
> >  5 files changed, 105 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
> >  create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h
> > 
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index 0ac7afc2c67d..9f1a1e128fef 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -306,6 +306,7 @@ config QCOM_MINIDUMP
> >  	tristate "QCOM APSS Minidump driver"
> >  	depends on ARCH_QCOM || COMPILE_TEST
> >  	depends on QCOM_SMEM
> > +	depends on PSTORE
> >  	help
> >  	  This config enables linux core infrastructure for Application
> >  	  processor subsystem (APSS) minidump collection i.e, it enables
> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> > index 4b5f72f78d3c..69df41aba7a9 100644
> > --- a/drivers/soc/qcom/Makefile
> > +++ b/drivers/soc/qcom/Makefile
> > @@ -33,4 +33,5 @@ obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
> >  qcom_ice-objs			+= ice.o
> >  obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= qcom_ice.o
> >  obj-$(CONFIG_QCOM_RPROC_MINIDUMP)	+= qcom_rproc_minidump.o
> > -obj-$(CONFIG_QCOM_MINIDUMP)		+= qcom_minidump.o
> > +obj-$(CONFIG_QCOM_MINIDUMP)		+= qcom_apss_minidump.o
> > +qcom_apss_minidump-objs			+= qcom_minidump.o qcom_ramoops_minidump.o
> > diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
> > index 4ce36f154e89..7930a80b9100 100644
> > --- a/drivers/soc/qcom/qcom_minidump.c
> > +++ b/drivers/soc/qcom/qcom_minidump.c
> > @@ -23,6 +23,7 @@
> >  #include <soc/qcom/qcom_minidump.h>
> >  
> >  #include "qcom_minidump_internal.h"
> > +#include "qcom_ramoops_minidump.h"
> >  
> >  /**
> >   * struct minidump_ss_data - Minidump subsystem private data
> > @@ -688,6 +689,8 @@ static int qcom_apss_minidump_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > +	qcom_ramoops_minidump_register(md->dev);
> > +
> >  	mutex_lock(&md_plist.plock);
> >  	platform_set_drvdata(pdev, md);
> >  	qcom_apss_register_pending_regions(md);
> > @@ -701,6 +704,7 @@ static int qcom_apss_minidump_remove(struct platform_device *pdev)
> >  	struct minidump *md = platform_get_drvdata(pdev);
> >  	struct minidump_ss_data *mdss_data;
> >  
> > +	qcom_ramoops_minidump_unregister();
> >  	mdss_data = md->apss_data;
> >  	memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct minidump_subsystem));
> >  	md = NULL;
> > diff --git a/drivers/soc/qcom/qcom_ramoops_minidump.c b/drivers/soc/qcom/qcom_ramoops_minidump.c
> > new file mode 100644
> > index 000000000000..eb97310e3858
> > --- /dev/null
> > +++ b/drivers/soc/qcom/qcom_ramoops_minidump.c
> > @@ -0,0 +1,88 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pstore.h>
> > +#include <linux/slab.h>
> > +#include <soc/qcom/qcom_minidump.h>
> > +
> > +#include "qcom_ramoops_minidump.h"
> > +
> > +static LIST_HEAD(ramoops_region_list);
> > +
> > +struct md_region_list {
> > +	struct qcom_minidump_region md_region;
> > +	struct list_head list;
> > +};
> > +
> > +static int qcom_ramoops_region_register(struct device *dev, int type)
> > +{
> > +	struct qcom_minidump_region *md_region;
> > +	struct md_region_list *mdr_list;
> > +	struct pstore_record record;
> > +	unsigned int max_dump_cnt;
> > +	phys_addr_t phys;
> > +	const char *name;
> > +	void *virt;
> > +	size_t size;
> > +	int ret;
> > +
> > +	record.type = type;
> > +	record.id = 0;
> > +	max_dump_cnt = 0;
> > +	name = pstore_type_to_name(record.type);
> > +	do {
> > +		ret = pstore_region_defined(&record, &virt, &phys, &size, &max_dump_cnt);
> 
> I really don't want this happening: you're building your own pstore_record
> (which has a common initializer that isn't used here) and manually
> scraping the ramoops regions.
> 
> It looks to me like you just want a way to talk all the records in
> pstore and then export their location to minidump. The record walker
> needs to be in the pstore core, and likely should be shared with
> fs/pstore/inode.c which does the same thing.
> 
> Then, in this code, you can just do something like:
> 
> 	for (record = pstore_get_record(NULL); record; record = pstore_get_record(record)) {

I just took another look at how records are stored, and I think the best
API here is going to be something like registering a callback so that
pstore can call into minidump for each record. (i.e. it can do this when
reading the records into the pstore filesystem);

	pstore_register_record_watcher(minidump_pstore_record)

Then pstore can make the calls if one is registered, in the middle of
pstore_mkfile(), with all the correct locks held, etc:

	if (psi->record_watcher)
		psi->record_watcher(record);

And then minidump_pstore_record() can do this part:


> 		if (ramoops_get_record_details(record, &virt, &phys) < 0)
> 			continue
> 		...
> 		md_region->virt_addr = virt;
> 		md_region->phys_addr = phys;
> 		md_region->size = record->size;
> 
> 		ret = qcom_minidump_region_register(md_region);

-Kees

-- 
Kees Cook