All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH] pstore: Add boot loader log messages support
       [not found] <5c5403a5.1c69fb81.e34c0.9a13SMTPIN_ADDED_BROKEN@mx.google.com>
@ 2019-02-12 20:43 ` Kees Cook
       [not found]   ` <5c6639a7.1c69fb81.b13a7.4bf0SMTPIN_ADDED_BROKEN@mx.google.com>
       [not found]   ` <5c6639a8.1c69fb81.7604b.22e8SMTPIN_ADDED_BROKEN@mx.google.com>
  0 siblings, 2 replies; 3+ messages in thread
From: Kees Cook @ 2019-02-12 20:43 UTC (permalink / raw)
  To: Yue Hu; +Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML

On Fri, Feb 1, 2019 at 12:30 AM Yue Hu <huyue2@yulong.com> wrote:
>
> From bac8bbcd6081b967422dc82074a41098a0cf5180 Mon Sep 17 00:00:00 2001
> From: Yue Hu <huyue2@yulong.com>
> Date: Tue, 29 Jan 2019 11:42:27 +0800
> Subject: [PATCH] pstore: Add boot loader log messages support
>
> 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.
>
> Therefore, let's add a logging support: PSTORE_TYPE_XBL.
>
> Signed-off-by: Yue Hu <huyue2@yulong.com>
> ---
>  fs/pstore/Kconfig      | 10 ++++++++
>  fs/pstore/platform.c   | 16 +++++++++++++
>  fs/pstore/ram.c        | 64 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/pstore.h | 21 +++++++++++++----
>  4 files changed, 104 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 0ca7657..7486e48 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -66,6 +66,7 @@
>         "mce",
>         "console",
>         "ftrace",
> +       "xbl",
>         "rtas",
>         "powerpc-ofw",
>         "powerpc-common",
> @@ -532,6 +533,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 writes a zero-length record at registration time. Why?

> +#else
> +static void pstore_register_xbl(void) {}
> +#endif
> +
>  static int pstore_write_user_compat(struct pstore_record *record,
>                                     const char __user *buf)
>  {
> @@ -618,6 +632,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 ca01778..0bb086b 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -56,6 +56,10 @@
>  module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400);
>  MODULE_PARM_DESC(pmsg_size, "size of user space message log");
>
> +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 +92,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 +100,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 +112,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 +126,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 +280,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 +372,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);

Before reading this data, please verify the prz buffer sizes (since
prior boots may have had a different size, etc).

> +
> +       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 +422,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;
>         }
>
>         if (record->type != PSTORE_TYPE_DMESG)
> @@ -469,6 +511,9 @@ static int ramoops_pstore_erase(struct pstore_record *record)
>         case PSTORE_TYPE_PMSG:
>                 prz = cxt->mprz;
>                 break;
> +       case PSTORE_TYPE_XBL:
> +               prz = cxt->bprz;
> +               break;
>         default:
>                 return -EINVAL;
>         }
> @@ -697,6 +742,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>         parse_size("console-size", pdata->console_size);
>         parse_size("ftrace-size", pdata->ftrace_size);
>         parse_size("pmsg-size", pdata->pmsg_size);
> +       parse_size("xbl-size", pdata->xbl_size);
>         parse_size("ecc-size", pdata->ecc_info.ecc_size);
>         parse_size("flags", pdata->flags);
>
> @@ -740,7 +786,7 @@ static int ramoops_probe(struct platform_device *pdev)
>         }
>
>         if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size &&
> -                       !pdata->ftrace_size && !pdata->pmsg_size)) {
> +               !pdata->ftrace_size && !pdata->pmsg_size && !pdata->xbl_size)) {
>                 pr_err("The memory size and the record/console size must be "
>                         "non-zero\n");
>                 goto fail_out;
> @@ -754,6 +800,8 @@ static int ramoops_probe(struct platform_device *pdev)
>                 pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
>         if (pdata->pmsg_size && !is_power_of_2(pdata->pmsg_size))
>                 pdata->pmsg_size = rounddown_pow_of_two(pdata->pmsg_size);
> +       if (pdata->xbl_size && !is_power_of_2(pdata->xbl_size))
> +               pdata->xbl_size = rounddown_pow_of_two(pdata->xbl_size);
>
>         cxt->size = pdata->mem_size;
>         cxt->phys_addr = pdata->mem_address;
> @@ -762,6 +810,7 @@ static int ramoops_probe(struct platform_device *pdev)
>         cxt->console_size = pdata->console_size;
>         cxt->ftrace_size = pdata->ftrace_size;
>         cxt->pmsg_size = pdata->pmsg_size;
> +       cxt->xbl_size = pdata->xbl_size;
>         cxt->dump_oops = pdata->dump_oops;
>         cxt->flags = pdata->flags;
>         cxt->ecc_info = pdata->ecc_info;
> @@ -769,7 +818,7 @@ static int ramoops_probe(struct platform_device *pdev)
>         paddr = cxt->phys_addr;
>
>         dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
> -                       - cxt->pmsg_size;
> +                       - cxt->pmsg_size - cxt->xbl_size;
>         err = ramoops_init_przs("dmesg", dev, cxt, &cxt->dprzs, &paddr,
>                                 dump_mem_sz, cxt->record_size,
>                                 &cxt->max_dump_cnt, 0, 0);
> @@ -797,6 +846,11 @@ static int ramoops_probe(struct platform_device *pdev)
>         if (err)
>                 goto fail_init_mprz;
>
> +       err = ramoops_init_prz("xbl", dev, cxt, &cxt->bprz, &paddr,
> +                               cxt->xbl_size, 0);
> +       if (err)
> +               goto fail_init_bprz;
> +
>         cxt->pstore.data = cxt;
>         /*
>          * Since bufsize is only used for dmesg crash dumps, it
> @@ -818,6 +872,8 @@ static int ramoops_probe(struct platform_device *pdev)
>                 cxt->pstore.flags |= PSTORE_FLAGS_FTRACE;
>         if (cxt->pmsg_size)
>                 cxt->pstore.flags |= PSTORE_FLAGS_PMSG;
> +       if (cxt->xbl_size)
> +               cxt->pstore.flags |= PSTORE_FLAGS_XBL;
>
>         err = pstore_register(&cxt->pstore);
>         if (err) {
> @@ -835,6 +891,7 @@ static int ramoops_probe(struct platform_device *pdev)
>         dump_oops = pdata->dump_oops;
>         ramoops_console_size = pdata->console_size;
>         ramoops_pmsg_size = pdata->pmsg_size;
> +       ramoops_xbl_size = pdata->xbl_size;
>         ramoops_ftrace_size = pdata->ftrace_size;
>
>         pr_info("using 0x%lx@0x%llx, ecc: %d\n",
> @@ -847,6 +904,8 @@ static int ramoops_probe(struct platform_device *pdev)
>         kfree(cxt->pstore.buf);
>  fail_clear:
>         cxt->pstore.bufsize = 0;
> +       persistent_ram_free(cxt->bprz);
> +fail_init_bprz:
>         persistent_ram_free(cxt->mprz);
>  fail_init_mprz:
>  fail_init_fprz:
> @@ -915,6 +974,7 @@ static void __init ramoops_register_dummy(void)
>         pdata.console_size = ramoops_console_size;
>         pdata.ftrace_size = ramoops_ftrace_size;
>         pdata.pmsg_size = ramoops_pmsg_size;
> +       pdata.xbl_size = ramoops_xbl_size;
>         pdata.dump_oops = dump_oops;
>         pdata.flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
>
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index b146181..e016bbf 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -43,13 +43,14 @@ enum pstore_type_id {
>         PSTORE_TYPE_MCE         = 1,
>         PSTORE_TYPE_CONSOLE     = 2,
>         PSTORE_TYPE_FTRACE      = 3,
> +       PSTORE_TYPE_XBL         = 4,

Instead of reordering, please just append after PPC_OPAL.

>         /* PPC64-specific partition types */
> -       PSTORE_TYPE_PPC_RTAS    = 4,
> -       PSTORE_TYPE_PPC_OF      = 5,
> -       PSTORE_TYPE_PPC_COMMON  = 6,
> -       PSTORE_TYPE_PMSG        = 7,
> -       PSTORE_TYPE_PPC_OPAL    = 8,
> +       PSTORE_TYPE_PPC_RTAS    = 5,
> +       PSTORE_TYPE_PPC_OF      = 6,
> +       PSTORE_TYPE_PPC_COMMON  = 7,
> +       PSTORE_TYPE_PMSG        = 8,
> +       PSTORE_TYPE_PPC_OPAL    = 9,
>
>         /* End of the list */
>         PSTORE_TYPE_MAX
> @@ -207,10 +208,20 @@ struct pstore_info {
>  #define PSTORE_FLAGS_CONSOLE   BIT(1)
>  #define PSTORE_FLAGS_FTRACE    BIT(2)
>  #define PSTORE_FLAGS_PMSG      BIT(3)
> +#define PSTORE_FLAGS_XBL       BIT(4)
>
>  extern int pstore_register(struct pstore_info *);
>  extern void pstore_unregister(struct pstore_info *);
>
> +#define PSTORE_XBL_COOKIE      0x4c425850 /* "PXBL" in ASCII */
> +
> +struct pstore_xbl_header {
> +       uint32_t cookie;
> +       uint32_t size_written;
> +};
> +
> +#define PSTORE_XBL_HDR_SIZE    sizeof(struct pstore_xbl_header)
> +
>  struct pstore_ftrace_record {
>         unsigned long ip;
>         unsigned long parent_ip;

I don't see anything that actually _writes_ the PXBL? How does pstore
interact with the boot loader to get the logs?

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] pstore: Add boot loader log messages support
       [not found]   ` <5c6639a7.1c69fb81.b13a7.4bf0SMTPIN_ADDED_BROKEN@mx.google.com>
@ 2019-02-15  4:04     ` Kees Cook
  0 siblings, 0 replies; 3+ messages in thread
From: Kees Cook @ 2019-02-15  4:04 UTC (permalink / raw)
  To: Yue Hu; +Cc: Anton Vorontsov, Colin Cross, Tony Luck, LKML, Yue Hu

On Thu, Feb 14, 2019 at 8:01 PM Yue Hu <huyue2@yulong.com> wrote:
> Yes, boot loader will did the _write_. When booting from power on, the first phase
> is boot loader, it will log to specific reserver memory. Then kernel/pstore is
> coming up, pstore will copy the log generated by boot loader to xbl zone. The xbl
> memory zone layout like below:
>
> +-----+---------
> |dest |
> |-----|  xbl zone
> |src  |
> +-----+---------
>
> Anyway, i think it is a useful debug feature.

Ah-ha, interesting. Okay, please mention this (and how the boot loader
knows to write in the area) in v2.

Thanks!

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] pstore: Add boot loader log messages support
       [not found]   ` <5c6639a8.1c69fb81.7604b.22e8SMTPIN_ADDED_BROKEN@mx.google.com>
@ 2019-02-18  8:00     ` Yue Hu
  0 siblings, 0 replies; 3+ messages in thread
From: Yue Hu @ 2019-02-18  8:00 UTC (permalink / raw)
  To: Kees Cook; +Cc: Yue Hu, Anton Vorontsov, Colin Cross, Tony Luck, LKML

On Fri, 15 Feb 2019 12:01:30 +0800
Yue Hu <huyue2@yulong.com> wrote:

> On Tue, 12 Feb 2019 12:43:36 -0800
> Kees Cook <keescook@chromium.org> wrote:
> 
> > On Fri, Feb 1, 2019 at 12:30 AM Yue Hu <huyue2@yulong.com> wrote:  
> > >
> > > From bac8bbcd6081b967422dc82074a41098a0cf5180 Mon Sep 17 00:00:00 2001
> > > From: Yue Hu <huyue2@yulong.com>
> > > Date: Tue, 29 Jan 2019 11:42:27 +0800
> > > Subject: [PATCH] pstore: Add boot loader log messages support
> > >
> > > 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.
> > >
> > > Therefore, let's add a logging support: PSTORE_TYPE_XBL.
> > >
> > > Signed-off-by: Yue Hu <huyue2@yulong.com>
> > > ---
> > >  fs/pstore/Kconfig      | 10 ++++++++
> > >  fs/pstore/platform.c   | 16 +++++++++++++
> > >  fs/pstore/ram.c        | 64 ++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  include/linux/pstore.h | 21 +++++++++++++----
> > >  4 files changed, 104 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 0ca7657..7486e48 100644
> > > --- a/fs/pstore/platform.c
> > > +++ b/fs/pstore/platform.c
> > > @@ -66,6 +66,7 @@
> > >         "mce",
> > >         "console",
> > >         "ftrace",
> > > +       "xbl",
> > >         "rtas",
> > >         "powerpc-ofw",
> > >         "powerpc-common",
> > > @@ -532,6 +533,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 writes a zero-length record at registration time. Why?  
> 
> It's related to interact with boot loader. Write callback will get the
> real record size which is set by boot loader.
> 
> >   
> > > +#else
> > > +static void pstore_register_xbl(void) {}
> > > +#endif
> > > +
> > >  static int pstore_write_user_compat(struct pstore_record *record,
> > >                                     const char __user *buf)
> > >  {
> > > @@ -618,6 +632,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 ca01778..0bb086b 100644
> > > --- a/fs/pstore/ram.c
> > > +++ b/fs/pstore/ram.c
> > > @@ -56,6 +56,10 @@
> > >  module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400);
> > >  MODULE_PARM_DESC(pmsg_size, "size of user space message log");
> > >
> > > +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 +92,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 +100,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 +112,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 +126,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 +280,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 +372,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);    
> > 
> > Before reading this data, please verify the prz buffer sizes (since
> > prior boots may have had a different size, etc).
> >   
> 
> ok, patch v2 will fix it.
> 

I check code again, here we just need to get xbl header address, seems do not need
to verify the prz buffer size. 

Thanks.

> > > +
> > > +       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 +422,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;
> > >         }
> > >
> > >         if (record->type != PSTORE_TYPE_DMESG)
> > > @@ -469,6 +511,9 @@ static int ramoops_pstore_erase(struct pstore_record *record)
> > >         case PSTORE_TYPE_PMSG:
> > >                 prz = cxt->mprz;
> > >                 break;
> > > +       case PSTORE_TYPE_XBL:
> > > +               prz = cxt->bprz;
> > > +               break;
> > >         default:
> > >                 return -EINVAL;
> > >         }
> > > @@ -697,6 +742,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> > >         parse_size("console-size", pdata->console_size);
> > >         parse_size("ftrace-size", pdata->ftrace_size);
> > >         parse_size("pmsg-size", pdata->pmsg_size);
> > > +       parse_size("xbl-size", pdata->xbl_size);
> > >         parse_size("ecc-size", pdata->ecc_info.ecc_size);
> > >         parse_size("flags", pdata->flags);
> > >
> > > @@ -740,7 +786,7 @@ static int ramoops_probe(struct platform_device *pdev)
> > >         }
> > >
> > >         if (!pdata->mem_size || (!pdata->record_size && !pdata->console_size &&
> > > -                       !pdata->ftrace_size && !pdata->pmsg_size)) {
> > > +               !pdata->ftrace_size && !pdata->pmsg_size && !pdata->xbl_size)) {
> > >                 pr_err("The memory size and the record/console size must be "
> > >                         "non-zero\n");
> > >                 goto fail_out;
> > > @@ -754,6 +800,8 @@ static int ramoops_probe(struct platform_device *pdev)
> > >                 pdata->ftrace_size = rounddown_pow_of_two(pdata->ftrace_size);
> > >         if (pdata->pmsg_size && !is_power_of_2(pdata->pmsg_size))
> > >                 pdata->pmsg_size = rounddown_pow_of_two(pdata->pmsg_size);
> > > +       if (pdata->xbl_size && !is_power_of_2(pdata->xbl_size))
> > > +               pdata->xbl_size = rounddown_pow_of_two(pdata->xbl_size);
> > >
> > >         cxt->size = pdata->mem_size;
> > >         cxt->phys_addr = pdata->mem_address;
> > > @@ -762,6 +810,7 @@ static int ramoops_probe(struct platform_device *pdev)
> > >         cxt->console_size = pdata->console_size;
> > >         cxt->ftrace_size = pdata->ftrace_size;
> > >         cxt->pmsg_size = pdata->pmsg_size;
> > > +       cxt->xbl_size = pdata->xbl_size;
> > >         cxt->dump_oops = pdata->dump_oops;
> > >         cxt->flags = pdata->flags;
> > >         cxt->ecc_info = pdata->ecc_info;
> > > @@ -769,7 +818,7 @@ static int ramoops_probe(struct platform_device *pdev)
> > >         paddr = cxt->phys_addr;
> > >
> > >         dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size
> > > -                       - cxt->pmsg_size;
> > > +                       - cxt->pmsg_size - cxt->xbl_size;
> > >         err = ramoops_init_przs("dmesg", dev, cxt, &cxt->dprzs, &paddr,
> > >                                 dump_mem_sz, cxt->record_size,
> > >                                 &cxt->max_dump_cnt, 0, 0);
> > > @@ -797,6 +846,11 @@ static int ramoops_probe(struct platform_device *pdev)
> > >         if (err)
> > >                 goto fail_init_mprz;
> > >
> > > +       err = ramoops_init_prz("xbl", dev, cxt, &cxt->bprz, &paddr,
> > > +                               cxt->xbl_size, 0);
> > > +       if (err)
> > > +               goto fail_init_bprz;
> > > +
> > >         cxt->pstore.data = cxt;
> > >         /*
> > >          * Since bufsize is only used for dmesg crash dumps, it
> > > @@ -818,6 +872,8 @@ static int ramoops_probe(struct platform_device *pdev)
> > >                 cxt->pstore.flags |= PSTORE_FLAGS_FTRACE;
> > >         if (cxt->pmsg_size)
> > >                 cxt->pstore.flags |= PSTORE_FLAGS_PMSG;
> > > +       if (cxt->xbl_size)
> > > +               cxt->pstore.flags |= PSTORE_FLAGS_XBL;
> > >
> > >         err = pstore_register(&cxt->pstore);
> > >         if (err) {
> > > @@ -835,6 +891,7 @@ static int ramoops_probe(struct platform_device *pdev)
> > >         dump_oops = pdata->dump_oops;
> > >         ramoops_console_size = pdata->console_size;
> > >         ramoops_pmsg_size = pdata->pmsg_size;
> > > +       ramoops_xbl_size = pdata->xbl_size;
> > >         ramoops_ftrace_size = pdata->ftrace_size;
> > >
> > >         pr_info("using 0x%lx@0x%llx, ecc: %d\n",
> > > @@ -847,6 +904,8 @@ static int ramoops_probe(struct platform_device *pdev)
> > >         kfree(cxt->pstore.buf);
> > >  fail_clear:
> > >         cxt->pstore.bufsize = 0;
> > > +       persistent_ram_free(cxt->bprz);
> > > +fail_init_bprz:
> > >         persistent_ram_free(cxt->mprz);
> > >  fail_init_mprz:
> > >  fail_init_fprz:
> > > @@ -915,6 +974,7 @@ static void __init ramoops_register_dummy(void)
> > >         pdata.console_size = ramoops_console_size;
> > >         pdata.ftrace_size = ramoops_ftrace_size;
> > >         pdata.pmsg_size = ramoops_pmsg_size;
> > > +       pdata.xbl_size = ramoops_xbl_size;
> > >         pdata.dump_oops = dump_oops;
> > >         pdata.flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
> > >
> > > diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> > > index b146181..e016bbf 100644
> > > --- a/include/linux/pstore.h
> > > +++ b/include/linux/pstore.h
> > > @@ -43,13 +43,14 @@ enum pstore_type_id {
> > >         PSTORE_TYPE_MCE         = 1,
> > >         PSTORE_TYPE_CONSOLE     = 2,
> > >         PSTORE_TYPE_FTRACE      = 3,
> > > +       PSTORE_TYPE_XBL         = 4,    
> > 
> > Instead of reordering, please just append after PPC_OPAL.  
> 
> i think PSTORE_TYPE_XBL belongs frontend storage type, so i appended there.
> 
> >   
> > >         /* PPC64-specific partition types */
> > > -       PSTORE_TYPE_PPC_RTAS    = 4,
> > > -       PSTORE_TYPE_PPC_OF      = 5,
> > > -       PSTORE_TYPE_PPC_COMMON  = 6,
> > > -       PSTORE_TYPE_PMSG        = 7,
> > > -       PSTORE_TYPE_PPC_OPAL    = 8,
> > > +       PSTORE_TYPE_PPC_RTAS    = 5,
> > > +       PSTORE_TYPE_PPC_OF      = 6,
> > > +       PSTORE_TYPE_PPC_COMMON  = 7,
> > > +       PSTORE_TYPE_PMSG        = 8,
> > > +       PSTORE_TYPE_PPC_OPAL    = 9,
> > >
> > >         /* End of the list */
> > >         PSTORE_TYPE_MAX
> > > @@ -207,10 +208,20 @@ struct pstore_info {
> > >  #define PSTORE_FLAGS_CONSOLE   BIT(1)
> > >  #define PSTORE_FLAGS_FTRACE    BIT(2)
> > >  #define PSTORE_FLAGS_PMSG      BIT(3)
> > > +#define PSTORE_FLAGS_XBL       BIT(4)
> > >
> > >  extern int pstore_register(struct pstore_info *);
> > >  extern void pstore_unregister(struct pstore_info *);
> > >
> > > +#define PSTORE_XBL_COOKIE      0x4c425850 /* "PXBL" in ASCII */
> > > +
> > > +struct pstore_xbl_header {
> > > +       uint32_t cookie;
> > > +       uint32_t size_written;
> > > +};
> > > +
> > > +#define PSTORE_XBL_HDR_SIZE    sizeof(struct pstore_xbl_header)
> > > +
> > >  struct pstore_ftrace_record {
> > >         unsigned long ip;
> > >         unsigned long parent_ip;    
> > 
> > I don't see anything that actually _writes_ the PXBL? How does pstore
> > interact with the boot loader to get the logs?
> >   
> 
> Yes, boot loader will did the _write_. When booting from power on, the first phase
> is boot loader, it will log to specific reserver memory. Then kernel/pstore is
> coming up, pstore will copy the log generated by boot loader to xbl zone. The xbl
> memory zone layout like below:
> 
> +-----+---------
> |dest |
> |-----|  xbl zone
> |src  |
> +-----+---------
> 
> Anyway, i think it is a useful debug feature.
> 
> 
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-02-18  8:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <5c5403a5.1c69fb81.e34c0.9a13SMTPIN_ADDED_BROKEN@mx.google.com>
2019-02-12 20:43 ` [PATCH] pstore: Add boot loader log messages support Kees Cook
     [not found]   ` <5c6639a7.1c69fb81.b13a7.4bf0SMTPIN_ADDED_BROKEN@mx.google.com>
2019-02-15  4:04     ` Kees Cook
     [not found]   ` <5c6639a8.1c69fb81.7604b.22e8SMTPIN_ADDED_BROKEN@mx.google.com>
2019-02-18  8:00     ` Yue Hu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.