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@xxxxxxxxxxx>
> > ---
> > 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
