On 28 July 2015 at 11:41, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>
>
> On 28 July 2015 at 10:26, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On 28 July 2015 at 11:01, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>> > [+ Tixy as he's interested in making sure UEFI follows the Linux
>> > requirements]
>> >
>> > On 28 July 2015 at 07:39, Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > wrote:

>>
>> >>
>> >> On 27 July 2015 at 22:42, Ryan Harkin <ryan.harkin@linaro.org> wrote:
>> >> > Device tree files in recent kernels (eg. Linux 4.2) can be >16KB.
>> >> >
>> >> > The max offset of 0x4000 meant that the device tree would be
>> >> > allocated
>> >> > at a "random address", which more often than not was above the
>> >> > recommended 128MiB boundary.
>> >> >
>> >>
>> >> From Documentation/arm/Booting:
>> >>
>> >> """
>> >> 4b. Setup the device tree
>> >> ...
>> >> A safe location is just above the 128MiB boundary from start of RAM.
>> >> """
>> >>
>> >> i.e., the documented protocol explicitly suggests using an address >
>> >> 128
>> >> MB.
>> >> So what exactly goes wrong if you load it at a random address?
>> >
>> >
>> > For some reason, I've been reading this as "before" 128MiB.  How strange
>> > of
>> > me.
>> >
>> > The advice I was following was from the bottom of the email thread where
>> > Nicolas Mitre says:
>> >
>> > "You can therefore load everything (zImage, initrd, DTB) sequentially
>> > from the 32MB mark in RAM and be safe."
>> >
>> > But mostly, I was trying to keep the bottom 32MB unused.
>> >
>>
>> Yes, I think that is the primary requirement here.
>>
>> > We don't have the option to load sequentially from 32MB unless I change
>> > the
>> > code a lot more.  I've already tried a hack that placed the 3 files
>> > sequentially from 0x82000000 on TC2 and it works well, although it's
>> > technically wrong because I didn't explicity reserve memory in those
>> > areas
>> > to stop UEFI from using it.
>> >
>> > I'll try changing the max offsets to be all above 128MiB and see if it
>> > still
>> > works.  As Tixy pointed out to me, the problem I had with the Linux
>> > Loader's
>> > "random address" for the DTB is that it was always in the same region
>> > that
>> > Linux reserves for CMA.  And I think that starts before the DTB is
>> > finished
>> > with.
>> >
>>
>> I think we could simply raise your max address limit to 132 MB: by the
>> looks of it, that is the maximum size the current kernel code will
>> ever be able to support since it uses two adjacent sections to map the
>> FDT, and sections are 2 MB at most when using long descriptors.
>
>
> I've tested it with a patch to change the max offsets to 0x84000000 and it
> works well.  My hacked in debug shows:
>
> linux:  address 0x87FD2000
> linux:  length  0x42D248

Hmm, this doesn't look right. The zImage should be below 128 MB, since
it infers the base of DRAM by rounding down its own address to a
multiple of 128 MB.
I seem to have missed the part before where the PCD in question also
affects the placement of the zImage and not only the FDT.

> fdt:    address 0x87E6E000
> fdt:    length  0x3FFC
> initrd: address 0x87E73000
> initrd: length  0x15E600
>

So the rules say:
- zImage between 32 MB and 128 MB
- FDT preferably at the next 128 MB boundary
- initrd preferably right above the FDT

I guess your v1 was best after all: even if the FDT and initrd end up
below 128 MB, it is the best we can do with just this PCD

--
Ard.

> From this patch:
>
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index b30de91..d5b930a 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -117,7 +117,7 @@
>    #
>    gArmTokenSpaceGuid.PcdArmMachineType|0|UINT32|0x0000001E
>    # The compressed Linux kernel is expected to be under 128MB from the
> beginning of the System Memory
> -
> gArmTokenSpaceGuid.PcdArmLinuxKernelMaxOffset|0x08000000|UINT32|0x0000001F
> +
> gArmTokenSpaceGuid.PcdArmLinuxKernelMaxOffset|0x08400000|UINT32|0x0000001F^M
>    # Maximum file size for TFTP servers that do not support 'tsize'
> extension
>    gArmTokenSpaceGuid.PcdMaxTftpFileSize|0x01000000|UINT32|0x00000000
>
> @@ -159,7 +159,7 @@
>    # If the fixed FDT address is not available, then it should be loaded
> below the kernel.
>    # The recommendation from the Linux kernel is to have the FDT below 16KB.
>    # (see the kernel doc: Documentation/arm/Booting)
> -  gArmTokenSpaceGuid.PcdArmLinuxFdtMaxOffset|0x4000|UINT32|0x00000023
> +  gArmTokenSpaceGuid.PcdArmLinuxFdtMaxOffset|0x08400000|UINT32|0x00000023^M
>    # The FDT blob must be loaded at a 64bit aligned address.
>    gArmTokenSpaceGuid.PcdArmLinuxFdtAlignment|0x8|UINT32|0x00000026
>
>
> Of course, the comments will have to change too: "under 128MB" => "above
> 128MiB" and there is no recommendation for the device tree to be < 16KB.
>
> Unless I get further comment, I'll submit a v2 patch as above with the
> comments updated.
>
>
>>
>>
>> BTW it seems odd for the LinuxLoader - which is now a separate EFI
>> application - to use fixed PCDs to parametrise its behavior. I think
>> it would be justified to hardcode the recommended behavior as per the
>> Linux/ARM boot protocol right into the LinuxLoader binary.
>>
>> --
>> Ard.
>>
>> >
>> >
>> >>
>> >> --
>> >> Ard.
>> >>
>> >>
>> >> > This email thread explains that the device tree should be placed
>> >> > higher
>> >> > in RAM:
>> >> >
>> >> >
>> >> >
>> >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/187476.html
>> >> >

>> >> > It also explaines that the kernel may use memory in the 16-32KB range
>> >> > and that a zImage will by default be uncompressed to an offset of
>> >> > 32KB.
>> >> >
>> >> > Setting the FDT max offset at 128MiB will allow UEFI to place it
>> >> > higher
>> >> > up in memory, thus avoiding the problems with it being loaded to a
>> >> > random address.
>> >> >
>> >> > With this patch, by using AllocateMaxAdress, where possible the Linux
>> >> > Loader will place the FDT, initrd and kernel at the top of the 128MiB
>> >> > range, which also allows for more efficient zImage uncompression, as
>> >> > per
>> >> > the above thread.
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.0
>> >> > Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org>

>> >> > ---
>> >> >  ArmPkg/ArmPkg.dec | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
>> >> > index da0c8a9..67c8cc9 100644
>> >> > --- a/ArmPkg/ArmPkg.dec
>> >> > +++ b/ArmPkg/ArmPkg.dec
>> >> > @@ -158,7 +158,7 @@
>> >> >    # If the fixed FDT address is not available, then it should be
>> >> > loaded
>> >> > below the kernel.
>> >> >    # The recommendation from the Linux kernel is to have the FDT
>> >> > below
>> >> > 16KB.
>> >> >    # (see the kernel doc: Documentation/arm/Booting)
>> >> > -
>> >> > gArmTokenSpaceGuid.PcdArmLinuxFdtMaxOffset|0x4000|UINT32|0x00000023
>> >> > +
>> >> >
>> >> > gArmTokenSpaceGuid.PcdArmLinuxFdtMaxOffset|0x08000000|UINT32|0x00000023
>> >> >    # The FDT blob must be loaded at a 64bit aligned address.
>> >> >    gArmTokenSpaceGuid.PcdArmLinuxFdtAlignment|0x8|UINT32|0x00000026
>> >> >
>> >> > --
>> >> > 2.1.0
>> >> >
>> >
>> >
>
>