Message-ID: <20151106011937.GA5960@leoy-linaro>
Date:	Fri, 6 Nov 2015 09:19:37 +0800
From:	Leo Yan <leo.yan@...aro.org>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Rob Herring <robherring2@...il.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Bintian Wang <bintian.wang@...wei.com>,
	Haojian Zhuang <haojian.zhuang@...aro.org>,
	Wei Xu <xuwei5@...ilicon.com>,
	Yiping Xu <xuyiping@...ilicon.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Mark Brown <broonie@...nel.org>,
	Tyler Baker <tyler.baker@...aro.org>
Subject: Re: [PATCH 1/4] arm64: dts: Reserve memory regions for hi6220

On Thu, Nov 05, 2015 at 04:13:15PM +0000, Mark Rutland wrote:
> On Thu, Nov 05, 2015 at 09:54:50PM +0800, Leo Yan wrote:
> > On Thu, Oct 29, 2015 at 04:33:01PM +0800, Leo Yan wrote:
> > > On Wed, Oct 28, 2015 at 11:32:29PM -0500, Rob Herring wrote:
> > > > On Fri, Oct 9, 2015 at 9:20 AM, Leo Yan <leo.yan@...aro.org> wrote:
> > > > > On Fri, Oct 09, 2015 at 08:50:13AM -0500, Rob Herring wrote:
> > > > >> On Fri, Oct 9, 2015 at 8:30 AM, Mark Rutland <mark.rutland@....com> wrote:
> > > > >> > On Fri, Oct 09, 2015 at 08:17:16AM -0500, Rob Herring wrote:
> > > > >> >> On Thu, Oct 8, 2015 at 11:36 PM, Leo Yan <leo.yan@...aro.org> wrote:
> > > > >> >> > On Hi6220, below memory regions in DDR have specific purpose:
> > > > >> >> >
> > > > >> >> >   0x05e0,0000 - 0x05ef,ffff: For MCU firmware using at runtime;
> > > > >> >> >   0x06df,f000 - 0x06df,ffff: For mailbox message data;
> > > > >> >> >   0x0740,f000 - 0x0740,ffff: For MCU firmware's section;
> > > > >> >> >   0x3e00,0000 - 0x3fff,ffff: For OP-TEE.
> > > > >> >> >
> > > > >> >> > This patch reserves these memory regions in DT.
> > > > >> >> >
> > > > >> >> > Signed-off-by: Leo Yan <leo.yan@...aro.org>
> > > > >> >> > ---
> > > > >> >> >  arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts | 16 ++++++++++++----
> > > > >> >> >  1 file changed, 12 insertions(+), 4 deletions(-)
> > > > >> >> >
> > > > >> >> > diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > >> >> > index e36a539..e3f4cb3 100644
> > > > >> >> > --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > >> >> > +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
> > > > >> >> > @@ -7,9 +7,6 @@
> > > > >> >> >
> > > > >> >> >  /dts-v1/;
> > > > >> >> >
> > > > >> >> > -/*Reserved 1MB memory for MCU*/
> > > > >> >> > -/memreserve/ 0x05e00000 0x00100000;
> > > > >> >> > -
> > > > >> >>
> > > > >> >> Why does memreserve not work for you? You can have multiple entries.
> > > > >> >>
> > > > >> >> >  #include "hi6220.dtsi"
> > > > >> >> >
> > > > >> >> >  / {
> > > > >> >> > @@ -24,8 +21,19 @@
> > > > >> >> >                 stdout-path = "serial0:115200n8";
> > > > >> >> >         };
> > > > >> >> >
> > > > >> >> > +       /*
> > > > >> >> > +        * Reserve below regions from memory node:
> > > > >> >> > +        *
> > > > >> >> > +        *  - 0x05e0,0000 - 0x05ef,ffff: MCU firmware runtime using
> > > > >> >> > +        *  - 0x06df,f000 - 0x06df,ffff: Mailbox message data
> > > > >> >> > +        *  - 0x0740,f000 - 0x0740,ffff: MCU firmware section
> > > > >> >> > +        *  - 0x3e00,0000 - 0x3fff,ffff: OP-TEE
> > > > >> >> > +        */
> > > > >> >> >         memory@0 {
> > > > >> >> >                 device_type = "memory";
> > > > >> >> > -               reg = <0x0 0x0 0x0 0x40000000>;
> > > > >> >> > +               reg = <0x00000000 0x00000000 0x00000000 0x05e00000>,
> > > > >> >> > +                     <0x00000000 0x05f00000 0x00000000 0x00eff000>,
> > > > >> >> > +                     <0x00000000 0x06e00000 0x00000000 0x0060f000>,
> > > > >> >> > +                     <0x00000000 0x07410000 0x00000000 0x36bf0000>;
> > > > >> >>
> > > > >> >> No, don't do this. Please use memreserve or reserved-memory binding[1]
> > > > >> >> or combination of both. Probably reserved-memory if you need the
> > > > >> >> kernel to access some of these regions.
> > > > >> >
> > > > >> > I disagree at least for those portions owned by the secure world. The
> > > > >> > kernel shouldn't map those at all, so memreserve isn't appropriate. That
> > > > >> > covers OP-TEE and the MCU firmware regions, and I'd expec the EFI memory
> > > > >> > map to not list those as available to the kernel.
> > > > >>
> > > > >> I'm fine carving out the beginning or end, but otherwise think memory
> > > > >> should correspond to the physical memory. We have a way to describe
> > > > >> holes to keep out, so we should use them. If secure world uses the DT,
> > > > >> then it would either want to know its region in memory or add the DT
> > > > >> data to say what it is using. We need that to be easy to find or easy
> > > > >> to set, respectively. The size secure world needs could vary as well.
> > > > >>
> > > > >> The fact that the kernel maps the memory is the kernel's problem, not
> > > > >> a DT problem.
> > > > >>
> > > > >
> > > > > Just give more input here. In previous time, we have long discussion [1];
> > > > > So actually your suggestion is exactly same what my old patch.
> > > > >
> > > > > From previous discussion, i think here have an assumtion: Use UEFI as
> > > > > bootloader, the kernel will ignore (or remove) memreserve and reserved-memory
> > > > > nodes, so just like Mark said "the EFI memory map to not list those
> > > > > as available to the kernel". My new patch is just to follow this and
> > > > > also make sure they have same behavior for different bootloader
> > > > > (between UEFI and uboot).
> > > > 
> > > > I've read thru the thread and see 2 main conclusions. Using
> > > > reserved-memory is problematic since things like grub don't support
> > > > that. That is fine and we should stick with /mem-reserve/ for now.
> > > 
> > > Thanks for reviewing, Rob.
> > > 
> > > One thing should note: after booting with UEFI, /memreserve/ nodes
> > > will be deleted by UEFI stub; and _ONLY_ can use /reserved-memory/
> > > node to reserve memory regions.
> > > 
> > > Ard have another patch [1], after applied this patch, then all
> > > /memreserve/ nodes and /reserved-memory/ nodes nodes will be ignored
> > > to scan after booting with UEFI stub.
> > > 
> > > This is make sense, that means UEFI need provide exactly correct memory
> > > map info by self and totally not depend on DT structures.
> > > 
> > > Another minor difference between /memreserve/ node and /reserved-memory/
> > > node is: we can add property "no-map" for /reserved-memory/; so that
> > > means it will totally remove region from memory block. it's more safe
> > > for the memory region will NOT be mapped twice with different mapping
> > > attribution.
> > > 
> > > [1] http://archive.arm.linux.org.uk/lurker/message/20150922.002128.46757034.en.html
> > > 
> > > > The other thing is the desire to have the memory presented to the kernel
> > > > be the same whether it comes from UEFI or DT structures. I can see why
> > > > there is some desire to have that alignment, but that doesn't really
> > > > buy us anything. We can't eliminate some code path in the kernel doing
> > > > so. So I still think that the memory node should reflect all of memory
> > > > as defined by the h/w and mem-reserve should be used for any software
> > > > defined reserved regions.
> > > 
> > > i think before we engaged much thinking for UEFI, that's meaningful for
> > > we found what's correct implementation for UEFI. We need make sure UEFI
> > > will do correct thing for itself.
> > > 
> > > If only consider purly from DT's usage, i have no strong opinion to
> > > stick to use memory node to carve memory regions out. It's okay for me to
> > > go back to use /reserved-memory/ to reserved regions.
> > > 
> > > Mark, do you agree with this?
> > 
> > Ping ...
> > 
> > Hi Mark,
> > 
> > Could u help confirm for this? i'm planning to resend new version
> > patch series in tommorrow, but it's better can get your feedback
> > firstly.
> 
> Sorry for the delay.
> 
> I'm still of the opinion that given the kernel has no business even
> reading this memory, it does not make sense to use a memreserve.
> 
> Given that, and the points about other software not knowing aobut
> /reserved-memory/, I don't think it makes sense to describe the region
> in the memory nodes.

Here "other software" means other OS but not Linux, right? i
rememebered before you meantioned /reserved-memory/ is dedicated for
Linux kernel and i checked ePAPR has not defined this property. If we
cannot use /reserved-memory/ currently just because other OS don't
know it, then we should firstly commit one patch to totally remove it
from Linux kernel; Haojian also brought up this question at very early
time.

Basically i'm reluctant to engage UEFI anymore for this discussion :),
but if you are meaning "other software" as UEFI, IMOH, this is also
not make sense to cannot use /reserved-memory/. Because after last
time's discussion, Haojian has implemented UEFI memory map and we
already verified it on Hikey. UEFI will create memory map for itself
and it's no matter with DT's memory node and have no dependancy with
/reserved-memory/.

But suppose /reserved-memory/ will kept in Linux kernel and and now
i'm just committing patches for Linux kernel. So i'd like to summary
some rules for memory reservasion from my own understanding (also have
posted on IRC):

- Memory node is just to describe physical memory layout; Before there
  have many discussion for DT binding should describe the hardware, in
  a fashion which is completely OS-agnostic[1]. i think it also can
  apply to memory node, we just use memory node to describe physical
  memory layout, but not describe software's usage.
- /memreserve/ is used to allow kernel to map linear virtual kernel
  address, usually it will not be used by driver; otherwise kernel
  will map the memory region twice with different memory attribution;
- /reserved-memory/ is used to the memory regions, which will be
  remove from memory block and can be mapped by driver with ioremap.

This will be easily for us follow up these rules and use it.

[1] https://lwn.net/Articles/560523/

Thanks,
Leo Yan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists