Re: [PATCH v2] pstore: Add boot loader log messages support

From: Yue Hu
Date: Mon May 20 2019 - 00:09:24 EST


On Wed, 15 May 2019 15:04:09 -0700
Kees Cook <keescook@xxxxxxxxxxxx> wrote:

> Hi!
>
> Thanks for the reminder to review this code. :) Sorry for the delay!
>
> On Thu, Feb 14, 2019 at 11:49 PM Yue Hu <zbestahu@xxxxxxxxx> wrote:
> >
> > From: Yue Hu <huyue2@xxxxxxxxxx>
> >
> > Sometimes we hope to check boot loader log messages (e.g. Android
> > Verified Boot status) when kernel is coming up. Generally it does
> > depend on serial device, but it will be removed for the hardware
> > shipping to market by most of manufacturers. In that case better
> > solder and proper serial cable for different interface (e.g. Type-C
> > or microUSB) are needed. That is inconvenient and even wasting much
> > time on it.
>
> Can you give some examples of how this would be used on a real device?
> More notes below...
>
> >
> > Therefore, let's add a logging support: PSTORE_TYPE_XBL.
> >
> > Signed-off-by: Yue Hu <huyue2@xxxxxxxxxx>
> > ---
> > v2: mention info of interacting with boot loader
> >
> > fs/pstore/Kconfig | 10 +++++++
> > fs/pstore/platform.c | 16 ++++++++++
> > fs/pstore/ram.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++--
> > include/linux/pstore.h | 21 +++++++++----
> > 4 files changed, 121 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
> > index 0d19d19..ef4a2dc 100644
> > --- a/fs/pstore/Kconfig
> > +++ b/fs/pstore/Kconfig
> > @@ -137,6 +137,16 @@ config PSTORE_FTRACE
> >
> > If unsure, say N.
> >
> > +config PSTORE_XBL
> > + bool "Log bootloader messages"
> > + depends on PSTORE
> > + help
> > + When the option is enabled, pstore will log boot loader
> > + messages to /sys/fs/pstore/xbl-ramoops-[ID] after reboot.
> > + Boot loader needs to support log buffer reserved.
> > +
> > + If unsure, say N.
> > +
> > config PSTORE_RAM
> > tristate "Log panic/oops to a RAM buffer"
> > depends on PSTORE
> > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> > index 2d1066e..2e6c3f8f 100644
> > --- a/fs/pstore/platform.c
> > +++ b/fs/pstore/platform.c
> > @@ -65,6 +65,7 @@
> > "mce",
> > "console",
> > "ftrace",
> > + "xbl",
> > "rtas",
> > "powerpc-ofw",
> > "powerpc-common",
> > @@ -530,6 +531,19 @@ static void pstore_register_console(void) {}
> > static void pstore_unregister_console(void) {}
> > #endif
> >
> > +#ifdef CONFIG_PSTORE_XBL
> > +static void pstore_register_xbl(void)
> > +{
> > + struct pstore_record record;
> > +
> > + pstore_record_init(&record, psinfo);
> > + record.type = PSTORE_TYPE_XBL;
> > + psinfo->write(&record);
> > +}
>
> This seems like a very strange way to get the record: this is an
> "empty" write that has a side-effect of reading the XBL region and
> copying it into the prz area. I would expect this to all happen in
> ramoops_pstore_read() instead.
>
> > +#else
> > +static void pstore_register_xbl(void) {}
> > +#endif
> > +
> > static int pstore_write_user_compat(struct pstore_record *record,
> > const char __user *buf)
> > {
> > @@ -616,6 +630,8 @@ int pstore_register(struct pstore_info *psi)
> > pstore_register_ftrace();
> > if (psi->flags & PSTORE_FLAGS_PMSG)
> > pstore_register_pmsg();
> > + if (psi->flags & PSTORE_FLAGS_XBL)
> > + pstore_register_xbl();
> >
> > /* Start watching for new records, if desired. */
> > if (pstore_update_ms >= 0) {
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index 1adb5e3..b114b1d 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -56,6 +56,27 @@
> > module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400);
> > MODULE_PARM_DESC(pmsg_size, "size of user space message log");
>
> How is the base address of the XBL area specified? It looks currently
> like it's up to the end user to do all the math correctly to line it
> up with where it's expected?
>
> >
> > +/*
> > + * interact with boot loader
> > + * =========================
> > + *
> > + * xbl memory layout:
> > + * +----+
> > + * |dst |
> > + * |----| +------------+
> > + * |src ----> | header |
> > + * +----+ |log messages|
> > + * +------------+
> > + *
> > + * As above, src memory is used to store header and log messages generated
> > + * by boot loader, pstore will copy the log messages to dst memory which
> > + * has same size as src. The header in src is to record log messages size
> > + * written and make xbl cookie.
>
> Why is such a copy needed? The log is already present in memory; why
> can't pstore just use what's already there?
>
> > + */
> > +static ulong ramoops_xbl_size = MIN_MEM_SIZE;
> > +module_param_named(xbl_size, ramoops_xbl_size, ulong, 0400);
> > +MODULE_PARM_DESC(xbl_size, "size of boot loader log");
> > +
> > static unsigned long long mem_address;
> > module_param_hw(mem_address, ullong, other, 0400);
> > MODULE_PARM_DESC(mem_address,
> > @@ -88,6 +109,7 @@ struct ramoops_context {
> > struct persistent_ram_zone *cprz; /* Console zone */
> > struct persistent_ram_zone **fprzs; /* Ftrace zones */
> > struct persistent_ram_zone *mprz; /* PMSG zone */
> > + struct persistent_ram_zone *bprz; /* XBL zone */
> > phys_addr_t phys_addr;
> > unsigned long size;
> > unsigned int memtype;
> > @@ -95,6 +117,7 @@ struct ramoops_context {
> > size_t console_size;
> > size_t ftrace_size;
> > size_t pmsg_size;
> > + size_t xbl_size;
> > int dump_oops;
> > u32 flags;
> > struct persistent_ram_ecc_info ecc_info;
> > @@ -106,6 +129,7 @@ struct ramoops_context {
> > unsigned int max_ftrace_cnt;
> > unsigned int ftrace_read_cnt;
> > unsigned int pmsg_read_cnt;
> > + unsigned int xbl_read_cnt;
> > struct pstore_info pstore;
> > };
> >
> > @@ -119,6 +143,7 @@ static int ramoops_pstore_open(struct pstore_info *psi)
> > cxt->console_read_cnt = 0;
> > cxt->ftrace_read_cnt = 0;
> > cxt->pmsg_read_cnt = 0;
> > + cxt->xbl_read_cnt = 0;
> > return 0;
> > }
> >
> > @@ -272,6 +297,10 @@ static ssize_t ramoops_pstore_read(struct pstore_record *record)
> > if (!prz_ok(prz) && !cxt->pmsg_read_cnt++)
> > prz = ramoops_get_next_prz(&cxt->mprz, 0 /* single */, record);
> >
> > + if (!prz_ok(prz) && !cxt->xbl_read_cnt++) {
> > + prz = ramoops_get_next_prz(&cxt->bprz, 0 /* single */, record);
> > + }
> > +
> > /* ftrace is last since it may want to dynamically allocate memory. */
> > if (!prz_ok(prz)) {
> > if (!(cxt->flags & RAMOOPS_FLAG_FTRACE_PER_CPU) &&
> > @@ -360,6 +389,26 @@ static size_t ramoops_write_kmsg_hdr(struct persistent_ram_zone *prz,
> > return len;
> > }
> >
> > +static void ramoops_get_xbl_record(struct persistent_ram_zone *prz,
> > + struct pstore_record *record,
> > + size_t xbl_size)
> > +{
> > + struct pstore_xbl_header *hdr = NULL;
> > + size_t half = xbl_size / 2;
> > + size_t max = half - PSTORE_XBL_HDR_SIZE;
> > +
> > + hdr = (struct pstore_xbl_header *)((size_t)prz->buffer + half);
> > +
> > + if (hdr->cookie == PSTORE_XBL_COOKIE) {
> > + record->buf = (char *)((size_t)hdr + PSTORE_XBL_HDR_SIZE);
> > + record->size = hdr->size_written;
> > + if (unlikely(record->size > max))
> > + record->size = max;
> > + return;
> > + }
> > + pr_debug("no valid xbl record (cookie = 0x%08x)\n", hdr->cookie);
> > +}
> > +
> > static int notrace ramoops_pstore_write(struct pstore_record *record)
> > {
> > struct ramoops_context *cxt = record->psi->data;
> > @@ -390,6 +439,16 @@ static int notrace ramoops_pstore_write(struct pstore_record *record)
> > } else if (record->type == PSTORE_TYPE_PMSG) {
> > pr_warn_ratelimited("PMSG shouldn't call %s\n", __func__);
> > return -EINVAL;
> > + } else if (record->type == PSTORE_TYPE_XBL) {
> > + if (!cxt->bprz)
> > + return -ENOMEM;
> > +
> > + ramoops_get_xbl_record(cxt->bprz, record, cxt->xbl_size);
> > + if (record->size == 0)
> > + return -EINVAL;
> > +
> > + persistent_ram_write(cxt->bprz, record->buf, record->size);
> > + return 0;
> > }
>
> This operates using a very strange side-effect (strange in the sense
> that none of the other przs work like this). I don't get the sense
> that XBL is actually a persistent ram zone: these are storage areas to
> be used for specific front-end purposes (console, oops, ftrace, etc).
> The XBL seems much more like a read-only backend, but having a
> read-only backend kind of defeats the purpose of pstore. :) This
> doesn't seem like it maps to pstore very well (it's not a storage
> area). You're looking to just expose a memory region somewhere in a
> kernel filesystem.
>
> I'm not entirely opposed to creating a new type of "thing" for pstore,
> just so the filesystem infrastructure can be reused (and it's
> conceptually similar), but it looks like the expected information
> needed to read XBL are: where in memory to find it, and what is the
> maximum expected size of the log. Extracting that directly should be a
> pretty small addition to pstore.

ok, just use it locally in my side :]

Thx.

>
> Alternatively, why not inject the XBL into the kernel printk log
> directly instead?
>
> Also, where can I find a public specification about XBL?
>