| Message ID | 1694429639-21484-9-git-send-email-quic_mojha@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Qualcomm Minidump kernel driver related support | expand |
Thanks for the response. On 9/12/2023 3:48 PM, Will Deacon wrote: > On Mon, Sep 11, 2023 at 04:23:50PM +0530, Mukesh Ojha wrote: >> The reserved memory region for ramoops is assumed to be at a fixed >> and known location when read from the devicetree. This may not be >> required for something like Qualcomm's minidump which is interested >> in knowing addresses of ramoops region but it does not put hard >> requirement of address being fixed as most of it's SoC does not >> support warm reset and does not use pstorefs at all instead it has >> firmware way of collecting ramoops region if it gets to know the >> address and register it with apss minidump table which is sitting >> in shared memory region in DDR and firmware will have access to >> these table during reset and collects it on crash of SoC. >> >> So, add the support of reserving ramoops region to be dynamically >> allocated early during boot if it is request through command line >> via 'dyn_ramoops_size=' and fill up reserved resource structure and >> export the structure, so that it can be read by ramoops driver. >> >> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >> --- >> arch/arm64/mm/init.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ > > Why does this need to be in the arch code? There's absolutely nothing > arm64-specific here. Current clients of this cmdline would be only arm64, and that is the reason of putting this here. I was checking the places where memblock_phys_alloc_range() gets called and found either it is arch/*/ and drivers/of/of_reserved_mem.c . Since, this is not related to device tree, did not find a proper place than this, i took the reference of this place from reserve_crashkernel(). Also, not sure how could it be of help to other non-arch64 users at this point. -Mukesh > > Will
On Wed, Sep 13, 2023 at 12:32:54PM +0530, Mukesh Ojha wrote: > Thanks for the response. > > On 9/12/2023 3:48 PM, Will Deacon wrote: > > On Mon, Sep 11, 2023 at 04:23:50PM +0530, Mukesh Ojha wrote: > > > The reserved memory region for ramoops is assumed to be at a fixed > > > and known location when read from the devicetree. This may not be > > > required for something like Qualcomm's minidump which is interested > > > in knowing addresses of ramoops region but it does not put hard > > > requirement of address being fixed as most of it's SoC does not > > > support warm reset and does not use pstorefs at all instead it has > > > firmware way of collecting ramoops region if it gets to know the > > > address and register it with apss minidump table which is sitting > > > in shared memory region in DDR and firmware will have access to > > > these table during reset and collects it on crash of SoC. > > > > > > So, add the support of reserving ramoops region to be dynamically > > > allocated early during boot if it is request through command line > > > via 'dyn_ramoops_size=' and fill up reserved resource structure and > > > export the structure, so that it can be read by ramoops driver. > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > > --- > > > arch/arm64/mm/init.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > Why does this need to be in the arch code? There's absolutely nothing > > arm64-specific here. > > Current clients of this cmdline would be only arm64, and that is the > reason of putting this here. I don't think that's a strong enough justification, tbh. We should at least be able to compile this for other architectures using TEST_COMPILE and so somewhere under drivers/ makes more sense to me. Will
On Tue, Sep 12, 2023 at 11:18:20AM +0100, Will Deacon wrote: > On Mon, Sep 11, 2023 at 04:23:50PM +0530, Mukesh Ojha wrote: > > The reserved memory region for ramoops is assumed to be at a fixed > > and known location when read from the devicetree. This may not be > > required for something like Qualcomm's minidump which is interested > > in knowing addresses of ramoops region but it does not put hard > > requirement of address being fixed as most of it's SoC does not > > support warm reset and does not use pstorefs at all instead it has > > firmware way of collecting ramoops region if it gets to know the > > address and register it with apss minidump table which is sitting > > in shared memory region in DDR and firmware will have access to > > these table during reset and collects it on crash of SoC. > > > > So, add the support of reserving ramoops region to be dynamically > > allocated early during boot if it is request through command line > > via 'dyn_ramoops_size=' and fill up reserved resource structure and > > export the structure, so that it can be read by ramoops driver. > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > --- > > arch/arm64/mm/init.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ > > Why does this need to be in the arch code? There's absolutely nothing > arm64-specific here. I would agree: this needs to be in ramoops itself, IMO. It should be a ramoops module argument, too. It being unhelpful for systems that don't have an external consumer is certainly true, but I think it would still make more sense for this change to live entirely within ramoops. Specifically: you're implementing a pstore backend behavioral change. In the same way that patch 10 is putting the "output" side of this into pstore/, I'd expect the "input" side also in pstore/ More comments there, though.
Sorry for the late reply, was on a long vacation. On 9/14/2023 4:47 AM, Kees Cook wrote: > On Tue, Sep 12, 2023 at 11:18:20AM +0100, Will Deacon wrote: >> On Mon, Sep 11, 2023 at 04:23:50PM +0530, Mukesh Ojha wrote: >>> The reserved memory region for ramoops is assumed to be at a fixed >>> and known location when read from the devicetree. This may not be >>> required for something like Qualcomm's minidump which is interested >>> in knowing addresses of ramoops region but it does not put hard >>> requirement of address being fixed as most of it's SoC does not >>> support warm reset and does not use pstorefs at all instead it has >>> firmware way of collecting ramoops region if it gets to know the >>> address and register it with apss minidump table which is sitting >>> in shared memory region in DDR and firmware will have access to >>> these table during reset and collects it on crash of SoC. >>> >>> So, add the support of reserving ramoops region to be dynamically >>> allocated early during boot if it is request through command line >>> via 'dyn_ramoops_size=' and fill up reserved resource structure and >>> export the structure, so that it can be read by ramoops driver. >>> >>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>> --- >>> arch/arm64/mm/init.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ >> >> Why does this need to be in the arch code? There's absolutely nothing >> arm64-specific here. > > I would agree: this needs to be in ramoops itself, IMO. It should be a > ramoops module argument, too. > > It being unhelpful for systems that don't have an external consumer is > certainly true, but I think it would still make more sense for this > change to live entirely within ramoops. Specifically: you're > implementing a pstore backend behavioral change. In the same way that > patch 10 is putting the "output" side of this into pstore/, I'd expect > the "input" side also in pstore/ How do we reserve memory? are you suggesting to use dma api's for dynamic ramoops ? -Mukesh > > More comments there, though. >
On Thu, Oct 05, 2023 at 04:52:20PM +0530, Mukesh Ojha wrote: > Sorry for the late reply, was on a long vacation. > > On 9/14/2023 4:47 AM, Kees Cook wrote: > > On Tue, Sep 12, 2023 at 11:18:20AM +0100, Will Deacon wrote: > > > On Mon, Sep 11, 2023 at 04:23:50PM +0530, Mukesh Ojha wrote: > > > > The reserved memory region for ramoops is assumed to be at a fixed > > > > and known location when read from the devicetree. This may not be > > > > required for something like Qualcomm's minidump which is interested > > > > in knowing addresses of ramoops region but it does not put hard > > > > requirement of address being fixed as most of it's SoC does not > > > > support warm reset and does not use pstorefs at all instead it has > > > > firmware way of collecting ramoops region if it gets to know the > > > > address and register it with apss minidump table which is sitting > > > > in shared memory region in DDR and firmware will have access to > > > > these table during reset and collects it on crash of SoC. > > > > > > > > So, add the support of reserving ramoops region to be dynamically > > > > allocated early during boot if it is request through command line > > > > via 'dyn_ramoops_size=' and fill up reserved resource structure and > > > > export the structure, so that it can be read by ramoops driver. > > > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > > > --- > > > > arch/arm64/mm/init.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > Why does this need to be in the arch code? There's absolutely nothing > > > arm64-specific here. > > > > I would agree: this needs to be in ramoops itself, IMO. It should be a > > ramoops module argument, too. > > > > It being unhelpful for systems that don't have an external consumer is > > certainly true, but I think it would still make more sense for this > > change to live entirely within ramoops. Specifically: you're > > implementing a pstore backend behavioral change. In the same way that > > patch 10 is putting the "output" side of this into pstore/, I'd expect > > the "input" side also in pstore/ > > How do we reserve memory? are you suggesting to use dma api's for > dynamic ramoops ? > Sharing my thoughts: Your patch is inspired from how kexec allocate memory for crash kernel right? There is a series [1] which moved arch code (ARM64/x86) to generic kexec core. Something we should also do as the feedback received here. Coming to how part, we still have to use memblock API to increase the chance of allocating contiguous memory. Since PSTORE_RAM can also be compiled as a module, we probably need another pstore layer that needs to be built statically in kernel to allocate memory using memblock API. once slab is available, all memblock API will re-direct to slab allocations. This layer can be enabled via ARCH_WANTS_PSTORE_xxx or another config that only supports 'y'. PSTORE_RAM can still be a module but when this layer is available, it supports dynamic ramoops. Another option would be just including this layer in PSTORE RAM module but take away module option when this layer is enabled. [1] https://lore.kernel.org/all/20211020020317.1220-6-thunder.leizhen@huawei.com/
On 10/5/2023 5:14 PM, Pavan Kondeti wrote: > On Thu, Oct 05, 2023 at 04:52:20PM +0530, Mukesh Ojha wrote: >> Sorry for the late reply, was on a long vacation. >> >> On 9/14/2023 4:47 AM, Kees Cook wrote: >>> On Tue, Sep 12, 2023 at 11:18:20AM +0100, Will Deacon wrote: >>>> On Mon, Sep 11, 2023 at 04:23:50PM +0530, Mukesh Ojha wrote: >>>>> The reserved memory region for ramoops is assumed to be at a fixed >>>>> and known location when read from the devicetree. This may not be >>>>> required for something like Qualcomm's minidump which is interested >>>>> in knowing addresses of ramoops region but it does not put hard >>>>> requirement of address being fixed as most of it's SoC does not >>>>> support warm reset and does not use pstorefs at all instead it has >>>>> firmware way of collecting ramoops region if it gets to know the >>>>> address and register it with apss minidump table which is sitting >>>>> in shared memory region in DDR and firmware will have access to >>>>> these table during reset and collects it on crash of SoC. >>>>> >>>>> So, add the support of reserving ramoops region to be dynamically >>>>> allocated early during boot if it is request through command line >>>>> via 'dyn_ramoops_size=' and fill up reserved resource structure and >>>>> export the structure, so that it can be read by ramoops driver. >>>>> >>>>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> >>>>> --- >>>>> arch/arm64/mm/init.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ >>>> >>>> Why does this need to be in the arch code? There's absolutely nothing >>>> arm64-specific here. >>> >>> I would agree: this needs to be in ramoops itself, IMO. It should be a >>> ramoops module argument, too. >>> >>> It being unhelpful for systems that don't have an external consumer is >>> certainly true, but I think it would still make more sense for this >>> change to live entirely within ramoops. Specifically: you're >>> implementing a pstore backend behavioral change. In the same way that >>> patch 10 is putting the "output" side of this into pstore/, I'd expect >>> the "input" side also in pstore/ >> >> How do we reserve memory? are you suggesting to use dma api's for >> dynamic ramoops ? >> > Sharing my thoughts: > > Your patch is inspired from how kexec allocate memory for crash kernel > right? Yes. > There is a series [1] which moved arch code (ARM64/x86) to > generic kexec core. Something we should also do as the feedback > received here. > > Coming to how part, we still have to use memblock API to increase the chance > of allocating contiguous memory. Since PSTORE_RAM can also be > compiled as a module, we probably need another pstore layer that needs to > be built statically in kernel to allocate memory using memblock API. > once slab is available, all memblock API will re-direct to slab > allocations. This layer can be enabled via ARCH_WANTS_PSTORE_xxx or > another config that only supports 'y'. PSTORE_RAM can still be a module but > when this layer is available, it supports dynamic ramoops. Another option > would be just including this layer in PSTORE RAM module but take away module > option when this layer is enabled. I thought about this but still the caller will be in Arch code, right ? would that be fine with others ? -Mukesh > > > [1] > https://lore.kernel.org/all/20211020020317.1220-6-thunder.leizhen@huawei.com/
On Thu, Oct 05, 2023 at 09:12:25PM +0530, Mukesh Ojha wrote: > > > On 10/5/2023 5:14 PM, Pavan Kondeti wrote: > > On Thu, Oct 05, 2023 at 04:52:20PM +0530, Mukesh Ojha wrote: > > > Sorry for the late reply, was on a long vacation. > > > > > > On 9/14/2023 4:47 AM, Kees Cook wrote: > > > > On Tue, Sep 12, 2023 at 11:18:20AM +0100, Will Deacon wrote: > > > > > On Mon, Sep 11, 2023 at 04:23:50PM +0530, Mukesh Ojha wrote: > > > > > > The reserved memory region for ramoops is assumed to be at a fixed > > > > > > and known location when read from the devicetree. This may not be > > > > > > required for something like Qualcomm's minidump which is interested > > > > > > in knowing addresses of ramoops region but it does not put hard > > > > > > requirement of address being fixed as most of it's SoC does not > > > > > > support warm reset and does not use pstorefs at all instead it has > > > > > > firmware way of collecting ramoops region if it gets to know the > > > > > > address and register it with apss minidump table which is sitting > > > > > > in shared memory region in DDR and firmware will have access to > > > > > > these table during reset and collects it on crash of SoC. > > > > > > > > > > > > So, add the support of reserving ramoops region to be dynamically > > > > > > allocated early during boot if it is request through command line > > > > > > via 'dyn_ramoops_size=' and fill up reserved resource structure and > > > > > > export the structure, so that it can be read by ramoops driver. > > > > > > > > > > > > Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> > > > > > > --- > > > > > > arch/arm64/mm/init.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > > > Why does this need to be in the arch code? There's absolutely nothing > > > > > arm64-specific here. > > > > > > > > I would agree: this needs to be in ramoops itself, IMO. It should be a > > > > ramoops module argument, too. > > > > > > > > It being unhelpful for systems that don't have an external consumer is > > > > certainly true, but I think it would still make more sense for this > > > > change to live entirely within ramoops. Specifically: you're > > > > implementing a pstore backend behavioral change. In the same way that > > > > patch 10 is putting the "output" side of this into pstore/, I'd expect > > > > the "input" side also in pstore/ > > > > > > How do we reserve memory? are you suggesting to use dma api's for > > > dynamic ramoops ? > > > > > Sharing my thoughts: > > > > Your patch is inspired from how kexec allocate memory for crash kernel > > right? > > Yes. > > > There is a series [1] which moved arch code (ARM64/x86) to > > generic kexec core. Something we should also do as the feedback > > received here. > > > > Coming to how part, we still have to use memblock API to increase the chance > > of allocating contiguous memory. Since PSTORE_RAM can also be > > compiled as a module, we probably need another pstore layer that needs to > > be built statically in kernel to allocate memory using memblock API. > > once slab is available, all memblock API will re-direct to slab > > allocations. This layer can be enabled via ARCH_WANTS_PSTORE_xxx or > > another config that only supports 'y'. PSTORE_RAM can still be a module but > > when this layer is available, it supports dynamic ramoops. Another option > > would be just including this layer in PSTORE RAM module but take away module > > option when this layer is enabled. > > I thought about this but still the caller will be in Arch code, > right ? would that be fine with others ? > The caller is not necessarily to be in the arch code. For ex: mm_core_init()->kfence_alloc_pool_and_metadata() > > > > > > [1] > > https://lore.kernel.org/all/20211020020317.1220-6-thunder.leizhen@huawei.com/
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 8a0f8604348b..680c1ce18fbe 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -31,6 +31,7 @@ #include <linux/hugetlb.h> #include <linux/acpi_iort.h> #include <linux/kmemleak.h> +#include <linux/pstore_ram.h> #include <asm/boot.h> #include <asm/fixmap.h> @@ -100,6 +101,93 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit; #define ARM64_MEMSTART_ALIGN (1UL << ARM64_MEMSTART_SHIFT) #endif +#define RAMOOPS_ADDR_HIGH_MAX (PHYS_MASK + 1) + +/* Location of the reserved area for the dynamic ramoops */ +struct resource dyn_ramoops_res = { + .name = "ramoops", + .start = 0, + .end = 0, + .flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM, + .desc = IORES_DESC_NONE, +}; +EXPORT_SYMBOL(dyn_ramoops_res); + +static int __init parse_dynamic_ramoops(char *cmdline, unsigned long long *size) +{ + const char *name = "dyn_ramoops_size="; + char *p = NULL; + char *q = NULL; + char *tmp; + + if (!cmdline) + return -ENOENT; + + /* Check for "dyn_ramoops_size" and use the later if there are more */ + p = strstr(cmdline, name); + while (p) { + q = p; + p = strchr(p, ' '); + if (!p) + break; + + p = strstr(p + 1, name); + } + + if (!q) { + pr_err("ramoops: No entry found for %s\n", name); + return -ENOENT; + } + + p = q + strlen(name); + *size = memparse(p, &tmp); + if (p == tmp) { + pr_err("ramoops: memory value expected\n"); + return -EINVAL; + } + + return 0; +} + +static int __init parse_dyn_ramoops_size_dummy(char *arg) +{ + return 0; +} +early_param("dyn_ramoops_size", parse_dyn_ramoops_size_dummy); + +/* + * reserve_dynamic_ramoops() - reserves memory for dynamic ramoops + * + * This enable dynamic reserve memory support for ramoops through + * command line. + */ +static void __init reserve_dynamic_ramoops(void) +{ + char *cmdline = boot_command_line; + unsigned long long ramoops_base; + unsigned long long ramoops_size; + + if (!IS_ENABLED(CONFIG_PSTORE_RAM)) + return; + + if (parse_dynamic_ramoops(cmdline, &ramoops_size)) + return; + + ramoops_base = memblock_phys_alloc_range(ramoops_size, SMP_CACHE_BYTES, + 0, RAMOOPS_ADDR_HIGH_MAX); + if (!ramoops_base) { + pr_err("cannot allocate ramoops dynamic memory (size:0x%llx).\n", + ramoops_size); + return; + } + + kmemleak_ignore_phys(ramoops_base); + + dyn_ramoops_res.start = ramoops_base; + dyn_ramoops_res.end = ramoops_base + ramoops_size - 1; + insert_resource(&iomem_resource, &dyn_ramoops_res); +} + static int __init reserve_crashkernel_low(unsigned long long low_size) { unsigned long long low_base; @@ -481,6 +569,12 @@ void __init bootmem_init(void) */ reserve_crashkernel(); + /* + * Reserving ramoops region resource dynamically in case it is + * requested from command line. + */ + reserve_dynamic_ramoops(); + memblock_dump_all(); } diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h index 9d65ff94e216..07d700b7649d 100644 --- a/include/linux/pstore_ram.h +++ b/include/linux/pstore_ram.h @@ -10,6 +10,8 @@ #include <linux/pstore.h> +extern struct resource dyn_ramoops_res; + struct persistent_ram_ecc_info { int block_size; int ecc_size;
The reserved memory region for ramoops is assumed to be at a fixed and known location when read from the devicetree. This may not be required for something like Qualcomm's minidump which is interested in knowing addresses of ramoops region but it does not put hard requirement of address being fixed as most of it's SoC does not support warm reset and does not use pstorefs at all instead it has firmware way of collecting ramoops region if it gets to know the address and register it with apss minidump table which is sitting in shared memory region in DDR and firmware will have access to these table during reset and collects it on crash of SoC. So, add the support of reserving ramoops region to be dynamically allocated early during boot if it is request through command line via 'dyn_ramoops_size=' and fill up reserved resource structure and export the structure, so that it can be read by ramoops driver. Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com> --- arch/arm64/mm/init.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++ include/linux/pstore_ram.h | 2 + 2 files changed, 96 insertions(+)