Re: [PATCH] pstore/ram: Add support for dynamically allocated ramoops memory regions

[Mukesh Ojha]

On 6/23/2023 1:21 AM, Elliot Berman wrote:
> On 6/22/2023 10:58 AM, Kees Cook wrote:
> > On June 22, 2023 10:26:35 AM PDT, Isaac Manjarres 
> > <isaacmanjarres@xxxxxxxxxx> wrote:
> > > On Wed, Jun 21, 2023 at 10:15:45PM -0700, Kees Cook wrote:
> > > > On Wed, Jun 21, 2023 at 09:47:26PM -0700, John Stultz wrote:
> > > > > > The reserved memory region for ramoops is assumed to be at a fixed
> > > > > > and known location when read from the devicetree. This is not 
> > > > > > desirable
> > > > > > in environments where it is preferred for the region to be 
> > > > > > dynamically
> > > > > > allocated early during boot (i.e. the memory region is defined with
> > > > > > the "alloc-ranges" property instead of the "reg" property).
> > > > >
> > > > > Thanks for sending this out, Isaac!
> > > > >
> > > > > Apologies, I've forgotten much of the details around dt bindings here,
> > > > > so forgive my questions:
> > > > > If the memory is dynamically allocated from a specific range, is it
> > > > > guaranteed to be consistently the same address boot to boot?
> > > > >
> > > > > > Since ramoops regions are part of the reserved-memory devicetree
> > > > > > node, they exist in the reserved_mem array. This means that the
> > > > > > of_reserved_mem_lookup() function can be used to retrieve the
> > > > > > reserved_mem structure for the ramoops region, and that structure
> > > > > > contains the base and size of the region, even if it has been
> > > > > > dynamically allocated.
> > > > >
> > > > > I think this is answering my question above, but it's a little opaque,
> > > > > so I'm not sure.
> > > >
> > > > Yeah, I had exactly the same question: will this be the same
> > > > boot-to-boot?
> > >
> > > Hi Kees,
> > >
> > > Thank you for taking a look at this patch and for your review! When the
> > > alloc-ranges property is used to describe a memory region, the memory
> > > region will always be allocated within that range, but it's not
> > > guaranteed to be allocated at the same base address across reboots.
> > >
> > > I had proposed re-wording the end of the commit message in my response
> > > to John as follows:
> > >
> > > "...and that structure contains the address of the base of the region
> > > that was allocated at boot anywhere within the range specified by the
> > > "alloc-ranges" devicetree property."
> > >
> > > Does that clarify things better?
> >
> > I am probably misunderstanding something still, but it it varies from 
> > boot to boot, what utility is there for pstore if it changes? I.e. the 
> > things written during the last boot would then no longer accessible at 
> > the next boot? E.g.:
> >
> > Boot 1.
> > Get address Foo.
> > Crash, write to Foo.
> > Boot 2.
> > Get address Bar, different from Foo.
> > Nothing found at Bar, so nothing populated in pstorefs; crash report 
> > from Boot 1 unavailable.
> >
> > I feel like there is something I don't understand about the Foo/Bar 
> > addresses in my example.
>
> I believe this is being added to support the QCOM SoC minidump feature. 
> Mukesh has posted it on the mailing lists here:
>
> https://lore.kernel.org/all/1683133352-10046-1-git-send-email-quic_mojha@xxxxxxxxxxx/
>
> https://lore.kernel.org/all/1683133352-10046-10-git-send-email-quic_mojha@xxxxxxxxxxx/
>
> Mukesh, could you comment whether this patch is wanted for us in the 
> version you have posted? It looks like maybe not based on the commit 
> text in patch #9.

No, this is no needed after patch #9 .

I have tried multiple attempt already to get this patch in

https://lore.kernel.org/lkml/1675330081-15029-2-git-send-email-quic_mojha@xxxxxxxxxxx/

later tried the approach of patch #9 along with minidump series..


- Mukesh

>   - Elliot