Messages in this thread |  | | From | 岩松信洋 / IWAMATSU,NOBUHIRO <> | Subject | RE: [PATCH v2 4/5] ramoops: support multiple pmsg instances | Date | Wed, 5 Oct 2016 04:13:14 +0000 |
| |
Hi,
> -----Original Message----- > From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees > Cook > Sent: Friday, September 09, 2016 6:53 AM > To: 岩松信洋 / IWAMATSU,NOBUHIRO > Cc: Anton Vorontsov; Colin Cross; Tony Luck; LKML; Hiraku Toyooka; Mark > Salyzyn; 阿口誠司 / AGUCHI,SEIJI > Subject: Re: [PATCH v2 4/5] ramoops: support multiple pmsg instances > > On Sun, Jul 24, 2016 at 8:56 PM, Nobuhiro Iwamatsu > <nobuhiro.iwamatsu.kw@hitachi.com> wrote: > > From: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com> > > > > This enables ramoops to deal with multiple pmsg instances. > > A User can configure the size of each buffers by its module parameter > > as follows. > > > > pmsg_size=0x1000,0x2000,... > > > > Then, the ramoops allocates multiple buffers and tells the number of > > buffers to pstore to create multiple /dev/pmsg[ID]. > > > > Signed-off-by: Hiraku Toyooka <hiraku.toyooka.gu@hitachi.com> > > Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.kw@hitachi.com> > > Cc: Mark Salyzyn <salyzyn@android.com> > > Cc: Seiji Aguchi <seiji.aguchi.tr@hitachi.com> > > --- > > Documentation/ramoops.txt | 22 +++++++ > > fs/pstore/ram.c | 146 > ++++++++++++++++++++++++++++++++++++++------- > > include/linux/pstore_ram.h | 8 ++- > > 3 files changed, 153 insertions(+), 23 deletions(-) > > > > diff --git a/Documentation/ramoops.txt b/Documentation/ramoops.txt > > index 5d86756..cff6ac7 100644 > > --- a/Documentation/ramoops.txt > > +++ b/Documentation/ramoops.txt > > @@ -126,3 +126,25 @@ file. Here is an example of usage: > > 0 ffffffff811d9c54 ffffffff8101a7a0 __const_udelay <- > native_machine_emergency_restart+0x110/0x1e0 > > 0 ffffffff811d9c34 ffffffff811d9c80 __delay <- > __const_udelay+0x30/0x40 > > 0 ffffffff811d9d14 ffffffff811d9c3f delay_tsc <- __delay+0xf/0x20 > > + > > +6. Pmsg support > > + > > +Ramoops supports pmsg - logging userspace mesages in persistent > > +store. By default, one pmsg area becomes available in ramoops. You > > +can write data into /dev/pmsg0, e.g.: > > + > > + # echo foo > /dev/pmsg0 > > + > > +After reboot, the stored data can be read from pmsg-ramoops-0 on the > > +pstore filesystem. > > + > > +You can specify size of the pmsg area by additional kernel command line, > e.g.: > > + > > + "ramoops.pmsg_size=0x1000" > > + > > +You can also use multiple pmsg areas, e.g.: > > + > > + "ramoops.pmsg_size=0x1000,0x2000,..." > > + > > +Then, pmsg0, pmsg1, ... will appear on /dev. This is useful to > > +control individual content aging or priority. > > The documentation for the DT bindings will need updating too, in: > Documentation/devicetree/bindings/reserved-memory/ramoops.txt
OK, I will add DT bindings to documentation.
> > > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c index 288c5d0..2bf893f > > 100644 > > --- a/fs/pstore/ram.c > > +++ b/fs/pstore/ram.c > > @@ -51,8 +51,8 @@ static ulong ramoops_ftrace_size = MIN_MEM_SIZE; > > module_param_named(ftrace_size, ramoops_ftrace_size, ulong, 0400); > > MODULE_PARM_DESC(ftrace_size, "size of ftrace log"); > > > > -static ulong ramoops_pmsg_size = MIN_MEM_SIZE; > > -module_param_named(pmsg_size, ramoops_pmsg_size, ulong, 0400); > > +static char *ramoops_pmsg_size_str; > > +module_param_named(pmsg_size, ramoops_pmsg_size_str, charp, 0400); > > MODULE_PARM_DESC(pmsg_size, "size of user space message log"); > > > > static unsigned long long mem_address; @@ -86,14 +86,14 @@ struct > > ramoops_context { > > struct persistent_ram_zone **dprzs; > > struct persistent_ram_zone *cprz; > > struct persistent_ram_zone *fprz; > > - struct persistent_ram_zone *mprz; > > + struct persistent_ram_zone **mprzs; > > phys_addr_t phys_addr; > > unsigned long size; > > unsigned int memtype; > > size_t record_size; > > size_t console_size; > > size_t ftrace_size; > > - size_t pmsg_size; > > + size_t *pmsg_size; > > int dump_oops; > > struct persistent_ram_ecc_info ecc_info; > > unsigned int max_dump_cnt; > > @@ -103,6 +103,7 @@ struct ramoops_context { > > unsigned int console_read_cnt; > > unsigned int ftrace_read_cnt; > > unsigned int pmsg_read_cnt; > > + unsigned int num_pmsg; > > struct pstore_info pstore; > > }; > > > > @@ -220,9 +221,10 @@ static ssize_t ramoops_pstore_read(u64 *id, enum > pstore_type_id *type, > > if (!prz_ok(prz)) > > prz = ramoops_get_next_prz(&cxt->fprz, > &cxt->ftrace_read_cnt, > > 1, id, type, > PSTORE_TYPE_FTRACE, 0); > > - if (!prz_ok(prz)) > > - prz = ramoops_get_next_prz(&cxt->mprz, > &cxt->pmsg_read_cnt, > > - 1, id, type, > PSTORE_TYPE_PMSG, 0); > > + while (cxt->pmsg_read_cnt < cxt->num_pmsg && !prz) > > + prz = ramoops_get_next_prz(cxt->mprzs, > &cxt->pmsg_read_cnt, > > + cxt->num_pmsg, id, type, > > + PSTORE_TYPE_PMSG, 0); > > if (!prz_ok(prz)) > > return 0; > > > > @@ -286,9 +288,11 @@ static int notrace ramoops_pstore_write_buf(enum > pstore_type_id type, > > persistent_ram_write(cxt->fprz, buf, size); > > return 0; > > } else if (type == PSTORE_TYPE_PMSG) { > > - if (!cxt->mprz) > > + if (part >= cxt->num_pmsg) > > + return -EINVAL; > > + if (!cxt->mprzs || !cxt->mprzs[part]) > > return -ENOMEM; > > - persistent_ram_write(cxt->mprz, buf, size); > > + persistent_ram_write(cxt->mprzs[part], buf, size); > > return 0; > > } > > > > @@ -348,7 +352,9 @@ static int ramoops_pstore_erase(enum pstore_type_id > type, u64 id, int count, > > prz = cxt->fprz; > > break; > > case PSTORE_TYPE_PMSG: > > - prz = cxt->mprz; > > + if (id >= cxt->num_pmsg) > > + return -EINVAL; > > + prz = cxt->mprzs[id]; > > break; > > default: > > return -EINVAL; > > @@ -426,6 +432,37 @@ fail_prz: > > return err; > > } > > > > +static int ramoops_init_mprzs(struct device *dev, struct > ramoops_context *cxt, > > + phys_addr_t *paddr, unsigned long total) > > +{ > > + int err = -ENOMEM; > > + int i; > > + > > + if (!total) > > + return 0; > > + > > + if (*paddr + total - cxt->phys_addr > cxt->size) { > > + dev_err(dev, "no room for pmsg\n"); > > + return -ENOMEM; > > + } > > + > > + cxt->mprzs = kcalloc(cxt->num_pmsg, sizeof(*cxt->mprzs), > GFP_KERNEL); > > + if (!cxt->mprzs) > > + goto fail_prz; > > + > > + for (i = 0; i < cxt->num_pmsg; i++) { > > + err = __ramoops_init_prz(dev, cxt, &cxt->mprzs[i], > paddr, > > + cxt->pmsg_size[i], 0, true); > > + if (err) > > + goto fail_prz; > > + } > > + > > + return 0; > > +fail_prz: > > + ramoops_free_przs(cxt->mprzs); > > + return err; > > +} > > + > > static int ramoops_init_prz(struct device *dev, struct ramoops_context > *cxt, > > struct persistent_ram_zone **prz, > > phys_addr_t *paddr, size_t sz, u32 sig) @@ > > -472,6 +509,8 @@ static int ramoops_probe(struct platform_device *pdev) > > size_t dump_mem_sz; > > phys_addr_t paddr; > > int err = -EINVAL; > > + unsigned long pmsg_size_total = 0; > > + unsigned int num_pmsg = 0; > > > > /* Only a single ramoops area allowed at a time, so fail extra > > * probes. > > @@ -492,8 +531,18 @@ static int ramoops_probe(struct platform_device > *pdev) > > pdata->console_size = > rounddown_pow_of_two(pdata->console_size); > > if (pdata->ftrace_size && !is_power_of_2(pdata->ftrace_size)) > > 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->pmsg_size) { > > + for (;; num_pmsg++) { > > + unsigned long size = > > + pdata->pmsg_size[num_pmsg]; > > + > > + if (!size) > > + break; > > + if (!is_power_of_2(size)) > > + pdata->pmsg_size[num_pmsg] > > + = rounddown_pow_of_two(size); > > + pmsg_size_total += size; > > + } > > + } > > > > cxt->size = pdata->mem_size; > > cxt->phys_addr = pdata->mem_address; @@ -501,17 +550,26 @@ > > static int ramoops_probe(struct platform_device *pdev) > > cxt->record_size = pdata->record_size; > > cxt->console_size = pdata->console_size; > > cxt->ftrace_size = pdata->ftrace_size; > > - cxt->pmsg_size = pdata->pmsg_size; > > cxt->dump_oops = pdata->dump_oops; > > cxt->ecc_info = pdata->ecc_info; > > + cxt->num_pmsg = num_pmsg; > > + if (num_pmsg) { > > + cxt->pmsg_size = kcalloc(num_pmsg, sizeof(size_t), > GFP_KERNEL); > > + if (!cxt->pmsg_size) { > > + err = -ENOMEM; > > + goto fail_out; > > + } > > + memcpy(cxt->pmsg_size, pdata->pmsg_size, > > + sizeof(size_t) * num_pmsg); > > + } > > > > paddr = cxt->phys_addr; > > > > dump_mem_sz = cxt->size - cxt->console_size - cxt->ftrace_size > > - - cxt->pmsg_size; > > + - pmsg_size_total; > > err = ramoops_init_dprzs(dev, cxt, &paddr, dump_mem_sz); > > if (err) > > - goto fail_out; > > + goto fail_init_dprzs; > > > > err = ramoops_init_prz(dev, cxt, &cxt->cprz, &paddr, > > cxt->console_size, 0); @@ -523,11 > > +581,12 @@ static int ramoops_probe(struct platform_device *pdev) > > if (err) > > goto fail_init_fprz; > > > > - err = ramoops_init_prz(dev, cxt, &cxt->mprz, &paddr, > cxt->pmsg_size, 0); > > + err = ramoops_init_mprzs(dev, cxt, &paddr, pmsg_size_total); > > if (err) > > - goto fail_init_mprz; > > + goto fail_init_mprzs; > > > > cxt->pstore.data = cxt; > > + cxt->pstore.num_pmsg = num_pmsg; > > /* > > * Console can handle any buffer size, so prefer LOG_LINE_MAX. > If we > > * have to handle dumps, we must have at least record_size > > buffer. And @@ -560,7 +619,6 @@ static int ramoops_probe(struct > platform_device *pdev) > > record_size = pdata->record_size; > > dump_oops = pdata->dump_oops; > > ramoops_console_size = pdata->console_size; > > - ramoops_pmsg_size = pdata->pmsg_size; > > Doesn't this mean the module parameters for the pmsg_size list won't be > visible to sysfs?
I see. I will think other solution. > > > ramoops_ftrace_size = pdata->ftrace_size; > > > > pr_info("attached 0x%lx@0x%llx, ecc: %d/%d\n", @@ -573,14 > > +631,16 @@ fail_buf: > > kfree(cxt->pstore.buf); > > fail_clear: > > cxt->pstore.bufsize = 0; > > - persistent_ram_free(cxt->mprz); > > -fail_init_mprz: > > + ramoops_free_przs(cxt->mprzs); > > +fail_init_mprzs: > > persistent_ram_free(cxt->fprz); > > fail_init_fprz: > > persistent_ram_free(cxt->cprz); > > fail_init_cprz: > > cxt->max_dump_cnt = 0; > > ramoops_free_przs(cxt->dprzs); > > +fail_init_dprzs: > > + kfree(cxt->pmsg_size); > > fail_out: > > return err; > > } > > @@ -594,8 +654,9 @@ static int ramoops_remove(struct platform_device > > *pdev) > > > > kfree(cxt->pstore.buf); > > cxt->pstore.bufsize = 0; > > + kfree(cxt->pmsg_size); > > > > - persistent_ram_free(cxt->mprz); > > + ramoops_free_przs(cxt->mprzs); > > persistent_ram_free(cxt->fprz); > > persistent_ram_free(cxt->cprz); > > ramoops_free_przs(cxt->dprzs); @@ -611,6 +672,38 @@ static > > struct platform_driver ramoops_driver = { > > }, > > }; > > > > +static unsigned long *parse_size_str(char *size_str) { > > + int i, ret; > > + unsigned long *size_array, count = 1; > > + > > + if (size_str) { > > + char *s = size_str; > > + /* Necessary array size is the number of commas + 1 */ > > + for (; (s = strchr(s, ',')); s++) > > + count++; > > + } > > + > > + size_array = kcalloc(count + 1, sizeof(unsigned long), > GFP_KERNEL); > > + if (!size_array) > > + goto out; > > + > > + if (size_str) { > > + for (i = 0; i < count; i++) { > > + ret = get_option(&size_str, (int > *)&size_array[i]); > > + if (ret == 1) > > + break; > > + else if (ret != 2) { > > + size_array[i] = 0; > > + break; > > + } > > + } > > + } else > > + size_array[0] = MIN_MEM_SIZE; > > +out: > > + return size_array; > > +} > > + > > static void ramoops_register_dummy(void) { > > if (!mem_size) > > @@ -630,7 +723,9 @@ static void ramoops_register_dummy(void) > > dummy_data->record_size = record_size; > > dummy_data->console_size = ramoops_console_size; > > dummy_data->ftrace_size = ramoops_ftrace_size; > > - dummy_data->pmsg_size = ramoops_pmsg_size; > > + dummy_data->pmsg_size = parse_size_str(ramoops_pmsg_size_str); > > I think this function is wrong, since it looks like the num_pmsg count a > bit above expects to find a NULL-terminated list. I think > parse_size_str() needs to include an extra empty array allocation to make > sure the list is always NULL-terminated.
OK, I will fix this.
> > > + if (!dummy_data->pmsg_size) > > + goto fail_pmsg_size; > > dummy_data->dump_oops = dump_oops; > > /* > > * For backwards compatibility ramoops.ecc=1 means 16 bytes > > ECC @@ -643,7 +738,13 @@ static void ramoops_register_dummy(void) > > if (IS_ERR(dummy)) { > > pr_info("could not create platform device: %ld\n", > > PTR_ERR(dummy)); > > + goto fail_pdev; > > } > > + return; > > +fail_pdev: > > + kfree(dummy_data->pmsg_size); > > +fail_pmsg_size: > > + kfree(dummy_data); > > } > > > > static int __init ramoops_init(void) > > @@ -657,6 +758,7 @@ static void __exit ramoops_exit(void) { > > platform_driver_unregister(&ramoops_driver); > > platform_device_unregister(dummy); > > + kfree(dummy_data->pmsg_size); > > kfree(dummy_data); > > } > > module_exit(ramoops_exit); > > diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h > > index 4660aaa..af7bbdd 100644 > > --- a/include/linux/pstore_ram.h > > +++ b/include/linux/pstore_ram.h > > @@ -72,6 +72,12 @@ ssize_t persistent_ram_ecc_string(struct > persistent_ram_zone *prz, > > * Ramoops platform data > > * @mem_size memory size for ramoops > > * @mem_address physical memory address to contain ramoops > > + * @pmsg_size array containing size of each pmsg area. 0 value in the > array > > + * indicates the end of the content. For example, if the > following > > + * array is given, > > + * .pmsg_size = { 0x1000, 0x2000, 0x0, 0x3000, }; > > + * then, pmsg areas are allocated for the first two size > values > > + * and '0x3000' is just ignored. > > */ > > > > struct ramoops_platform_data { > > @@ -81,7 +87,7 @@ struct ramoops_platform_data { > > unsigned long record_size; > > unsigned long console_size; > > unsigned long ftrace_size; > > - unsigned long pmsg_size; > > + unsigned long *pmsg_size; > > int dump_oops; > > struct persistent_ram_ecc_info ecc_info; }; > > -- > > 2.8.1 > > > > > > -Kees > > -- > Kees Cook > Nexus Security
Best regards, Nobuhiro
|  |