Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 22 Jun 2023 10:19:31 -0700
From:   Isaac Manjarres <isaacmanjarres@google.com>
To:     John Stultz <jstultz@google.com>
Cc:     Kees Cook <keescook@chromium.org>, Tony Luck <tony.luck@intel.com>,
        "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
        "Isaac J. Manjarres" <isaacm@codeaurora.org>,
        Mukesh Ojha <mojha@codeaurora.org>,
        Prasad Sodagudi <psodagud@codeaurora.org>,
        kernel-team@android.com, linux-hardening@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] pstore/ram: Add support for dynamically allocated
 ramoops memory regions
Message-ID: <ZJSCox1cJ1U0Qukz@google.com>
References: <20230622005213.458236-1-isaacmanjarres@google.com>
 <CANDhNCrEhx4LUPaz-FHZZJMG2yX670hk-vGTZs=HxiGR18zm5A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANDhNCrEhx4LUPaz-FHZZJMG2yX670hk-vGTZs=HxiGR18zm5A@mail.gmail.com>
Precedence: bulk

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?

Hi John,

Thanks for reviewing this patch! When you use the "alloc-ranges"
property, you specify a range for the memory region to reside in.
This means that the region will lie somewhere in between the range
you specified, but it is not guaranteed to be in the same location
across reboots.

> > 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.

How about re-wording the end as such?

"...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."


> > @@ -643,6 +644,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >  {
> >         struct device_node *of_node = pdev->dev.of_node;
> >         struct device_node *parent_node;
> > +       struct reserved_mem *rmem;
> >         struct resource *res;
> >         u32 value;
> >         int ret;
> > @@ -651,13 +653,20 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >
> >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >         if (!res) {
> > -               dev_err(&pdev->dev,
> > -                       "failed to locate DT /reserved-memory resource\n");
> > -               return -EINVAL;
> > +               rmem = of_reserved_mem_lookup(of_node);
> 
> Nit: you could keep rmem scoped locally here.

Noted! Thanks for the feedback.

> Otherwise the code looks sane, I just suspect the commit message could
> be more clear in explaining the need/utility of the dts entry using
> alloc-ranges.

Got it; please let me know if the edit I suggested earlier clarifies
things.

Thanks,
Isaac