From: Rob Herring <robh@kernel.org> To: Andre Przywara <andre.przywara@arm.com> Cc: Frank Rowand <frowand.list@gmail.com>, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Ross Burton <ross.burton@arm.com>, Peter Maydell <peter.maydell@linaro.org>, Ard Biesheuvel <ardb@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Russell King <linux@armlinux.org.uk> Subject: Re: [PATCH] of/fdt: Ignore disabled memory nodes Date: Tue, 17 May 2022 10:34:44 -0500 [thread overview] Message-ID: <20220517153444.GA1057027-robh@kernel.org> (raw) In-Reply-To: <20220517101410.3493781-1-andre.przywara@arm.com> On Tue, May 17, 2022 at 11:14:10AM +0100, Andre Przywara wrote: > When we boot a machine using a devicetree, the generic DT code goes > through all nodes with a 'device_type = "memory"' property, and collects > all memory banks mentioned there. However it does not check for the > status property, so any nodes which are explicitly "disabled" will still > be added as a memblock. > This ends up badly for QEMU, when booting with secure firmware on > arm/arm64 machines, because QEMU adds a node describing secure-only > memory: > =================== > secram@e000000 { BTW, 'memory' is the correct node name. > secure-status = "okay"; > status = "disabled"; > reg = <0x00 0xe000000 0x00 0x1000000>; > device_type = "memory"; > }; > =================== > > The kernel will eventually use that memory block (which is located below > the main DRAM bank), but accesses to that will be answered with an > SError: > =================== > [ 0.000000] Internal error: synchronous external abort: 96000050 [#1] PREEMPT SMP > [ 0.000000] Modules linked in: > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0-rc6-00014-g10c8acb8b679 #524 > [ 0.000000] Hardware name: linux,dummy-virt (DT) > [ 0.000000] pstate: 200000c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 0.000000] pc : new_slab+0x190/0x340 > [ 0.000000] lr : new_slab+0x184/0x340 > [ 0.000000] sp : ffff80000a4b3d10 > .... > ================== > The actual crash location and call stack will be somewhat random, and > depend on the specific allocation of that physical memory range. > > As the DT spec[1] explicitly mentions standard properties, add a simple > check to skip over disabled memory nodes, so that we only use memory > that is meant for non-secure code to use. > > That fixes booting a QEMU arm64 VM with EL3 enabled ("secure=on"), when > not using UEFI. In this case the QEMU generated DT will be handed on > to the kernel, which will see the secram node. > This issue is reproducible when using TF-A together with U-Boot as > firmware, then booting with the "booti" command. > > When using U-Boot as an UEFI provider, the code there [2] explicitly > filters for disabled nodes when generating the UEFI memory map, so we > are safe. > EDK/2 only reads the first bank of the first DT memory node [3] to learn > about memory, so we got lucky there. > > [1] https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter3-devicenodes.rst#memory-node (after the table) > [2] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/fdtdec.c#L1061-1063 > [3] https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/PrePi/FdtParser.c > > Reported-by: Ross Burton <ross.burton@arm.com> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > drivers/of/fdt.c | 3 +++ > 1 file changed, 3 insertions(+) Applied, thanks!
WARNING: multiple messages have this Message-ID (diff)
From: Rob Herring <robh@kernel.org> To: Andre Przywara <andre.przywara@arm.com> Cc: Frank Rowand <frowand.list@gmail.com>, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Ross Burton <ross.burton@arm.com>, Peter Maydell <peter.maydell@linaro.org>, Ard Biesheuvel <ardb@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Russell King <linux@armlinux.org.uk> Subject: Re: [PATCH] of/fdt: Ignore disabled memory nodes Date: Tue, 17 May 2022 10:34:44 -0500 [thread overview] Message-ID: <20220517153444.GA1057027-robh@kernel.org> (raw) In-Reply-To: <20220517101410.3493781-1-andre.przywara@arm.com> On Tue, May 17, 2022 at 11:14:10AM +0100, Andre Przywara wrote: > When we boot a machine using a devicetree, the generic DT code goes > through all nodes with a 'device_type = "memory"' property, and collects > all memory banks mentioned there. However it does not check for the > status property, so any nodes which are explicitly "disabled" will still > be added as a memblock. > This ends up badly for QEMU, when booting with secure firmware on > arm/arm64 machines, because QEMU adds a node describing secure-only > memory: > =================== > secram@e000000 { BTW, 'memory' is the correct node name. > secure-status = "okay"; > status = "disabled"; > reg = <0x00 0xe000000 0x00 0x1000000>; > device_type = "memory"; > }; > =================== > > The kernel will eventually use that memory block (which is located below > the main DRAM bank), but accesses to that will be answered with an > SError: > =================== > [ 0.000000] Internal error: synchronous external abort: 96000050 [#1] PREEMPT SMP > [ 0.000000] Modules linked in: > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.18.0-rc6-00014-g10c8acb8b679 #524 > [ 0.000000] Hardware name: linux,dummy-virt (DT) > [ 0.000000] pstate: 200000c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 0.000000] pc : new_slab+0x190/0x340 > [ 0.000000] lr : new_slab+0x184/0x340 > [ 0.000000] sp : ffff80000a4b3d10 > .... > ================== > The actual crash location and call stack will be somewhat random, and > depend on the specific allocation of that physical memory range. > > As the DT spec[1] explicitly mentions standard properties, add a simple > check to skip over disabled memory nodes, so that we only use memory > that is meant for non-secure code to use. > > That fixes booting a QEMU arm64 VM with EL3 enabled ("secure=on"), when > not using UEFI. In this case the QEMU generated DT will be handed on > to the kernel, which will see the secram node. > This issue is reproducible when using TF-A together with U-Boot as > firmware, then booting with the "booti" command. > > When using U-Boot as an UEFI provider, the code there [2] explicitly > filters for disabled nodes when generating the UEFI memory map, so we > are safe. > EDK/2 only reads the first bank of the first DT memory node [3] to learn > about memory, so we got lucky there. > > [1] https://github.com/devicetree-org/devicetree-specification/blob/main/source/chapter3-devicenodes.rst#memory-node (after the table) > [2] https://source.denx.de/u-boot/u-boot/-/blob/master/lib/fdtdec.c#L1061-1063 > [3] https://github.com/tianocore/edk2/blob/master/ArmVirtPkg/PrePi/FdtParser.c > > Reported-by: Ross Burton <ross.burton@arm.com> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > drivers/of/fdt.c | 3 +++ > 1 file changed, 3 insertions(+) Applied, thanks! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-05-17 15:35 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-17 10:14 [PATCH] of/fdt: Ignore disabled memory nodes Andre Przywara 2022-05-17 10:14 ` Andre Przywara 2022-05-17 15:34 ` Rob Herring [this message] 2022-05-17 15:34 ` Rob Herring 2022-05-17 16:54 ` Peter Maydell 2022-05-17 16:54 ` Peter Maydell 2022-05-17 17:47 ` Rob Herring 2022-05-17 17:47 ` Rob Herring 2022-05-17 19:19 ` Peter Maydell 2022-05-17 19:19 ` Peter Maydell 2022-05-18 16:54 ` Rob Herring 2022-05-18 16:54 ` Rob Herring 2022-05-18 17:54 ` Peter Maydell 2022-05-18 17:54 ` Peter Maydell 2022-05-18 20:46 ` Ard Biesheuvel 2022-05-18 20:46 ` Ard Biesheuvel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220517153444.GA1057027-robh@kernel.org \ --to=robh@kernel.org \ --cc=andre.przywara@arm.com \ --cc=ardb@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=devicetree@vger.kernel.org \ --cc=frowand.list@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=peter.maydell@linaro.org \ --cc=ross.burton@arm.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.