lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <d2bdfbfd-3721-407f-991e-566d48392add@amd.com>
Date: Tue, 14 Jan 2025 16:03:42 +0100
From: Michal Simek <michal.simek@....com>
To: Masahiro Yamada <masahiroy@...nel.org>
Cc: linux-kernel@...r.kernel.org, Conor Dooley <conor+dt@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Rob Herring <robh@...nel.org>,
 devicetree@...r.kernel.org
Subject: Re: [PATCH] microblaze: migrate to the generic rule for built-in DTB



On 1/14/25 03:41, Masahiro Yamada wrote:
> On Wed, Jan 8, 2025 at 1:37 AM Michal Simek <michal.simek@....com> wrote:
>>
>> Hi Masahiro,
>>
>> On 12/22/24 10:46, Masahiro Yamada wrote:
>>> Commit 654102df2ac2 ("kbuild: add generic support for built-in boot
>>> DTBs") introduced generic support for built-in DTBs.
>>>
>>> Select GENERIC_BUILTIN_DTB to use the generic rule.
>>>
>>> MicroBlaze is the only architecture that places the built-in DTB to its
>>> own section, __fdt_blob, rather than the standard location defined by
>>> the KERNEL_DTB() macro.
>>>
>>> For backward compatibility, arch/microblaze/boot/dts/system.dtb is still
>>> placed in the __fdt_blob section, and possibly overwritten by an externel
>>> DTB passed via the r7 register.
>>>
>>> Signed-off-by: Masahiro Yamada <masahiroy@...nel.org>
>>> ---
>>>
>>>    arch/microblaze/Kbuild                | 1 -
>>>    arch/microblaze/Kconfig               | 5 +++++
>>>    arch/microblaze/boot/dts/Makefile     | 5 -----
>>>    arch/microblaze/boot/dts/linked_dtb.S | 2 --
>>>    arch/microblaze/kernel/vmlinux.lds.S  | 2 +-
>>>    5 files changed, 6 insertions(+), 9 deletions(-)
>>>    delete mode 100644 arch/microblaze/boot/dts/linked_dtb.S
>>>
>>> diff --git a/arch/microblaze/Kbuild b/arch/microblaze/Kbuild
>>> index 077a0b8e9615..70510389eb92 100644
>>> --- a/arch/microblaze/Kbuild
>>> +++ b/arch/microblaze/Kbuild
>>> @@ -2,7 +2,6 @@
>>>    obj-y                       += kernel/
>>>    obj-y                       += mm/
>>>    obj-$(CONFIG_PCI)   += pci/
>>> -obj-y                        += boot/dts/
>>>
>>>    # for cleaning
>>>    subdir- += boot
>>> diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
>>> index f18ec02ddeb2..4ed8ca89f0c9 100644
>>> --- a/arch/microblaze/Kconfig
>>> +++ b/arch/microblaze/Kconfig
>>> @@ -10,6 +10,7 @@ config MICROBLAZE
>>>        select ARCH_MIGHT_HAVE_PC_PARPORT
>>>        select ARCH_WANT_IPC_PARSE_VERSION
>>>        select BUILDTIME_TABLE_SORT
>>> +     select GENERIC_BUILTIN_DTB
>>>        select TIMER_OF
>>>        select CLONE_BACKWARDS3
>>>        select COMMON_CLK
>>> @@ -47,6 +48,10 @@ config MICROBLAZE
>>>        select TRACE_IRQFLAGS_SUPPORT
>>>        select GENERIC_IRQ_MULTI_HANDLER
>>>
>>> +config BUILTIN_DTB_NAME
>>> +     string
>>> +     default "system"
>>> +
>>>    # Endianness selection
>>>    choice
>>>        prompt "Endianness selection"
>>> diff --git a/arch/microblaze/boot/dts/Makefile b/arch/microblaze/boot/dts/Makefile
>>> index b84e2cbb20ee..87c1d25ff096 100644
>>> --- a/arch/microblaze/boot/dts/Makefile
>>> +++ b/arch/microblaze/boot/dts/Makefile
>>> @@ -4,11 +4,6 @@
>>>    dtb-y := system.dtb
>>>
>>>    ifneq ($(DTB),)
>>> -obj-y += linked_dtb.o
>>> -
>>> -# Ensure system.dtb exists
>>> -$(obj)/linked_dtb.o: $(obj)/system.dtb
>>> -
>>>    # Generate system.dtb from $(DTB).dtb
>>>    ifneq ($(DTB),system)
>>>    $(obj)/system.dtb: $(obj)/$(DTB).dtb
>>> diff --git a/arch/microblaze/boot/dts/linked_dtb.S b/arch/microblaze/boot/dts/linked_dtb.S
>>> deleted file mode 100644
>>> index 23345af3721f..000000000000
>>> --- a/arch/microblaze/boot/dts/linked_dtb.S
>>> +++ /dev/null
>>> @@ -1,2 +0,0 @@
>>> -.section __fdt_blob,"a"
>>> -.incbin "arch/microblaze/boot/dts/system.dtb"
>>> diff --git a/arch/microblaze/kernel/vmlinux.lds.S b/arch/microblaze/kernel/vmlinux.lds.S
>>> index ae50d3d04a7d..e86f9ca8e979 100644
>>> --- a/arch/microblaze/kernel/vmlinux.lds.S
>>> +++ b/arch/microblaze/kernel/vmlinux.lds.S
>>> @@ -47,7 +47,7 @@ SECTIONS {
>>>        . = ALIGN (8) ;
>>>        __fdt_blob : AT(ADDR(__fdt_blob) - LOAD_OFFSET) {
>>>                _fdt_start = . ;                /* place for fdt blob */
>>> -             *(__fdt_blob) ;                 /* Any link-placed DTB */
>>> +             *(.dtb.init.rodata) ;           /* Any link-placed DTB */
>>>                . = _fdt_start + 0x10000;       /* Pad up to 64kbyte */
>>>                _fdt_end = . ;
>>>        }
>>
>> This patch is better then previous one but still it is changing behavior of
>> build. When this patch is applied linux.bin contains dtb which is not the same
>> behavior as before (which was empty).
>> DTB should be filled when simpleImage.<dt> is built.
> 
> Why is it a problem?

Firstly that change is not described in commit message.
Second system.dtb is really old dtb more for demonstration purpose and nothing 
else and likely it is not working on any existing board.

> 
> Microblaze always keeps 64kbyte space in case
> an external DTB is passed from a boot-loader.
> 
> Even if the default system.dtb is placed there,
> nothing is wasted.

It is not about wasting of space. It is about information which none is going to 
use. That's why linux.bin has no dtb inside.


> One more thing, Microblaze is only the architecture
> that determines the built-in DTB by the command line.

yes and it has been done like this for many years without any complain about it. 
simpleImage has been taken from PPC in past.

> 
> I do not think it is a sensible way.
> 
>      $ make linux.bin simpleImage.foo
>        --> linux.bin also contains the builtin device tree
> 
>      $ make simpleImage.foo simpleImage.bar
>        --> does not work

yes because foo.dtb and bar.dtb are not in the Linux tree.


> All other architectures determinantes the built-in DTB
> by CONFIG option.

Microblaze is soft core and make no sense to start to push other DTSes to the 
Linux kernel because pretty much every design is unique. That's why only 
system.dts is there for demonstration purpose.

If users have multiple boards they can just add dts to the source code and build 
kernel. And linux.bin should be used with U-Boot ITBs where dtb is passed via 
register.

Thanks,
Michal






Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ