[2.4] hw/arm/boot: Increase fdt alignment
diff mbox

Message ID 1436802997-170244-1-git-send-email-agraf@suse.de
State New
Headers show

Commit Message

Alexander Graf July 13, 2015, 3:56 p.m. UTC
The Linux kernel on aarch64 creates a page table entry at early bootup
that spans the 2MB range on memory spanning the fdt start address:

  [ ALIGN_DOWN(fdt, 2MB) ... ALIGN_DOWN(fdt, 2MB) + 2MB ]

This means that when our current 4k alignment happens to fall at the end
of the aligned region, Linux tries to access memory that is not mapped.

The easy fix is to instead increase the alignment to 2MB, making Linux's
logic always succeed.

Reported-by: Andreas Schwab <schwab@suse.de>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/arm/boot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Peter Maydell July 13, 2015, 4:13 p.m. UTC | #1
On 13 July 2015 at 16:56, Alexander Graf <agraf@suse.de> wrote:
> The Linux kernel on aarch64 creates a page table entry at early bootup
> that spans the 2MB range on memory spanning the fdt start address:
>
>   [ ALIGN_DOWN(fdt, 2MB) ... ALIGN_DOWN(fdt, 2MB) + 2MB ]
>
> This means that when our current 4k alignment happens to fall at the end
> of the aligned region, Linux tries to access memory that is not mapped.
>
> The easy fix is to instead increase the alignment to 2MB, making Linux's
> logic always succeed.

This is a kernel bug and should be fixed there. The booting
protocol document is pretty clear:
https://www.kernel.org/doc/Documentation/arm64/booting.txt

"The device tree blob (dtb) must be placed on an 8-byte boundary"

-- PMM
Alexander Graf July 13, 2015, 4:20 p.m. UTC | #2
On 07/13/15 18:13, Peter Maydell wrote:
> On 13 July 2015 at 16:56, Alexander Graf <agraf@suse.de> wrote:
>> The Linux kernel on aarch64 creates a page table entry at early bootup
>> that spans the 2MB range on memory spanning the fdt start address:
>>
>>    [ ALIGN_DOWN(fdt, 2MB) ... ALIGN_DOWN(fdt, 2MB) + 2MB ]
>>
>> This means that when our current 4k alignment happens to fall at the end
>> of the aligned region, Linux tries to access memory that is not mapped.
>>
>> The easy fix is to instead increase the alignment to 2MB, making Linux's
>> logic always succeed.
> This is a kernel bug and should be fixed there. The booting
> protocol document is pretty clear:
> https://www.kernel.org/doc/Documentation/arm64/booting.txt
>
> "The device tree blob (dtb) must be placed on an 8-byte boundary"

While I tend to agree, we can't fix old / released kernels. So maybe we 
should just fix it in both?


Alex
Peter Maydell July 13, 2015, 4:27 p.m. UTC | #3
On 13 July 2015 at 17:20, Alexander Graf <agraf@suse.de> wrote:
> On 07/13/15 18:13, Peter Maydell wrote:
>>
>> On 13 July 2015 at 16:56, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> The Linux kernel on aarch64 creates a page table entry at early bootup
>>> that spans the 2MB range on memory spanning the fdt start address:
>>>
>>>    [ ALIGN_DOWN(fdt, 2MB) ... ALIGN_DOWN(fdt, 2MB) + 2MB ]
>>>
>>> This means that when our current 4k alignment happens to fall at the end
>>> of the aligned region, Linux tries to access memory that is not mapped.
>>>
>>> The easy fix is to instead increase the alignment to 2MB, making Linux's
>>> logic always succeed.
>>
>> This is a kernel bug and should be fixed there. The booting
>> protocol document is pretty clear:
>> https://www.kernel.org/doc/Documentation/arm64/booting.txt
>>
>> "The device tree blob (dtb) must be placed on an 8-byte boundary"
>
>
> While I tend to agree, we can't fix old / released kernels. So maybe we
> should just fix it in both?

Maybe. The trouble with just bumping up the alignment though is that
not all boards have so much RAM that they can trivially waste another
meg or two on alignment padding without noticing it. AArch64 boards
are probably OK but some of the old 32 bit boards are not anywhere
near as well supplied with RAM. Picking dtb alignment based on 32/64
bits might be safer.

-- PMM
Alexander Graf July 13, 2015, 4:32 p.m. UTC | #4
On 07/13/15 18:27, Peter Maydell wrote:
> On 13 July 2015 at 17:20, Alexander Graf <agraf@suse.de> wrote:
>> On 07/13/15 18:13, Peter Maydell wrote:
>>> On 13 July 2015 at 16:56, Alexander Graf <agraf@suse.de> wrote:
>>>> The Linux kernel on aarch64 creates a page table entry at early bootup
>>>> that spans the 2MB range on memory spanning the fdt start address:
>>>>
>>>>     [ ALIGN_DOWN(fdt, 2MB) ... ALIGN_DOWN(fdt, 2MB) + 2MB ]
>>>>
>>>> This means that when our current 4k alignment happens to fall at the end
>>>> of the aligned region, Linux tries to access memory that is not mapped.
>>>>
>>>> The easy fix is to instead increase the alignment to 2MB, making Linux's
>>>> logic always succeed.
>>> This is a kernel bug and should be fixed there. The booting
>>> protocol document is pretty clear:
>>> https://www.kernel.org/doc/Documentation/arm64/booting.txt
>>>
>>> "The device tree blob (dtb) must be placed on an 8-byte boundary"
>>
>> While I tend to agree, we can't fix old / released kernels. So maybe we
>> should just fix it in both?
> Maybe. The trouble with just bumping up the alignment though is that
> not all boards have so much RAM that they can trivially waste another
> meg or two on alignment padding without noticing it. AArch64 boards
> are probably OK but some of the old 32 bit boards are not anywhere
> near as well supplied with RAM. Picking dtb alignment based on 32/64
> bits might be safer.

How about we just align it to the next log2 of the dtb size? That should 
solve all problems and be good enough for low mem boards too.


Alex
Alexander Graf July 13, 2015, 4:39 p.m. UTC | #5
On 07/13/15 18:27, Peter Maydell wrote:
> On 13 July 2015 at 17:20, Alexander Graf <agraf@suse.de> wrote:
>> On 07/13/15 18:13, Peter Maydell wrote:
>>> On 13 July 2015 at 16:56, Alexander Graf <agraf@suse.de> wrote:
>>>> The Linux kernel on aarch64 creates a page table entry at early bootup
>>>> that spans the 2MB range on memory spanning the fdt start address:
>>>>
>>>>     [ ALIGN_DOWN(fdt, 2MB) ... ALIGN_DOWN(fdt, 2MB) + 2MB ]
>>>>
>>>> This means that when our current 4k alignment happens to fall at the end
>>>> of the aligned region, Linux tries to access memory that is not mapped.
>>>>
>>>> The easy fix is to instead increase the alignment to 2MB, making Linux's
>>>> logic always succeed.
>>> This is a kernel bug and should be fixed there. The booting
>>> protocol document is pretty clear:
>>> https://www.kernel.org/doc/Documentation/arm64/booting.txt
>>>
>>> "The device tree blob (dtb) must be placed on an 8-byte boundary"
>>
>> While I tend to agree, we can't fix old / released kernels. So maybe we
>> should just fix it in both?
> Maybe. The trouble with just bumping up the alignment though is that
> not all boards have so much RAM that they can trivially waste another
> meg or two on alignment padding without noticing it. AArch64 boards
> are probably OK but some of the old 32 bit boards are not anywhere
> near as well supplied with RAM. Picking dtb alignment based on 32/64
> bits might be safer.

Ugh, we don't know the size yet at this point. And calling load_fdt 
multiple times feels like a can of worms I don't want to open. So yeah, 
I'll just make it depend on the RAM size - if there are >= 128MB RAM in 
our VM we align on 2MB.


Alex
Peter Maydell July 13, 2015, 4:41 p.m. UTC | #6
On 13 July 2015 at 17:39, Alexander Graf <agraf@suse.de> wrote:
> Ugh, we don't know the size yet at this point. And calling load_fdt multiple
> times feels like a can of worms I don't want to open. So yeah, I'll just
> make it depend on the RAM size - if there are >= 128MB RAM in our VM we
> align on 2MB.

This is an AArch64-kernel-specific problem, why are we changing the
alignment for 32-bit kernels?

-- PMM

Patch
diff mbox

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index f48ed2d..7594d7a 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -735,12 +735,12 @@  static void arm_load_kernel_notify(Notifier *notifier, void *data)
          * we point to the kernel args.
          */
         if (have_dtb(info)) {
-            /* Place the DTB after the initrd in memory. Note that some
-             * kernels will trash anything in the 4K page the initrd
-             * ends in, so make sure the DTB isn't caught up in that.
+            /* Place the DTB after the initrd in memory. Note that the kernel
+             * maps [ ALIGN_DOWN(fdt, 2MB) ... ALGIN_DOWN(fdt, 2MB) + 2MB ]
+             * and thus needs the fdt be preferably in its own 2MB window.
              */
             hwaddr dtb_start = QEMU_ALIGN_UP(info->initrd_start + initrd_size,
-                                             4096);
+                                             2 * 1024 * 1024);
             if (load_dtb(dtb_start, info, 0) < 0) {
                 exit(1);
             }