* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-04 10:13 ` Grant Likely
@ 2018-09-04 17:19 ` Scott Branden
2018-09-05 9:40 ` Ard Biesheuvel
2018-09-04 17:28 ` Olof Johansson
` (2 subsequent siblings)
3 siblings, 1 reply; 25+ messages in thread
From: Scott Branden @ 2018-09-04 17:19 UTC (permalink / raw)
To: Grant Likely, Ard Biesheuvel
Cc: Olof Johansson, Catalin Marinas, Will Deacon, Arnd Bergmann,
bcm-kernel-feedback-list, Linux ARM, Linux Kernel Mailing List,
Leif Lindholm, Alexander Graf
On 18-09-04 03:13 AM, Grant Likely wrote:
> Hey folks. More comments below, but the short answer is I really don't
> see what the problem is. Distros cannot easily support platforms that
> require a dtb= parameter, and so they probably won't. They may or may
> not disable 'dtb=', depending on whether they see it as valuable for
> debug.
>
> Vertically integrated platforms are a different beast. We may strongly
> recommend firmware provides the dtb for all the mentioned good
> reasons, but they still get to decide their deployment methodology,
> and it is not burdensome for the kernel to keep the dtb= feature that
> they are using.
>
> On Tue, Sep 4, 2018 at 7:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 2 September 2018 at 04:54, Olof Johansson <olof@lixom.net> wrote:
>>> On Thu, Aug 30, 2018 at 9:23 AM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 30 August 2018 at 17:06, Olof Johansson <olof@lixom.net> wrote:
>>>>> On Wed, Aug 29, 2018 at 10:54 PM, Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>> On 29 August 2018 at 20:59, Scott Branden <scott.branden@broadcom.com> wrote:
>>>>>>> Hi Olof,
>>>>>>>
>>>>>>>
>>>>>>> On 18-08-29 11:44 AM, Olof Johansson wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Wed, Aug 29, 2018 at 10:21 AM, Scott Branden
>>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>> Enable EFI_ARMSTUB_DTB_LOADER to add support for the dtb= command line
>>>>>>>>> parameter to function with efi loader.
>>>>>>>>>
>>>>>>>>> Required to boot on existing bootloaders that do not support devicetree
>>>>>>>>> provided by the platform or by the bootloader.
>>>>>>>>>
>>>>>>>>> Fixes: 3d7ee348aa41 ("efi/libstub/arm: Add opt-in Kconfig option for the
>>>>>>>>> DTB loader")
>>>>>>>>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>>>>>>>> Why did Ard create an option for this if it's just going be turned on
>>>>>>>> in default configs? Doesn't make sense to me.
>>>>>>>>
>>>>>>>> It would help to know what firmware still is crippled and how common
>>>>>>>> it is, since it's been a few years that this has been a requirement by
>>>>>>>> now.
>>>>>>> Broadcom NS2 and Stingray in current development and production need this
>>>>>>> option in the kernel enabled in order to boot.
>>>>>> And these production systems run mainline kernels in a defconfig configuration?
>>>>>>
>>>>>> The simply reality is that the DTB loader has been deprecated for a
>>>>>> good reason: it was only ever intended as a development hack anyway,
>>>>>> and if we need to treat the EFI stub provided DTB as a first class
>>>>>> citizen, there are things we need to fix to make things works as
>>>>>> expected. For instance, GRUB will put a property in the /chosen node
>>>>>> for the initramfs which will get dropped if you boot with dtb=.
>>>>>>
>>>>>> Don't be surprised if some future enhancements of the EFI stub code
>>>>>> depend on !EFI_ARMSTUB_DTB_LOADER.
> That's an odd statement to make. The DTB loader code is well contained
> and with defined semantics... True, the semantics are "I DON'T BELIEVE
> FIRMWARE", but it is still well defined. What scenario are you
> envisioning where EFI_ARMSTUB_DTB_LOADER would be explicitly excluded?
>
> Conversely, the dtb= argument is an invaluable debug tool during
> development. As Olof has already said, there are a lot of embedded
> deployments where there is no desire for grub or any other
> intermediary loader.
>
>>>>>> On UEFI systems, DTBs [or ACPI
>>>>>> tables] are used by the firmware to describe itself and the underlying
>>>>>> platform to the OS, and the practice of booting with DTB file images
>>>>>> (taken from the kernel build as well) conflicts with that view. Note
>>>>>> that GRUB still permits you to load DTBs from files (and supports more
>>>>>> sources than just the file system the kernel Image was loaded from).
>>>>> Ard,
>>>>>
>>>>> Maybe a WARN() splat would be more useful as a phasing-out method than
>>>>> removing functionality for them that needs to be reinstated through
>>>>> changing the config?
>>>>>
>>>> We don't have any of that in the stub, and inventing new ways to pass
>>>> such information between the stub and the kernel proper seems like a
>>>> cart-before-horse kind of thing to me. The EFI stub diagnostic
>>>> messages you get on the serial console are not recorded in the kernel
>>>> log buffer, so they only appear if you actually look at the serial
>>>> output.
> As an aside, they probably should be recorded. That is probably a
> question for the UEFI USWG. Grub and the ARMSTUB could probably bodge
> something together, but that would be non-standard.
>
>>> Ah yeah. I suppose you could do it in the kernel later if you detect
>>> you've booted through EFI with dtb= on the command line though.
>>>
>>>>> Once the stub and the boot method is there, it's hard to undo as we
>>>>> can see here. Being loud and warn might be more useful, and set a
>>>>> timeline for hard removal (12 months?).
>>>>>
>>>> The dtb= handling is still there, it is just not enabled by default.
>>>> We can keep it around if people are still using it. But as I pointed
>>>> out, we may decide to make new functionality available only if it is
>>>> disabled, and at that point, we'll have to choose between one or the
>>>> other in defconfig, which is annoying.
>>>>
>>>>> Scott; an alternative for you is to do a boot wrapper that bundles a
>>>>> DT and kernel, and boot that instead of the kernel image (outside of
>>>>> the kernel tree). Some 32-bit platforms from Marvell use that. That
>>>>> way the kernel will just see it as a normally passed in DT.
>>>>>
>>>> Or use GRUB. It comes wired up in all the distros, and let's you load
>>>> a DT binary from anywhere you can imagine, as opposed to the EFI stub
>>>> which can only load it if it happens to reside in the same file system
>>>> (or even directory - I can't remember) as the kernel image. Note that
>>>> the same reservations apply to doing that - the firmware is no longer
>>>> able to describe itself to the OS via the DT, which is really the only
>>>> conduit it has available on an arm64 system..
>>> So, I've looked at the history here a bit, and dtb= support was
>>> introduced in 2014. Nowhere does it say that it isn't a recommended
>>> way of booting.
>>>
>>> There are some firmware stacks today that modify and provide a
>>> runtime-updated devicetree to the kernel, but there are also a bunch
>>> who don't. Most "real" products will want a firmware that knows how to
>>> pass in things such as firmware environment variables, or MAC
>>> addresses, etc, to the kernel, but not all of them need it.
>>>
>>> In particular, in a world where you want EFI to be used on embedded
>>> platforms, requiring another bootloader step such as GRUB to be able
>>> to reasonably boot said platforms seems like a significant and
>>> unfortunate new limitation. Documentation/efi-stub.txt has absolutely
>>> no indication that it is a second-class option that isn't expected to
>>> be available everywhere. It doesn't really matter what _your_
>>> intention was around it, if those who use it never found out and now
>>> rely on it.
>>>
>>> Unfortunately the way forward here is to revert 3d7ee348aa4127a.
>>>
>> I agree with your analysis but not with your conclusion.
>>
>> Whether or not the option is def_bool y and/or enabled in defconfig is
>> a matter of policy. ACPI-only distros such as RHEL are definitely
>> going to disable this option. But in general, supporting DTBs loaded
>> from files is a huge pain for the distros, so I expect most of them to
>> disable it as well.
> I support leaving 3d7ee348 in, and making it def_bool y
Rather than introduce EFI_ARMSTUB_DTB_LOADER, why not have
the efistub use CONFIG_OF to determine whether it supports dtb= or not?
That way ACPI-only distros disable devicetree support entirely.
>
> g.
>> As for EFI on embedded systems: this will be mostly on U-boot's
>> bootefi implementation, which definitely does the right thing when it
>> comes to passing the DTB via a UEFI configuration table (regardless of
>> whether it makes any modifications to it)
>>
>> In any case, I won't object to a patch that reenables the EFI stub DTB
>> loader in defconfig. Whether or not it should be def_bool y is
>> something we can discuss as well. I have added Leif and Alex to cc,
>> perhaps they have anything to add.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-04 17:19 ` Scott Branden
@ 2018-09-05 9:40 ` Ard Biesheuvel
2018-09-05 17:27 ` Scott Branden
0 siblings, 1 reply; 25+ messages in thread
From: Ard Biesheuvel @ 2018-09-05 9:40 UTC (permalink / raw)
To: Scott Branden
Cc: Grant Likely, Olof Johansson, Catalin Marinas, Will Deacon,
Arnd Bergmann, BCM Kernel Feedback, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On 4 September 2018 at 19:19, Scott Branden <scott.branden@broadcom.com> wrote:
>
> Rather than introduce EFI_ARMSTUB_DTB_LOADER, why not have
> the efistub use CONFIG_OF to determine whether it supports dtb= or not?
>
> That way ACPI-only distros disable devicetree support entirely.
>
Unfortunately, CONFIG_OF cannot be disabled on arm64 even on ACPI-only builds.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-05 9:40 ` Ard Biesheuvel
@ 2018-09-05 17:27 ` Scott Branden
2018-09-05 18:00 ` Grant Likely
0 siblings, 1 reply; 25+ messages in thread
From: Scott Branden @ 2018-09-05 17:27 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Grant Likely, Olof Johansson, Catalin Marinas, Will Deacon,
Arnd Bergmann, BCM Kernel Feedback, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On 18-09-05 02:40 AM, Ard Biesheuvel wrote:
> On 4 September 2018 at 19:19, Scott Branden <scott.branden@broadcom.com> wrote:
>> Rather than introduce EFI_ARMSTUB_DTB_LOADER, why not have
>> the efistub use CONFIG_OF to determine whether it supports dtb= or not?
>>
>> That way ACPI-only distros disable devicetree support entirely.
>>
> Unfortunately, CONFIG_OF cannot be disabled on arm64 even on ACPI-only builds.
OF shouldn't be automatically selected in the arm64/Kconfig. It should
have a config
parmaeter like other archs as mips and arm. I can submit a patch so it
functions the
same way as other archs so it is not always selected. It will be good
to add a USE_OF
config options like the other archs (or simply remove the select from
the Kconfig and choose OF directly in the defconfig.
This will have the added benefit of doing away with OF support when its
not needed on an ARM64 platform. ACPI is already not automatically
selected for all ARM64 platforms, nor should devicetree.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-05 17:27 ` Scott Branden
@ 2018-09-05 18:00 ` Grant Likely
2018-09-05 18:04 ` Scott Branden
0 siblings, 1 reply; 25+ messages in thread
From: Grant Likely @ 2018-09-05 18:00 UTC (permalink / raw)
To: scott.branden
Cc: Ard Biesheuvel, Olof Johansson, Catalin Marinas, Will Deacon,
Arnd Bergmann, bcm-kernel-feedback-list, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On Wed, Sep 5, 2018 at 6:27 PM Scott Branden <scott.branden@broadcom.com> wrote:
>
>
>
> On 18-09-05 02:40 AM, Ard Biesheuvel wrote:
> > On 4 September 2018 at 19:19, Scott Branden <scott.branden@broadcom.com> wrote:
> >> Rather than introduce EFI_ARMSTUB_DTB_LOADER, why not have
> >> the efistub use CONFIG_OF to determine whether it supports dtb= or not?
> >>
> >> That way ACPI-only distros disable devicetree support entirely.
> >>
> > Unfortunately, CONFIG_OF cannot be disabled on arm64 even on ACPI-only builds.
> OF shouldn't be automatically selected in the arm64/Kconfig. It should
> have a config
> parmaeter like other archs as mips and arm. I can submit a patch so it
> functions the
> same way as other archs so it is not always selected. It will be good
> to add a USE_OF
> config options like the other archs (or simply remove the select from
> the Kconfig and choose OF directly in the defconfig.
> This will have the added benefit of doing away with OF support when its
> not needed on an ARM64 platform. ACPI is already not automatically
> selected for all ARM64 platforms, nor should devicetree.
We don't do that on Arm because a devicetree is always required at
boot time. Even on ACPI systems a tiny DTB is used containing just a
/chosen node for passing the kernel command line and the initrd
location.
g.
g.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-05 18:00 ` Grant Likely
@ 2018-09-05 18:04 ` Scott Branden
2018-09-09 11:07 ` Catalin Marinas
0 siblings, 1 reply; 25+ messages in thread
From: Scott Branden @ 2018-09-05 18:04 UTC (permalink / raw)
To: Grant Likely
Cc: Ard Biesheuvel, Olof Johansson, Catalin Marinas, Will Deacon,
Arnd Bergmann, bcm-kernel-feedback-list, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On 18-09-05 11:00 AM, Grant Likely wrote:
> On Wed, Sep 5, 2018 at 6:27 PM Scott Branden <scott.branden@broadcom.com> wrote:
>>
>>
>> On 18-09-05 02:40 AM, Ard Biesheuvel wrote:
>>> On 4 September 2018 at 19:19, Scott Branden <scott.branden@broadcom.com> wrote:
>>>> Rather than introduce EFI_ARMSTUB_DTB_LOADER, why not have
>>>> the efistub use CONFIG_OF to determine whether it supports dtb= or not?
>>>>
>>>> That way ACPI-only distros disable devicetree support entirely.
>>>>
>>> Unfortunately, CONFIG_OF cannot be disabled on arm64 even on ACPI-only builds.
>> OF shouldn't be automatically selected in the arm64/Kconfig. It should
>> have a config
>> parmaeter like other archs as mips and arm. I can submit a patch so it
>> functions the
>> same way as other archs so it is not always selected. It will be good
>> to add a USE_OF
>> config options like the other archs (or simply remove the select from
>> the Kconfig and choose OF directly in the defconfig.
>> This will have the added benefit of doing away with OF support when its
>> not needed on an ARM64 platform. ACPI is already not automatically
>> selected for all ARM64 platforms, nor should devicetree.
> We don't do that on Arm because a devicetree is always required at
> boot time. Even on ACPI systems a tiny DTB is used containing just a
> /chosen node for passing the kernel command line and the initrd
> location.
Seems bizarre DTB is not needed for x86 to boot from UEFI with ACPI
support? I'll look into it further at some point in order to remove
such anomaly. There should be no need for such devicetree reliance.
>
> g.
>
> g.
Thanks,
Scott
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-05 18:04 ` Scott Branden
@ 2018-09-09 11:07 ` Catalin Marinas
2018-09-09 15:55 ` Ard Biesheuvel
0 siblings, 1 reply; 25+ messages in thread
From: Catalin Marinas @ 2018-09-09 11:07 UTC (permalink / raw)
To: Scott Branden
Cc: Grant Likely, Arnd Bergmann, Ard Biesheuvel, Will Deacon,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf,
bcm-kernel-feedback-list, Olof Johansson, Linux ARM
On Wed, Sep 05, 2018 at 11:04:36AM -0700, Scott Branden wrote:
> On 18-09-05 11:00 AM, Grant Likely wrote:
> > On Wed, Sep 5, 2018 at 6:27 PM Scott Branden <scott.branden@broadcom.com> wrote:
> > > On 18-09-05 02:40 AM, Ard Biesheuvel wrote:
> > > > On 4 September 2018 at 19:19, Scott Branden <scott.branden@broadcom.com> wrote:
> > > > > Rather than introduce EFI_ARMSTUB_DTB_LOADER, why not have
> > > > > the efistub use CONFIG_OF to determine whether it supports dtb= or not?
> > > > >
> > > > > That way ACPI-only distros disable devicetree support entirely.
> > > > >
> > > > Unfortunately, CONFIG_OF cannot be disabled on arm64 even on ACPI-only builds.
> > > OF shouldn't be automatically selected in the arm64/Kconfig. It should
> > > have a config parmaeter like other archs as mips and arm. I can
> > > submit a patch so it functions the same way as other archs so it
> > > is not always selected. It will be good to add a USE_OF config
> > > options like the other archs (or simply remove the select from the
> > > Kconfig and choose OF directly in the defconfig. This will have
> > > the added benefit of doing away with OF support when its not
> > > needed on an ARM64 platform. ACPI is already not automatically
> > > selected for all ARM64 platforms, nor should devicetree.
> > We don't do that on Arm because a devicetree is always required at
> > boot time. Even on ACPI systems a tiny DTB is used containing just a
> > /chosen node for passing the kernel command line and the initrd
> > location.
>
> Seems bizarre DTB is not needed for x86 to boot from UEFI with ACPI
> support? I'll look into it further at some point in order to remove such
> anomaly. There should be no need for such devicetree reliance.
I'd say don't waste time on this, the patch would not get merged ;). As
Grant said, we use a tiny dtb to pass the command line, initrd to the
kernel. You'd have to invent an alternative (setup_header, ATAGs) and I
really don't see the point of increased complexity just because of some
philosophical arguments against OF.
--
Catalin
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-09 11:07 ` Catalin Marinas
@ 2018-09-09 15:55 ` Ard Biesheuvel
0 siblings, 0 replies; 25+ messages in thread
From: Ard Biesheuvel @ 2018-09-09 15:55 UTC (permalink / raw)
To: Catalin Marinas
Cc: Scott Branden, Grant Likely, Arnd Bergmann, Will Deacon,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf,
BCM Kernel Feedback, Olof Johansson, Linux ARM
On 9 September 2018 at 13:07, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Sep 05, 2018 at 11:04:36AM -0700, Scott Branden wrote:
>> On 18-09-05 11:00 AM, Grant Likely wrote:
>> > On Wed, Sep 5, 2018 at 6:27 PM Scott Branden <scott.branden@broadcom.com> wrote:
>> > > On 18-09-05 02:40 AM, Ard Biesheuvel wrote:
>> > > > On 4 September 2018 at 19:19, Scott Branden <scott.branden@broadcom.com> wrote:
>> > > > > Rather than introduce EFI_ARMSTUB_DTB_LOADER, why not have
>> > > > > the efistub use CONFIG_OF to determine whether it supports dtb= or not?
>> > > > >
>> > > > > That way ACPI-only distros disable devicetree support entirely.
>> > > > >
>> > > > Unfortunately, CONFIG_OF cannot be disabled on arm64 even on ACPI-only builds.
>> > > OF shouldn't be automatically selected in the arm64/Kconfig. It should
>> > > have a config parmaeter like other archs as mips and arm. I can
>> > > submit a patch so it functions the same way as other archs so it
>> > > is not always selected. It will be good to add a USE_OF config
>> > > options like the other archs (or simply remove the select from the
>> > > Kconfig and choose OF directly in the defconfig. This will have
>> > > the added benefit of doing away with OF support when its not
>> > > needed on an ARM64 platform. ACPI is already not automatically
>> > > selected for all ARM64 platforms, nor should devicetree.
>> > We don't do that on Arm because a devicetree is always required at
>> > boot time. Even on ACPI systems a tiny DTB is used containing just a
>> > /chosen node for passing the kernel command line and the initrd
>> > location.
>>
>> Seems bizarre DTB is not needed for x86 to boot from UEFI with ACPI
>> support? I'll look into it further at some point in order to remove such
>> anomaly. There should be no need for such devicetree reliance.
>
> I'd say don't waste time on this, the patch would not get merged ;). As
> Grant said, we use a tiny dtb to pass the command line, initrd to the
> kernel. You'd have to invent an alternative (setup_header, ATAGs) and I
> really don't see the point of increased complexity just because of some
> philosophical arguments against OF.
>
Not just that: we also need of_match_table support for ACPI's PRP0001
device (which is a horrible hack in itself, but currently supported in
Linux/arm64 *and* Linux/x86 nonetheless)
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-04 10:13 ` Grant Likely
2018-09-04 17:19 ` Scott Branden
@ 2018-09-04 17:28 ` Olof Johansson
2018-09-05 9:37 ` Ard Biesheuvel
2018-09-10 17:53 ` Scott Branden
3 siblings, 0 replies; 25+ messages in thread
From: Olof Johansson @ 2018-09-04 17:28 UTC (permalink / raw)
To: Grant Likely
Cc: Ard Biesheuvel, Scott Branden, Catalin Marinas, Will Deacon,
Arnd Bergmann, Broadcom Kernel Feedback List, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On Tue, Sep 4, 2018 at 3:13 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> Hey folks. More comments below, but the short answer is I really don't
> see what the problem is. Distros cannot easily support platforms that
> require a dtb= parameter, and so they probably won't. They may or may
> not disable 'dtb=', depending on whether they see it as valuable for
> debug.
Sure, I'm all for enterprise distros not wanting to support this, but
there are boatloads of kernel parameters that they're unlikely to
already be supporting, including "initrd=" and others. "dtb="
shouldn't be any different.
Needing to disable it with a config option sounds like an odd approach to this.
> Vertically integrated platforms are a different beast. We may strongly
> recommend firmware provides the dtb for all the mentioned good
> reasons, but they still get to decide their deployment methodology,
> and it is not burdensome for the kernel to keep the dtb= feature that
> they are using.
+1
> On Tue, Sep 4, 2018 at 7:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On 2 September 2018 at 04:54, Olof Johansson <olof@lixom.net> wrote:
>> > On Thu, Aug 30, 2018 at 9:23 AM, Ard Biesheuvel
>> > <ard.biesheuvel@linaro.org> wrote:
>> >> On 30 August 2018 at 17:06, Olof Johansson <olof@lixom.net> wrote:
>> >>> On Wed, Aug 29, 2018 at 10:54 PM, Ard Biesheuvel
>> >>> <ard.biesheuvel@linaro.org> wrote:
>> >>>> On 29 August 2018 at 20:59, Scott Branden <scott.branden@broadcom.com> wrote:
>> >>>>> Hi Olof,
>> >>>>>
>> >>>>>
>> >>>>> On 18-08-29 11:44 AM, Olof Johansson wrote:
>> >>>>>>
>> >>>>>> Hi,
>> >>>>>>
>> >>>>>> On Wed, Aug 29, 2018 at 10:21 AM, Scott Branden
>> >>>>>> <scott.branden@broadcom.com> wrote:
>> >>>>>>>
>> >>>>>>> Enable EFI_ARMSTUB_DTB_LOADER to add support for the dtb= command line
>> >>>>>>> parameter to function with efi loader.
>> >>>>>>>
>> >>>>>>> Required to boot on existing bootloaders that do not support devicetree
>> >>>>>>> provided by the platform or by the bootloader.
>> >>>>>>>
>> >>>>>>> Fixes: 3d7ee348aa41 ("efi/libstub/arm: Add opt-in Kconfig option for the
>> >>>>>>> DTB loader")
>> >>>>>>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>> >>>>>>
>> >>>>>> Why did Ard create an option for this if it's just going be turned on
>> >>>>>> in default configs? Doesn't make sense to me.
>> >>>>>>
>> >>>>>> It would help to know what firmware still is crippled and how common
>> >>>>>> it is, since it's been a few years that this has been a requirement by
>> >>>>>> now.
>> >>>>>
>> >>>>> Broadcom NS2 and Stingray in current development and production need this
>> >>>>> option in the kernel enabled in order to boot.
>> >>>>
>> >>>> And these production systems run mainline kernels in a defconfig configuration?
>> >>>>
>> >>>> The simply reality is that the DTB loader has been deprecated for a
>> >>>> good reason: it was only ever intended as a development hack anyway,
>> >>>> and if we need to treat the EFI stub provided DTB as a first class
>> >>>> citizen, there are things we need to fix to make things works as
>> >>>> expected. For instance, GRUB will put a property in the /chosen node
>> >>>> for the initramfs which will get dropped if you boot with dtb=.
>> >>>>
>> >>>> Don't be surprised if some future enhancements of the EFI stub code
>> >>>> depend on !EFI_ARMSTUB_DTB_LOADER.
>
> That's an odd statement to make. The DTB loader code is well contained
> and with defined semantics... True, the semantics are "I DON'T BELIEVE
> FIRMWARE", but it is still well defined. What scenario are you
> envisioning where EFI_ARMSTUB_DTB_LOADER would be explicitly excluded?
>
> Conversely, the dtb= argument is an invaluable debug tool during
> development. As Olof has already said, there are a lot of embedded
> deployments where there is no desire for grub or any other
> intermediary loader.
>
>> >>>> On UEFI systems, DTBs [or ACPI
>> >>>> tables] are used by the firmware to describe itself and the underlying
>> >>>> platform to the OS, and the practice of booting with DTB file images
>> >>>> (taken from the kernel build as well) conflicts with that view. Note
>> >>>> that GRUB still permits you to load DTBs from files (and supports more
>> >>>> sources than just the file system the kernel Image was loaded from).
>> >>>
>> >>> Ard,
>> >>>
>> >>> Maybe a WARN() splat would be more useful as a phasing-out method than
>> >>> removing functionality for them that needs to be reinstated through
>> >>> changing the config?
>> >>>
>> >>
>> >> We don't have any of that in the stub, and inventing new ways to pass
>> >> such information between the stub and the kernel proper seems like a
>> >> cart-before-horse kind of thing to me. The EFI stub diagnostic
>> >> messages you get on the serial console are not recorded in the kernel
>> >> log buffer, so they only appear if you actually look at the serial
>> >> output.
>
> As an aside, they probably should be recorded. That is probably a
> question for the UEFI USWG. Grub and the ARMSTUB could probably bodge
> something together, but that would be non-standard.
Having a way to pass firmware console messages onto the kernel is a
generic problem; different firmware stacks seem to solve this in
different ways. Having a unified way of passing, say, a text buffer to
the kernel filled with the firmware console log would indeed be useful
for everyone, not just EFI.
>> > Ah yeah. I suppose you could do it in the kernel later if you detect
>> > you've booted through EFI with dtb= on the command line though.
>> >
>> >>
>> >>> Once the stub and the boot method is there, it's hard to undo as we
>> >>> can see here. Being loud and warn might be more useful, and set a
>> >>> timeline for hard removal (12 months?).
>> >>>
>> >>
>> >> The dtb= handling is still there, it is just not enabled by default.
>> >> We can keep it around if people are still using it. But as I pointed
>> >> out, we may decide to make new functionality available only if it is
>> >> disabled, and at that point, we'll have to choose between one or the
>> >> other in defconfig, which is annoying.
>> >>
>> >>> Scott; an alternative for you is to do a boot wrapper that bundles a
>> >>> DT and kernel, and boot that instead of the kernel image (outside of
>> >>> the kernel tree). Some 32-bit platforms from Marvell use that. That
>> >>> way the kernel will just see it as a normally passed in DT.
>> >>>
>> >>
>> >> Or use GRUB. It comes wired up in all the distros, and let's you load
>> >> a DT binary from anywhere you can imagine, as opposed to the EFI stub
>> >> which can only load it if it happens to reside in the same file system
>> >> (or even directory - I can't remember) as the kernel image. Note that
>> >> the same reservations apply to doing that - the firmware is no longer
>> >> able to describe itself to the OS via the DT, which is really the only
>> >> conduit it has available on an arm64 system..
>> >
>> > So, I've looked at the history here a bit, and dtb= support was
>> > introduced in 2014. Nowhere does it say that it isn't a recommended
>> > way of booting.
>> >
>> > There are some firmware stacks today that modify and provide a
>> > runtime-updated devicetree to the kernel, but there are also a bunch
>> > who don't. Most "real" products will want a firmware that knows how to
>> > pass in things such as firmware environment variables, or MAC
>> > addresses, etc, to the kernel, but not all of them need it.
>> >
>> > In particular, in a world where you want EFI to be used on embedded
>> > platforms, requiring another bootloader step such as GRUB to be able
>> > to reasonably boot said platforms seems like a significant and
>> > unfortunate new limitation. Documentation/efi-stub.txt has absolutely
>> > no indication that it is a second-class option that isn't expected to
>> > be available everywhere. It doesn't really matter what _your_
>> > intention was around it, if those who use it never found out and now
>> > rely on it.
>> >
>> > Unfortunately the way forward here is to revert 3d7ee348aa4127a.
>> >
>>
>> I agree with your analysis but not with your conclusion.
>>
>> Whether or not the option is def_bool y and/or enabled in defconfig is
>> a matter of policy. ACPI-only distros such as RHEL are definitely
>> going to disable this option. But in general, supporting DTBs loaded
>> from files is a huge pain for the distros, so I expect most of them to
>> disable it as well.
>
> I support leaving 3d7ee348 in, and making it def_bool y
Again, with the argument above -- I question its presence at all. We
usually don't pick up kernel changes just because a distro wants to
make a policy statement.
In this case, there is no genuine technical motivation for this
change, especially when the parallel "initrd=" option is still there.
-Olof
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-04 10:13 ` Grant Likely
2018-09-04 17:19 ` Scott Branden
2018-09-04 17:28 ` Olof Johansson
@ 2018-09-05 9:37 ` Ard Biesheuvel
2018-09-05 19:09 ` Grant Likely
2018-09-10 17:53 ` Scott Branden
3 siblings, 1 reply; 25+ messages in thread
From: Ard Biesheuvel @ 2018-09-05 9:37 UTC (permalink / raw)
To: Grant Likely
Cc: Olof Johansson, Scott Branden, Catalin Marinas, Will Deacon,
Arnd Bergmann, BCM Kernel Feedback, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
Hi Grant,
Thanks for chiming in.
On 4 September 2018 at 12:13, Grant Likely <grant.likely@secretlab.ca> wrote:
> Hey folks. More comments below, but the short answer is I really don't
> see what the problem is. Distros cannot easily support platforms that
> require a dtb= parameter, and so they probably won't. They may or may
> not disable 'dtb=', depending on whether they see it as valuable for
> debug.
>
> Vertically integrated platforms are a different beast. We may strongly
> recommend firmware provides the dtb for all the mentioned good
> reasons, but they still get to decide their deployment methodology,
> and it is not burdensome for the kernel to keep the dtb= feature that
> they are using.
>
> On Tue, Sep 4, 2018 at 7:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> On 2 September 2018 at 04:54, Olof Johansson <olof@lixom.net> wrote:
>> > On Thu, Aug 30, 2018 at 9:23 AM, Ard Biesheuvel
>> > <ard.biesheuvel@linaro.org> wrote:
>> >> On 30 August 2018 at 17:06, Olof Johansson <olof@lixom.net> wrote:
>> >>> On Wed, Aug 29, 2018 at 10:54 PM, Ard Biesheuvel
>> >>> <ard.biesheuvel@linaro.org> wrote:
>> >>>> On 29 August 2018 at 20:59, Scott Branden <scott.branden@broadcom.com> wrote:
>> >>>>> Hi Olof,
>> >>>>>
>> >>>>>
>> >>>>> On 18-08-29 11:44 AM, Olof Johansson wrote:
>> >>>>>>
>> >>>>>> Hi,
>> >>>>>>
>> >>>>>> On Wed, Aug 29, 2018 at 10:21 AM, Scott Branden
>> >>>>>> <scott.branden@broadcom.com> wrote:
>> >>>>>>>
>> >>>>>>> Enable EFI_ARMSTUB_DTB_LOADER to add support for the dtb= command line
>> >>>>>>> parameter to function with efi loader.
>> >>>>>>>
>> >>>>>>> Required to boot on existing bootloaders that do not support devicetree
>> >>>>>>> provided by the platform or by the bootloader.
>> >>>>>>>
>> >>>>>>> Fixes: 3d7ee348aa41 ("efi/libstub/arm: Add opt-in Kconfig option for the
>> >>>>>>> DTB loader")
>> >>>>>>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>> >>>>>>
>> >>>>>> Why did Ard create an option for this if it's just going be turned on
>> >>>>>> in default configs? Doesn't make sense to me.
>> >>>>>>
>> >>>>>> It would help to know what firmware still is crippled and how common
>> >>>>>> it is, since it's been a few years that this has been a requirement by
>> >>>>>> now.
>> >>>>>
>> >>>>> Broadcom NS2 and Stingray in current development and production need this
>> >>>>> option in the kernel enabled in order to boot.
>> >>>>
>> >>>> And these production systems run mainline kernels in a defconfig configuration?
>> >>>>
>> >>>> The simply reality is that the DTB loader has been deprecated for a
>> >>>> good reason: it was only ever intended as a development hack anyway,
>> >>>> and if we need to treat the EFI stub provided DTB as a first class
>> >>>> citizen, there are things we need to fix to make things works as
>> >>>> expected. For instance, GRUB will put a property in the /chosen node
>> >>>> for the initramfs which will get dropped if you boot with dtb=.
>> >>>>
>> >>>> Don't be surprised if some future enhancements of the EFI stub code
>> >>>> depend on !EFI_ARMSTUB_DTB_LOADER.
>
> That's an odd statement to make. The DTB loader code is well contained
> and with defined semantics... True, the semantics are "I DON'T BELIEVE
> FIRMWARE", but it is still well defined. What scenario are you
> envisioning where EFI_ARMSTUB_DTB_LOADER would be explicitly excluded?
>
Well, to be honest, I don't have a real-world example at hand, but my
concern is about cases where the firmware provided DTB and the
override DTB diverge in a way that leaves it up to the EFI stub to
reconcile them and/or to reason about which one it should prefer.
One example could be OP-TEE support: currently, we put a
/firmware/optee node in the DT to inform the OS that OP-TEE is running
in the secure world. If we allow a DT to be provided via dtb= that
provides such a node, we are blocking all future opportunities in
future kernels to do any kind of preparatory OP-TEE related
initialization in the EFI stub [while boot services are still
available] unless we decide to make it the EFI stub's problem to
reason about which version of the DT is the correct one to use. What
if the firmware's DT has /firmware/optee/status = disabled and the
dtb= one does not?
Another trivial example is GRUB: passing dtb= via the command line
will break initrds loaded via GRUB, since they are passed via the
/chosen node.
> Conversely, the dtb= argument is an invaluable debug tool during
> development. As Olof has already said, there are a lot of embedded
> deployments where there is no desire for grub or any other
> intermediary loader.
>
Agreed.
So perhaps the conclusion here is that dtb= should be supported fully
if no DTB is provided by the firmware in the first place, and only
marginally (perhaps with a taint) when the firmware provides one as
well.
>> >>>> On UEFI systems, DTBs [or ACPI
>> >>>> tables] are used by the firmware to describe itself and the underlying
>> >>>> platform to the OS, and the practice of booting with DTB file images
>> >>>> (taken from the kernel build as well) conflicts with that view. Note
>> >>>> that GRUB still permits you to load DTBs from files (and supports more
>> >>>> sources than just the file system the kernel Image was loaded from).
>> >>>
>> >>> Ard,
>> >>>
>> >>> Maybe a WARN() splat would be more useful as a phasing-out method than
>> >>> removing functionality for them that needs to be reinstated through
>> >>> changing the config?
>> >>>
>> >>
>> >> We don't have any of that in the stub, and inventing new ways to pass
>> >> such information between the stub and the kernel proper seems like a
>> >> cart-before-horse kind of thing to me. The EFI stub diagnostic
>> >> messages you get on the serial console are not recorded in the kernel
>> >> log buffer, so they only appear if you actually look at the serial
>> >> output.
>
> As an aside, they probably should be recorded. That is probably a
> question for the UEFI USWG. Grub and the ARMSTUB could probably bodge
> something together, but that would be non-standard.
>
>> >
>> > Ah yeah. I suppose you could do it in the kernel later if you detect
>> > you've booted through EFI with dtb= on the command line though.
>> >
>> >>
>> >>> Once the stub and the boot method is there, it's hard to undo as we
>> >>> can see here. Being loud and warn might be more useful, and set a
>> >>> timeline for hard removal (12 months?).
>> >>>
>> >>
>> >> The dtb= handling is still there, it is just not enabled by default.
>> >> We can keep it around if people are still using it. But as I pointed
>> >> out, we may decide to make new functionality available only if it is
>> >> disabled, and at that point, we'll have to choose between one or the
>> >> other in defconfig, which is annoying.
>> >>
>> >>> Scott; an alternative for you is to do a boot wrapper that bundles a
>> >>> DT and kernel, and boot that instead of the kernel image (outside of
>> >>> the kernel tree). Some 32-bit platforms from Marvell use that. That
>> >>> way the kernel will just see it as a normally passed in DT.
>> >>>
>> >>
>> >> Or use GRUB. It comes wired up in all the distros, and let's you load
>> >> a DT binary from anywhere you can imagine, as opposed to the EFI stub
>> >> which can only load it if it happens to reside in the same file system
>> >> (or even directory - I can't remember) as the kernel image. Note that
>> >> the same reservations apply to doing that - the firmware is no longer
>> >> able to describe itself to the OS via the DT, which is really the only
>> >> conduit it has available on an arm64 system..
>> >
>> > So, I've looked at the history here a bit, and dtb= support was
>> > introduced in 2014. Nowhere does it say that it isn't a recommended
>> > way of booting.
>> >
>> > There are some firmware stacks today that modify and provide a
>> > runtime-updated devicetree to the kernel, but there are also a bunch
>> > who don't. Most "real" products will want a firmware that knows how to
>> > pass in things such as firmware environment variables, or MAC
>> > addresses, etc, to the kernel, but not all of them need it.
>> >
>> > In particular, in a world where you want EFI to be used on embedded
>> > platforms, requiring another bootloader step such as GRUB to be able
>> > to reasonably boot said platforms seems like a significant and
>> > unfortunate new limitation. Documentation/efi-stub.txt has absolutely
>> > no indication that it is a second-class option that isn't expected to
>> > be available everywhere. It doesn't really matter what _your_
>> > intention was around it, if those who use it never found out and now
>> > rely on it.
>> >
>> > Unfortunately the way forward here is to revert 3d7ee348aa4127a.
>> >
>>
>> I agree with your analysis but not with your conclusion.
>>
>> Whether or not the option is def_bool y and/or enabled in defconfig is
>> a matter of policy. ACPI-only distros such as RHEL are definitely
>> going to disable this option. But in general, supporting DTBs loaded
>> from files is a huge pain for the distros, so I expect most of them to
>> disable it as well.
>
> I support leaving 3d7ee348 in, and making it def_bool y
>
> g.
>>
>> As for EFI on embedded systems: this will be mostly on U-boot's
>> bootefi implementation, which definitely does the right thing when it
>> comes to passing the DTB via a UEFI configuration table (regardless of
>> whether it makes any modifications to it)
>>
>> In any case, I won't object to a patch that reenables the EFI stub DTB
>> loader in defconfig. Whether or not it should be def_bool y is
>> something we can discuss as well. I have added Leif and Alex to cc,
>> perhaps they have anything to add.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-05 9:37 ` Ard Biesheuvel
@ 2018-09-05 19:09 ` Grant Likely
0 siblings, 0 replies; 25+ messages in thread
From: Grant Likely @ 2018-09-05 19:09 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Olof Johansson, scott.branden, Catalin Marinas, Will Deacon,
Arnd Bergmann, bcm-kernel-feedback-list, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On Wed, Sep 5, 2018 at 10:37 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 4 September 2018 at 12:13, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Tue, Sep 4, 2018 at 7:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >>
> >> On 2 September 2018 at 04:54, Olof Johansson <olof@lixom.net> wrote:
> >> > On Thu, Aug 30, 2018 at 9:23 AM, Ard Biesheuvel
> >> > <ard.biesheuvel@linaro.org> wrote:
> >> >> On 30 August 2018 at 17:06, Olof Johansson <olof@lixom.net> wrote:
> >> >>> On Wed, Aug 29, 2018 at 10:54 PM, Ard Biesheuvel
> >> >>> <ard.biesheuvel@linaro.org> wrote:
> >> >>>> Don't be surprised if some future enhancements of the EFI stub code
> >> >>>> depend on !EFI_ARMSTUB_DTB_LOADER.
> >
> > That's an odd statement to make. The DTB loader code is well contained
> > and with defined semantics... True, the semantics are "I DON'T BELIEVE
> > FIRMWARE", but it is still well defined. What scenario are you
> > envisioning where EFI_ARMSTUB_DTB_LOADER would be explicitly excluded?
> >
>
> Well, to be honest, I don't have a real-world example at hand, but my
> concern is about cases where the firmware provided DTB and the
> override DTB diverge in a way that leaves it up to the EFI stub to
> reconcile them and/or to reason about which one it should prefer.
>
> One example could be OP-TEE support: currently, we put a
> /firmware/optee node in the DT to inform the OS that OP-TEE is running
> in the secure world. If we allow a DT to be provided via dtb= that
> provides such a node, we are blocking all future opportunities in
> future kernels to do any kind of preparatory OP-TEE related
> initialization in the EFI stub [while boot services are still
> available] unless we decide to make it the EFI stub's problem to
> reason about which version of the DT is the correct one to use. What
> if the firmware's DT has /firmware/optee/status = disabled and the
> dtb= one does not?
>
> Another trivial example is GRUB: passing dtb= via the command line
> will break initrds loaded via GRUB, since they are passed via the
> /chosen node.
Using 'dtb=' straight out means *I don't believe anything firmware
tells me*, so of course nothing like OP-TEE integration, command line
passing, dynamic configuration, or anything that firmware might want
to tell the kernel is going to work. Anyone who uses dtb= gets to keep
the pieces when they break stuff. That can be written down into policy
in the dtb= documentation if you like. I've just posted a patch to do
that.
g.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-04 10:13 ` Grant Likely
` (2 preceding siblings ...)
2018-09-05 9:37 ` Ard Biesheuvel
@ 2018-09-10 17:53 ` Scott Branden
2018-09-10 18:01 ` Olof Johansson
3 siblings, 1 reply; 25+ messages in thread
From: Scott Branden @ 2018-09-10 17:53 UTC (permalink / raw)
To: Grant Likely, Ard Biesheuvel
Cc: Olof Johansson, Catalin Marinas, Will Deacon, Arnd Bergmann,
bcm-kernel-feedback-list, Linux ARM, Linux Kernel Mailing List,
Leif Lindholm, Alexander Graf
Olof/All,
On 18-09-04 03:13 AM, Grant Likely wrote:
> Hey folks. More comments below, but the short answer is I really don't
> see what the problem is. Distros cannot easily support platforms that
> require a dtb= parameter, and so they probably won't. They may or may
> not disable 'dtb=', depending on whether they see it as valuable for
> debug.
>
> Vertically integrated platforms are a different beast. We may strongly
> recommend firmware provides the dtb for all the mentioned good
> reasons, but they still get to decide their deployment methodology,
> and it is not burdensome for the kernel to keep the dtb= feature that
> they are using.
>
> On Tue, Sep 4, 2018 at 7:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 2 September 2018 at 04:54, Olof Johansson <olof@lixom.net> wrote:
>>> On Thu, Aug 30, 2018 at 9:23 AM, Ard Biesheuvel
>>> <ard.biesheuvel@linaro.org> wrote:
>>>> On 30 August 2018 at 17:06, Olof Johansson <olof@lixom.net> wrote:
>>>>> On Wed, Aug 29, 2018 at 10:54 PM, Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>> On 29 August 2018 at 20:59, Scott Branden <scott.branden@broadcom.com> wrote:
>>>>>>> Hi Olof,
>>>>>>>
>>>>>>>
>>>>>>> On 18-08-29 11:44 AM, Olof Johansson wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Wed, Aug 29, 2018 at 10:21 AM, Scott Branden
>>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>> Enable EFI_ARMSTUB_DTB_LOADER to add support for the dtb= command line
>>>>>>>>> parameter to function with efi loader.
>>>>>>>>>
>>>>>>>>> Required to boot on existing bootloaders that do not support devicetree
>>>>>>>>> provided by the platform or by the bootloader.
>>>>>>>>>
>>>>>>>>> Fixes: 3d7ee348aa41 ("efi/libstub/arm: Add opt-in Kconfig option for the
>>>>>>>>> DTB loader")
>>>>>>>>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>>>>>>>> Why did Ard create an option for this if it's just going be turned on
>>>>>>>> in default configs? Doesn't make sense to me.
>>>>>>>>
>>>>>>>> It would help to know what firmware still is crippled and how common
>>>>>>>> it is, since it's been a few years that this has been a requirement by
>>>>>>>> now.
>>>>>>> Broadcom NS2 and Stingray in current development and production need this
>>>>>>> option in the kernel enabled in order to boot.
>>>>>> And these production systems run mainline kernels in a defconfig configuration?
>>>>>>
>>>>>> The simply reality is that the DTB loader has been deprecated for a
>>>>>> good reason: it was only ever intended as a development hack anyway,
>>>>>> and if we need to treat the EFI stub provided DTB as a first class
>>>>>> citizen, there are things we need to fix to make things works as
>>>>>> expected. For instance, GRUB will put a property in the /chosen node
>>>>>> for the initramfs which will get dropped if you boot with dtb=.
>>>>>>
>>>>>> Don't be surprised if some future enhancements of the EFI stub code
>>>>>> depend on !EFI_ARMSTUB_DTB_LOADER.
> That's an odd statement to make. The DTB loader code is well contained
> and with defined semantics... True, the semantics are "I DON'T BELIEVE
> FIRMWARE", but it is still well defined. What scenario are you
> envisioning where EFI_ARMSTUB_DTB_LOADER would be explicitly excluded?
>
> Conversely, the dtb= argument is an invaluable debug tool during
> development. As Olof has already said, there are a lot of embedded
> deployments where there is no desire for grub or any other
> intermediary loader.
>
>>>>>> On UEFI systems, DTBs [or ACPI
>>>>>> tables] are used by the firmware to describe itself and the underlying
>>>>>> platform to the OS, and the practice of booting with DTB file images
>>>>>> (taken from the kernel build as well) conflicts with that view. Note
>>>>>> that GRUB still permits you to load DTBs from files (and supports more
>>>>>> sources than just the file system the kernel Image was loaded from).
>>>>> Ard,
>>>>>
>>>>> Maybe a WARN() splat would be more useful as a phasing-out method than
>>>>> removing functionality for them that needs to be reinstated through
>>>>> changing the config?
>>>>>
>>>> We don't have any of that in the stub, and inventing new ways to pass
>>>> such information between the stub and the kernel proper seems like a
>>>> cart-before-horse kind of thing to me. The EFI stub diagnostic
>>>> messages you get on the serial console are not recorded in the kernel
>>>> log buffer, so they only appear if you actually look at the serial
>>>> output.
> As an aside, they probably should be recorded. That is probably a
> question for the UEFI USWG. Grub and the ARMSTUB could probably bodge
> something together, but that would be non-standard.
>
>>> Ah yeah. I suppose you could do it in the kernel later if you detect
>>> you've booted through EFI with dtb= on the command line though.
>>>
>>>>> Once the stub and the boot method is there, it's hard to undo as we
>>>>> can see here. Being loud and warn might be more useful, and set a
>>>>> timeline for hard removal (12 months?).
>>>>>
>>>> The dtb= handling is still there, it is just not enabled by default.
>>>> We can keep it around if people are still using it. But as I pointed
>>>> out, we may decide to make new functionality available only if it is
>>>> disabled, and at that point, we'll have to choose between one or the
>>>> other in defconfig, which is annoying.
>>>>
>>>>> Scott; an alternative for you is to do a boot wrapper that bundles a
>>>>> DT and kernel, and boot that instead of the kernel image (outside of
>>>>> the kernel tree). Some 32-bit platforms from Marvell use that. That
>>>>> way the kernel will just see it as a normally passed in DT.
>>>>>
>>>> Or use GRUB. It comes wired up in all the distros, and let's you load
>>>> a DT binary from anywhere you can imagine, as opposed to the EFI stub
>>>> which can only load it if it happens to reside in the same file system
>>>> (or even directory - I can't remember) as the kernel image. Note that
>>>> the same reservations apply to doing that - the firmware is no longer
>>>> able to describe itself to the OS via the DT, which is really the only
>>>> conduit it has available on an arm64 system..
>>> So, I've looked at the history here a bit, and dtb= support was
>>> introduced in 2014. Nowhere does it say that it isn't a recommended
>>> way of booting.
>>>
>>> There are some firmware stacks today that modify and provide a
>>> runtime-updated devicetree to the kernel, but there are also a bunch
>>> who don't. Most "real" products will want a firmware that knows how to
>>> pass in things such as firmware environment variables, or MAC
>>> addresses, etc, to the kernel, but not all of them need it.
>>>
>>> In particular, in a world where you want EFI to be used on embedded
>>> platforms, requiring another bootloader step such as GRUB to be able
>>> to reasonably boot said platforms seems like a significant and
>>> unfortunate new limitation. Documentation/efi-stub.txt has absolutely
>>> no indication that it is a second-class option that isn't expected to
>>> be available everywhere. It doesn't really matter what _your_
>>> intention was around it, if those who use it never found out and now
>>> rely on it.
>>>
>>> Unfortunately the way forward here is to revert 3d7ee348aa4127a.
What's the path forward? Revert, defconfig change (this patch), or
Kconfig default addition?
>> I agree with your analysis but not with your conclusion.
>>
>> Whether or not the option is def_bool y and/or enabled in defconfig is
>> a matter of policy. ACPI-only distros such as RHEL are definitely
>> going to disable this option. But in general, supporting DTBs loaded
>> from files is a huge pain for the distros, so I expect most of them to
>> disable it as well.
> I support leaving 3d7ee348 in, and making it def_bool y
>
> g.
>> As for EFI on embedded systems: this will be mostly on U-boot's
>> bootefi implementation, which definitely does the right thing when it
>> comes to passing the DTB via a UEFI configuration table (regardless of
>> whether it makes any modifications to it)
>>
>> In any case, I won't object to a patch that reenables the EFI stub DTB
>> loader in defconfig. Whether or not it should be def_bool y is
>> something we can discuss as well. I have added Leif and Alex to cc,
>> perhaps they have anything to add.
Thanks,
Scott
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-10 17:53 ` Scott Branden
@ 2018-09-10 18:01 ` Olof Johansson
2018-09-10 18:08 ` Ard Biesheuvel
0 siblings, 1 reply; 25+ messages in thread
From: Olof Johansson @ 2018-09-10 18:01 UTC (permalink / raw)
To: Scott Branden
Cc: Grant Likely, Ard Biesheuvel, Catalin Marinas, Will Deacon,
Arnd Bergmann, Broadcom Kernel Feedback List, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On Mon, Sep 10, 2018 at 10:53 AM, Scott Branden
<scott.branden@broadcom.com> wrote:
> Olof/All,
>
>
> On 18-09-04 03:13 AM, Grant Likely wrote:
>>
>> Hey folks. More comments below, but the short answer is I really don't
>> see what the problem is. Distros cannot easily support platforms that
>> require a dtb= parameter, and so they probably won't. They may or may
>> not disable 'dtb=', depending on whether they see it as valuable for
>> debug.
>>
>> Vertically integrated platforms are a different beast. We may strongly
>> recommend firmware provides the dtb for all the mentioned good
>> reasons, but they still get to decide their deployment methodology,
>> and it is not burdensome for the kernel to keep the dtb= feature that
>> they are using.
>>
>> On Tue, Sep 4, 2018 at 7:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> wrote:
>>>
>>> On 2 September 2018 at 04:54, Olof Johansson <olof@lixom.net> wrote:
>>>>
>>>> On Thu, Aug 30, 2018 at 9:23 AM, Ard Biesheuvel
>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>
>>>>> On 30 August 2018 at 17:06, Olof Johansson <olof@lixom.net> wrote:
>>>>>>
>>>>>> On Wed, Aug 29, 2018 at 10:54 PM, Ard Biesheuvel
>>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>>>
>>>>>>> On 29 August 2018 at 20:59, Scott Branden
>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>
>>>>>>>> Hi Olof,
>>>>>>>>
>>>>>>>>
>>>>>>>> On 18-08-29 11:44 AM, Olof Johansson wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Wed, Aug 29, 2018 at 10:21 AM, Scott Branden
>>>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Enable EFI_ARMSTUB_DTB_LOADER to add support for the dtb= command
>>>>>>>>>> line
>>>>>>>>>> parameter to function with efi loader.
>>>>>>>>>>
>>>>>>>>>> Required to boot on existing bootloaders that do not support
>>>>>>>>>> devicetree
>>>>>>>>>> provided by the platform or by the bootloader.
>>>>>>>>>>
>>>>>>>>>> Fixes: 3d7ee348aa41 ("efi/libstub/arm: Add opt-in Kconfig option
>>>>>>>>>> for the
>>>>>>>>>> DTB loader")
>>>>>>>>>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>>>>>>>>>
>>>>>>>>> Why did Ard create an option for this if it's just going be turned
>>>>>>>>> on
>>>>>>>>> in default configs? Doesn't make sense to me.
>>>>>>>>>
>>>>>>>>> It would help to know what firmware still is crippled and how
>>>>>>>>> common
>>>>>>>>> it is, since it's been a few years that this has been a requirement
>>>>>>>>> by
>>>>>>>>> now.
>>>>>>>>
>>>>>>>> Broadcom NS2 and Stingray in current development and production need
>>>>>>>> this
>>>>>>>> option in the kernel enabled in order to boot.
>>>>>>>
>>>>>>> And these production systems run mainline kernels in a defconfig
>>>>>>> configuration?
>>>>>>>
>>>>>>> The simply reality is that the DTB loader has been deprecated for a
>>>>>>> good reason: it was only ever intended as a development hack anyway,
>>>>>>> and if we need to treat the EFI stub provided DTB as a first class
>>>>>>> citizen, there are things we need to fix to make things works as
>>>>>>> expected. For instance, GRUB will put a property in the /chosen node
>>>>>>> for the initramfs which will get dropped if you boot with dtb=.
>>>>>>>
>>>>>>> Don't be surprised if some future enhancements of the EFI stub code
>>>>>>> depend on !EFI_ARMSTUB_DTB_LOADER.
>>
>> That's an odd statement to make. The DTB loader code is well contained
>> and with defined semantics... True, the semantics are "I DON'T BELIEVE
>> FIRMWARE", but it is still well defined. What scenario are you
>> envisioning where EFI_ARMSTUB_DTB_LOADER would be explicitly excluded?
>>
>> Conversely, the dtb= argument is an invaluable debug tool during
>> development. As Olof has already said, there are a lot of embedded
>> deployments where there is no desire for grub or any other
>> intermediary loader.
>>
>>>>>>> On UEFI systems, DTBs [or ACPI
>>>>>>> tables] are used by the firmware to describe itself and the
>>>>>>> underlying
>>>>>>> platform to the OS, and the practice of booting with DTB file images
>>>>>>> (taken from the kernel build as well) conflicts with that view. Note
>>>>>>> that GRUB still permits you to load DTBs from files (and supports
>>>>>>> more
>>>>>>> sources than just the file system the kernel Image was loaded from).
>>>>>>
>>>>>> Ard,
>>>>>>
>>>>>> Maybe a WARN() splat would be more useful as a phasing-out method than
>>>>>> removing functionality for them that needs to be reinstated through
>>>>>> changing the config?
>>>>>>
>>>>> We don't have any of that in the stub, and inventing new ways to pass
>>>>> such information between the stub and the kernel proper seems like a
>>>>> cart-before-horse kind of thing to me. The EFI stub diagnostic
>>>>> messages you get on the serial console are not recorded in the kernel
>>>>> log buffer, so they only appear if you actually look at the serial
>>>>> output.
>>
>> As an aside, they probably should be recorded. That is probably a
>> question for the UEFI USWG. Grub and the ARMSTUB could probably bodge
>> something together, but that would be non-standard.
>>
>>>> Ah yeah. I suppose you could do it in the kernel later if you detect
>>>> you've booted through EFI with dtb= on the command line though.
>>>>
>>>>>> Once the stub and the boot method is there, it's hard to undo as we
>>>>>> can see here. Being loud and warn might be more useful, and set a
>>>>>> timeline for hard removal (12 months?).
>>>>>>
>>>>> The dtb= handling is still there, it is just not enabled by default.
>>>>> We can keep it around if people are still using it. But as I pointed
>>>>> out, we may decide to make new functionality available only if it is
>>>>> disabled, and at that point, we'll have to choose between one or the
>>>>> other in defconfig, which is annoying.
>>>>>
>>>>>> Scott; an alternative for you is to do a boot wrapper that bundles a
>>>>>> DT and kernel, and boot that instead of the kernel image (outside of
>>>>>> the kernel tree). Some 32-bit platforms from Marvell use that. That
>>>>>> way the kernel will just see it as a normally passed in DT.
>>>>>>
>>>>> Or use GRUB. It comes wired up in all the distros, and let's you load
>>>>> a DT binary from anywhere you can imagine, as opposed to the EFI stub
>>>>> which can only load it if it happens to reside in the same file system
>>>>> (or even directory - I can't remember) as the kernel image. Note that
>>>>> the same reservations apply to doing that - the firmware is no longer
>>>>> able to describe itself to the OS via the DT, which is really the only
>>>>> conduit it has available on an arm64 system..
>>>>
>>>> So, I've looked at the history here a bit, and dtb= support was
>>>> introduced in 2014. Nowhere does it say that it isn't a recommended
>>>> way of booting.
>>>>
>>>> There are some firmware stacks today that modify and provide a
>>>> runtime-updated devicetree to the kernel, but there are also a bunch
>>>> who don't. Most "real" products will want a firmware that knows how to
>>>> pass in things such as firmware environment variables, or MAC
>>>> addresses, etc, to the kernel, but not all of them need it.
>>>>
>>>> In particular, in a world where you want EFI to be used on embedded
>>>> platforms, requiring another bootloader step such as GRUB to be able
>>>> to reasonably boot said platforms seems like a significant and
>>>> unfortunate new limitation. Documentation/efi-stub.txt has absolutely
>>>> no indication that it is a second-class option that isn't expected to
>>>> be available everywhere. It doesn't really matter what _your_
>>>> intention was around it, if those who use it never found out and now
>>>> rely on it.
>>>>
>>>> Unfortunately the way forward here is to revert 3d7ee348aa4127a.
>
> What's the path forward? Revert, defconfig change (this patch), or Kconfig
> default addition?
Revert or Kconfig select, and a Kconfig select means that the option
is a dead one anyway so we might as well revert.
Ard, do you have other fixes lined up or should we take the patch
through arm-soc?
Thanks,
-Olof
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-10 18:01 ` Olof Johansson
@ 2018-09-10 18:08 ` Ard Biesheuvel
2018-09-13 20:22 ` Scott Branden
0 siblings, 1 reply; 25+ messages in thread
From: Ard Biesheuvel @ 2018-09-10 18:08 UTC (permalink / raw)
To: Olof Johansson
Cc: Scott Branden, Grant Likely, Catalin Marinas, Will Deacon,
Arnd Bergmann, Broadcom Kernel Feedback List, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On 10 September 2018 at 20:01, Olof Johansson <olof@lixom.net> wrote:
> On Mon, Sep 10, 2018 at 10:53 AM, Scott Branden
> <scott.branden@broadcom.com> wrote:
>> Olof/All,
>>
>>
>> On 18-09-04 03:13 AM, Grant Likely wrote:
>>>
>>> Hey folks. More comments below, but the short answer is I really don't
>>> see what the problem is. Distros cannot easily support platforms that
>>> require a dtb= parameter, and so they probably won't. They may or may
>>> not disable 'dtb=', depending on whether they see it as valuable for
>>> debug.
>>>
>>> Vertically integrated platforms are a different beast. We may strongly
>>> recommend firmware provides the dtb for all the mentioned good
>>> reasons, but they still get to decide their deployment methodology,
>>> and it is not burdensome for the kernel to keep the dtb= feature that
>>> they are using.
>>>
>>> On Tue, Sep 4, 2018 at 7:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> wrote:
>>>>
>>>> On 2 September 2018 at 04:54, Olof Johansson <olof@lixom.net> wrote:
>>>>>
>>>>> On Thu, Aug 30, 2018 at 9:23 AM, Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>>
>>>>>> On 30 August 2018 at 17:06, Olof Johansson <olof@lixom.net> wrote:
>>>>>>>
>>>>>>> On Wed, Aug 29, 2018 at 10:54 PM, Ard Biesheuvel
>>>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>>>>
>>>>>>>> On 29 August 2018 at 20:59, Scott Branden
>>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>>
>>>>>>>>> Hi Olof,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 18-08-29 11:44 AM, Olof Johansson wrote:
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On Wed, Aug 29, 2018 at 10:21 AM, Scott Branden
>>>>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Enable EFI_ARMSTUB_DTB_LOADER to add support for the dtb= command
>>>>>>>>>>> line
>>>>>>>>>>> parameter to function with efi loader.
>>>>>>>>>>>
>>>>>>>>>>> Required to boot on existing bootloaders that do not support
>>>>>>>>>>> devicetree
>>>>>>>>>>> provided by the platform or by the bootloader.
>>>>>>>>>>>
>>>>>>>>>>> Fixes: 3d7ee348aa41 ("efi/libstub/arm: Add opt-in Kconfig option
>>>>>>>>>>> for the
>>>>>>>>>>> DTB loader")
>>>>>>>>>>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>>>>>>>>>>
>>>>>>>>>> Why did Ard create an option for this if it's just going be turned
>>>>>>>>>> on
>>>>>>>>>> in default configs? Doesn't make sense to me.
>>>>>>>>>>
>>>>>>>>>> It would help to know what firmware still is crippled and how
>>>>>>>>>> common
>>>>>>>>>> it is, since it's been a few years that this has been a requirement
>>>>>>>>>> by
>>>>>>>>>> now.
>>>>>>>>>
>>>>>>>>> Broadcom NS2 and Stingray in current development and production need
>>>>>>>>> this
>>>>>>>>> option in the kernel enabled in order to boot.
>>>>>>>>
>>>>>>>> And these production systems run mainline kernels in a defconfig
>>>>>>>> configuration?
>>>>>>>>
>>>>>>>> The simply reality is that the DTB loader has been deprecated for a
>>>>>>>> good reason: it was only ever intended as a development hack anyway,
>>>>>>>> and if we need to treat the EFI stub provided DTB as a first class
>>>>>>>> citizen, there are things we need to fix to make things works as
>>>>>>>> expected. For instance, GRUB will put a property in the /chosen node
>>>>>>>> for the initramfs which will get dropped if you boot with dtb=.
>>>>>>>>
>>>>>>>> Don't be surprised if some future enhancements of the EFI stub code
>>>>>>>> depend on !EFI_ARMSTUB_DTB_LOADER.
>>>
>>> That's an odd statement to make. The DTB loader code is well contained
>>> and with defined semantics... True, the semantics are "I DON'T BELIEVE
>>> FIRMWARE", but it is still well defined. What scenario are you
>>> envisioning where EFI_ARMSTUB_DTB_LOADER would be explicitly excluded?
>>>
>>> Conversely, the dtb= argument is an invaluable debug tool during
>>> development. As Olof has already said, there are a lot of embedded
>>> deployments where there is no desire for grub or any other
>>> intermediary loader.
>>>
>>>>>>>> On UEFI systems, DTBs [or ACPI
>>>>>>>> tables] are used by the firmware to describe itself and the
>>>>>>>> underlying
>>>>>>>> platform to the OS, and the practice of booting with DTB file images
>>>>>>>> (taken from the kernel build as well) conflicts with that view. Note
>>>>>>>> that GRUB still permits you to load DTBs from files (and supports
>>>>>>>> more
>>>>>>>> sources than just the file system the kernel Image was loaded from).
>>>>>>>
>>>>>>> Ard,
>>>>>>>
>>>>>>> Maybe a WARN() splat would be more useful as a phasing-out method than
>>>>>>> removing functionality for them that needs to be reinstated through
>>>>>>> changing the config?
>>>>>>>
>>>>>> We don't have any of that in the stub, and inventing new ways to pass
>>>>>> such information between the stub and the kernel proper seems like a
>>>>>> cart-before-horse kind of thing to me. The EFI stub diagnostic
>>>>>> messages you get on the serial console are not recorded in the kernel
>>>>>> log buffer, so they only appear if you actually look at the serial
>>>>>> output.
>>>
>>> As an aside, they probably should be recorded. That is probably a
>>> question for the UEFI USWG. Grub and the ARMSTUB could probably bodge
>>> something together, but that would be non-standard.
>>>
>>>>> Ah yeah. I suppose you could do it in the kernel later if you detect
>>>>> you've booted through EFI with dtb= on the command line though.
>>>>>
>>>>>>> Once the stub and the boot method is there, it's hard to undo as we
>>>>>>> can see here. Being loud and warn might be more useful, and set a
>>>>>>> timeline for hard removal (12 months?).
>>>>>>>
>>>>>> The dtb= handling is still there, it is just not enabled by default.
>>>>>> We can keep it around if people are still using it. But as I pointed
>>>>>> out, we may decide to make new functionality available only if it is
>>>>>> disabled, and at that point, we'll have to choose between one or the
>>>>>> other in defconfig, which is annoying.
>>>>>>
>>>>>>> Scott; an alternative for you is to do a boot wrapper that bundles a
>>>>>>> DT and kernel, and boot that instead of the kernel image (outside of
>>>>>>> the kernel tree). Some 32-bit platforms from Marvell use that. That
>>>>>>> way the kernel will just see it as a normally passed in DT.
>>>>>>>
>>>>>> Or use GRUB. It comes wired up in all the distros, and let's you load
>>>>>> a DT binary from anywhere you can imagine, as opposed to the EFI stub
>>>>>> which can only load it if it happens to reside in the same file system
>>>>>> (or even directory - I can't remember) as the kernel image. Note that
>>>>>> the same reservations apply to doing that - the firmware is no longer
>>>>>> able to describe itself to the OS via the DT, which is really the only
>>>>>> conduit it has available on an arm64 system..
>>>>>
>>>>> So, I've looked at the history here a bit, and dtb= support was
>>>>> introduced in 2014. Nowhere does it say that it isn't a recommended
>>>>> way of booting.
>>>>>
>>>>> There are some firmware stacks today that modify and provide a
>>>>> runtime-updated devicetree to the kernel, but there are also a bunch
>>>>> who don't. Most "real" products will want a firmware that knows how to
>>>>> pass in things such as firmware environment variables, or MAC
>>>>> addresses, etc, to the kernel, but not all of them need it.
>>>>>
>>>>> In particular, in a world where you want EFI to be used on embedded
>>>>> platforms, requiring another bootloader step such as GRUB to be able
>>>>> to reasonably boot said platforms seems like a significant and
>>>>> unfortunate new limitation. Documentation/efi-stub.txt has absolutely
>>>>> no indication that it is a second-class option that isn't expected to
>>>>> be available everywhere. It doesn't really matter what _your_
>>>>> intention was around it, if those who use it never found out and now
>>>>> rely on it.
>>>>>
>>>>> Unfortunately the way forward here is to revert 3d7ee348aa4127a.
>>
>> What's the path forward? Revert, defconfig change (this patch), or Kconfig
>> default addition?
>
> Revert or Kconfig select, and a Kconfig select means that the option
> is a dead one anyway so we might as well revert.
>
I disagree. Making it default y is fine by me, but please don't remove it.
> Ard, do you have other fixes lined up or should we take the patch
> through arm-soc?
>
I don't have any fixes but either way is fine.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-10 18:08 ` Ard Biesheuvel
@ 2018-09-13 20:22 ` Scott Branden
2018-09-14 6:06 ` Ard Biesheuvel
0 siblings, 1 reply; 25+ messages in thread
From: Scott Branden @ 2018-09-13 20:22 UTC (permalink / raw)
To: Ard Biesheuvel, Olof Johansson
Cc: Grant Likely, Catalin Marinas, Will Deacon, Arnd Bergmann,
Broadcom Kernel Feedback List, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On 18-09-10 11:08 AM, Ard Biesheuvel wrote:
> On 10 September 2018 at 20:01, Olof Johansson <olof@lixom.net> wrote:
>> On Mon, Sep 10, 2018 at 10:53 AM, Scott Branden
>> <scott.branden@broadcom.com> wrote:
>>> Olof/All,
>>>
>>>
>>> On 18-09-04 03:13 AM, Grant Likely wrote:
>>>> Hey folks. More comments below, but the short answer is I really don't
>>>> see what the problem is. Distros cannot easily support platforms that
>>>> require a dtb= parameter, and so they probably won't. They may or may
>>>> not disable 'dtb=', depending on whether they see it as valuable for
>>>> debug.
>>>>
>>>> Vertically integrated platforms are a different beast. We may strongly
>>>> recommend firmware provides the dtb for all the mentioned good
>>>> reasons, but they still get to decide their deployment methodology,
>>>> and it is not burdensome for the kernel to keep the dtb= feature that
>>>> they are using.
>>>>
>>>> On Tue, Sep 4, 2018 at 7:24 AM Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> wrote:
>>>>> On 2 September 2018 at 04:54, Olof Johansson <olof@lixom.net> wrote:
>>>>>> On Thu, Aug 30, 2018 at 9:23 AM, Ard Biesheuvel
>>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>>> On 30 August 2018 at 17:06, Olof Johansson <olof@lixom.net> wrote:
>>>>>>>> On Wed, Aug 29, 2018 at 10:54 PM, Ard Biesheuvel
>>>>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>>>>> On 29 August 2018 at 20:59, Scott Branden
>>>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>>> Hi Olof,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 18-08-29 11:44 AM, Olof Johansson wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Aug 29, 2018 at 10:21 AM, Scott Branden
>>>>>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>>>>> Enable EFI_ARMSTUB_DTB_LOADER to add support for the dtb= command
>>>>>>>>>>>> line
>>>>>>>>>>>> parameter to function with efi loader.
>>>>>>>>>>>>
>>>>>>>>>>>> Required to boot on existing bootloaders that do not support
>>>>>>>>>>>> devicetree
>>>>>>>>>>>> provided by the platform or by the bootloader.
>>>>>>>>>>>>
>>>>>>>>>>>> Fixes: 3d7ee348aa41 ("efi/libstub/arm: Add opt-in Kconfig option
>>>>>>>>>>>> for the
>>>>>>>>>>>> DTB loader")
>>>>>>>>>>>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>>>>>>>>>>> Why did Ard create an option for this if it's just going be turned
>>>>>>>>>>> on
>>>>>>>>>>> in default configs? Doesn't make sense to me.
>>>>>>>>>>>
>>>>>>>>>>> It would help to know what firmware still is crippled and how
>>>>>>>>>>> common
>>>>>>>>>>> it is, since it's been a few years that this has been a requirement
>>>>>>>>>>> by
>>>>>>>>>>> now.
>>>>>>>>>> Broadcom NS2 and Stingray in current development and production need
>>>>>>>>>> this
>>>>>>>>>> option in the kernel enabled in order to boot.
>>>>>>>>> And these production systems run mainline kernels in a defconfig
>>>>>>>>> configuration?
>>>>>>>>>
>>>>>>>>> The simply reality is that the DTB loader has been deprecated for a
>>>>>>>>> good reason: it was only ever intended as a development hack anyway,
>>>>>>>>> and if we need to treat the EFI stub provided DTB as a first class
>>>>>>>>> citizen, there are things we need to fix to make things works as
>>>>>>>>> expected. For instance, GRUB will put a property in the /chosen node
>>>>>>>>> for the initramfs which will get dropped if you boot with dtb=.
>>>>>>>>>
>>>>>>>>> Don't be surprised if some future enhancements of the EFI stub code
>>>>>>>>> depend on !EFI_ARMSTUB_DTB_LOADER.
>>>> That's an odd statement to make. The DTB loader code is well contained
>>>> and with defined semantics... True, the semantics are "I DON'T BELIEVE
>>>> FIRMWARE", but it is still well defined. What scenario are you
>>>> envisioning where EFI_ARMSTUB_DTB_LOADER would be explicitly excluded?
>>>>
>>>> Conversely, the dtb= argument is an invaluable debug tool during
>>>> development. As Olof has already said, there are a lot of embedded
>>>> deployments where there is no desire for grub or any other
>>>> intermediary loader.
>>>>
>>>>>>>>> On UEFI systems, DTBs [or ACPI
>>>>>>>>> tables] are used by the firmware to describe itself and the
>>>>>>>>> underlying
>>>>>>>>> platform to the OS, and the practice of booting with DTB file images
>>>>>>>>> (taken from the kernel build as well) conflicts with that view. Note
>>>>>>>>> that GRUB still permits you to load DTBs from files (and supports
>>>>>>>>> more
>>>>>>>>> sources than just the file system the kernel Image was loaded from).
>>>>>>>> Ard,
>>>>>>>>
>>>>>>>> Maybe a WARN() splat would be more useful as a phasing-out method than
>>>>>>>> removing functionality for them that needs to be reinstated through
>>>>>>>> changing the config?
>>>>>>>>
>>>>>>> We don't have any of that in the stub, and inventing new ways to pass
>>>>>>> such information between the stub and the kernel proper seems like a
>>>>>>> cart-before-horse kind of thing to me. The EFI stub diagnostic
>>>>>>> messages you get on the serial console are not recorded in the kernel
>>>>>>> log buffer, so they only appear if you actually look at the serial
>>>>>>> output.
>>>> As an aside, they probably should be recorded. That is probably a
>>>> question for the UEFI USWG. Grub and the ARMSTUB could probably bodge
>>>> something together, but that would be non-standard.
>>>>
>>>>>> Ah yeah. I suppose you could do it in the kernel later if you detect
>>>>>> you've booted through EFI with dtb= on the command line though.
>>>>>>
>>>>>>>> Once the stub and the boot method is there, it's hard to undo as we
>>>>>>>> can see here. Being loud and warn might be more useful, and set a
>>>>>>>> timeline for hard removal (12 months?).
>>>>>>>>
>>>>>>> The dtb= handling is still there, it is just not enabled by default.
>>>>>>> We can keep it around if people are still using it. But as I pointed
>>>>>>> out, we may decide to make new functionality available only if it is
>>>>>>> disabled, and at that point, we'll have to choose between one or the
>>>>>>> other in defconfig, which is annoying.
>>>>>>>
>>>>>>>> Scott; an alternative for you is to do a boot wrapper that bundles a
>>>>>>>> DT and kernel, and boot that instead of the kernel image (outside of
>>>>>>>> the kernel tree). Some 32-bit platforms from Marvell use that. That
>>>>>>>> way the kernel will just see it as a normally passed in DT.
>>>>>>>>
>>>>>>> Or use GRUB. It comes wired up in all the distros, and let's you load
>>>>>>> a DT binary from anywhere you can imagine, as opposed to the EFI stub
>>>>>>> which can only load it if it happens to reside in the same file system
>>>>>>> (or even directory - I can't remember) as the kernel image. Note that
>>>>>>> the same reservations apply to doing that - the firmware is no longer
>>>>>>> able to describe itself to the OS via the DT, which is really the only
>>>>>>> conduit it has available on an arm64 system..
>>>>>> So, I've looked at the history here a bit, and dtb= support was
>>>>>> introduced in 2014. Nowhere does it say that it isn't a recommended
>>>>>> way of booting.
>>>>>>
>>>>>> There are some firmware stacks today that modify and provide a
>>>>>> runtime-updated devicetree to the kernel, but there are also a bunch
>>>>>> who don't. Most "real" products will want a firmware that knows how to
>>>>>> pass in things such as firmware environment variables, or MAC
>>>>>> addresses, etc, to the kernel, but not all of them need it.
>>>>>>
>>>>>> In particular, in a world where you want EFI to be used on embedded
>>>>>> platforms, requiring another bootloader step such as GRUB to be able
>>>>>> to reasonably boot said platforms seems like a significant and
>>>>>> unfortunate new limitation. Documentation/efi-stub.txt has absolutely
>>>>>> no indication that it is a second-class option that isn't expected to
>>>>>> be available everywhere. It doesn't really matter what _your_
>>>>>> intention was around it, if those who use it never found out and now
>>>>>> rely on it.
>>>>>>
>>>>>> Unfortunately the way forward here is to revert 3d7ee348aa4127a.
>>> What's the path forward? Revert, defconfig change (this patch), or Kconfig
>>> default addition?
>> Revert or Kconfig select, and a Kconfig select means that the option
>> is a dead one anyway so we might as well revert.
>>
> I disagree. Making it default y is fine by me, but please don't remove it.
>
>> Ard, do you have other fixes lined up or should we take the patch
>> through arm-soc?
>>
> I don't have any fixes but either way is fine.
I submitted the version of the patch Ard requested here for somebody to
pick up.
https://lore.kernel.org/patchwork/patch/984521/
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH] arm64: defconfig: enable EFI_ARMSTUB_DTB_LOADER
2018-09-13 20:22 ` Scott Branden
@ 2018-09-14 6:06 ` Ard Biesheuvel
0 siblings, 0 replies; 25+ messages in thread
From: Ard Biesheuvel @ 2018-09-14 6:06 UTC (permalink / raw)
To: Scott Branden
Cc: Olof Johansson, Grant Likely, Catalin Marinas, Will Deacon,
Arnd Bergmann, Broadcom Kernel Feedback List, Linux ARM,
Linux Kernel Mailing List, Leif Lindholm, Alexander Graf
On 13 September 2018 at 22:22, Scott Branden <scott.branden@broadcom.com> wrote:
>
>
> On 18-09-10 11:08 AM, Ard Biesheuvel wrote:
>>
>> On 10 September 2018 at 20:01, Olof Johansson <olof@lixom.net> wrote:
>>>
>>> On Mon, Sep 10, 2018 at 10:53 AM, Scott Branden
>>> <scott.branden@broadcom.com> wrote:
>>>>
>>>> Olof/All,
>>>>
>>>>
>>>> On 18-09-04 03:13 AM, Grant Likely wrote:
>>>>>
>>>>> Hey folks. More comments below, but the short answer is I really don't
>>>>> see what the problem is. Distros cannot easily support platforms that
>>>>> require a dtb= parameter, and so they probably won't. They may or may
>>>>> not disable 'dtb=', depending on whether they see it as valuable for
>>>>> debug.
>>>>>
>>>>> Vertically integrated platforms are a different beast. We may strongly
>>>>> recommend firmware provides the dtb for all the mentioned good
>>>>> reasons, but they still get to decide their deployment methodology,
>>>>> and it is not burdensome for the kernel to keep the dtb= feature that
>>>>> they are using.
>>>>>
>>>>> On Tue, Sep 4, 2018 at 7:24 AM Ard Biesheuvel
>>>>> <ard.biesheuvel@linaro.org>
>>>>> wrote:
>>>>>>
>>>>>> On 2 September 2018 at 04:54, Olof Johansson <olof@lixom.net> wrote:
>>>>>>>
>>>>>>> On Thu, Aug 30, 2018 at 9:23 AM, Ard Biesheuvel
>>>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>>>>
>>>>>>>> On 30 August 2018 at 17:06, Olof Johansson <olof@lixom.net> wrote:
>>>>>>>>>
>>>>>>>>> On Wed, Aug 29, 2018 at 10:54 PM, Ard Biesheuvel
>>>>>>>>> <ard.biesheuvel@linaro.org> wrote:
>>>>>>>>>>
>>>>>>>>>> On 29 August 2018 at 20:59, Scott Branden
>>>>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Olof,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 18-08-29 11:44 AM, Olof Johansson wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On Wed, Aug 29, 2018 at 10:21 AM, Scott Branden
>>>>>>>>>>>> <scott.branden@broadcom.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Enable EFI_ARMSTUB_DTB_LOADER to add support for the dtb=
>>>>>>>>>>>>> command
>>>>>>>>>>>>> line
>>>>>>>>>>>>> parameter to function with efi loader.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Required to boot on existing bootloaders that do not support
>>>>>>>>>>>>> devicetree
>>>>>>>>>>>>> provided by the platform or by the bootloader.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Fixes: 3d7ee348aa41 ("efi/libstub/arm: Add opt-in Kconfig
>>>>>>>>>>>>> option
>>>>>>>>>>>>> for the
>>>>>>>>>>>>> DTB loader")
>>>>>>>>>>>>> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Why did Ard create an option for this if it's just going be
>>>>>>>>>>>> turned
>>>>>>>>>>>> on
>>>>>>>>>>>> in default configs? Doesn't make sense to me.
>>>>>>>>>>>>
>>>>>>>>>>>> It would help to know what firmware still is crippled and how
>>>>>>>>>>>> common
>>>>>>>>>>>> it is, since it's been a few years that this has been a
>>>>>>>>>>>> requirement
>>>>>>>>>>>> by
>>>>>>>>>>>> now.
>>>>>>>>>>>
>>>>>>>>>>> Broadcom NS2 and Stingray in current development and production
>>>>>>>>>>> need
>>>>>>>>>>> this
>>>>>>>>>>> option in the kernel enabled in order to boot.
>>>>>>>>>>
>>>>>>>>>> And these production systems run mainline kernels in a defconfig
>>>>>>>>>> configuration?
>>>>>>>>>>
>>>>>>>>>> The simply reality is that the DTB loader has been deprecated for
>>>>>>>>>> a
>>>>>>>>>> good reason: it was only ever intended as a development hack
>>>>>>>>>> anyway,
>>>>>>>>>> and if we need to treat the EFI stub provided DTB as a first class
>>>>>>>>>> citizen, there are things we need to fix to make things works as
>>>>>>>>>> expected. For instance, GRUB will put a property in the /chosen
>>>>>>>>>> node
>>>>>>>>>> for the initramfs which will get dropped if you boot with dtb=.
>>>>>>>>>>
>>>>>>>>>> Don't be surprised if some future enhancements of the EFI stub
>>>>>>>>>> code
>>>>>>>>>> depend on !EFI_ARMSTUB_DTB_LOADER.
>>>>>
>>>>> That's an odd statement to make. The DTB loader code is well contained
>>>>> and with defined semantics... True, the semantics are "I DON'T BELIEVE
>>>>> FIRMWARE", but it is still well defined. What scenario are you
>>>>> envisioning where EFI_ARMSTUB_DTB_LOADER would be explicitly excluded?
>>>>>
>>>>> Conversely, the dtb= argument is an invaluable debug tool during
>>>>> development. As Olof has already said, there are a lot of embedded
>>>>> deployments where there is no desire for grub or any other
>>>>> intermediary loader.
>>>>>
>>>>>>>>>> On UEFI systems, DTBs [or ACPI
>>>>>>>>>> tables] are used by the firmware to describe itself and the
>>>>>>>>>> underlying
>>>>>>>>>> platform to the OS, and the practice of booting with DTB file
>>>>>>>>>> images
>>>>>>>>>> (taken from the kernel build as well) conflicts with that view.
>>>>>>>>>> Note
>>>>>>>>>> that GRUB still permits you to load DTBs from files (and supports
>>>>>>>>>> more
>>>>>>>>>> sources than just the file system the kernel Image was loaded
>>>>>>>>>> from).
>>>>>>>>>
>>>>>>>>> Ard,
>>>>>>>>>
>>>>>>>>> Maybe a WARN() splat would be more useful as a phasing-out method
>>>>>>>>> than
>>>>>>>>> removing functionality for them that needs to be reinstated through
>>>>>>>>> changing the config?
>>>>>>>>>
>>>>>>>> We don't have any of that in the stub, and inventing new ways to
>>>>>>>> pass
>>>>>>>> such information between the stub and the kernel proper seems like a
>>>>>>>> cart-before-horse kind of thing to me. The EFI stub diagnostic
>>>>>>>> messages you get on the serial console are not recorded in the
>>>>>>>> kernel
>>>>>>>> log buffer, so they only appear if you actually look at the serial
>>>>>>>> output.
>>>>>
>>>>> As an aside, they probably should be recorded. That is probably a
>>>>> question for the UEFI USWG. Grub and the ARMSTUB could probably bodge
>>>>> something together, but that would be non-standard.
>>>>>
>>>>>>> Ah yeah. I suppose you could do it in the kernel later if you detect
>>>>>>> you've booted through EFI with dtb= on the command line though.
>>>>>>>
>>>>>>>>> Once the stub and the boot method is there, it's hard to undo as we
>>>>>>>>> can see here. Being loud and warn might be more useful, and set a
>>>>>>>>> timeline for hard removal (12 months?).
>>>>>>>>>
>>>>>>>> The dtb= handling is still there, it is just not enabled by default.
>>>>>>>> We can keep it around if people are still using it. But as I pointed
>>>>>>>> out, we may decide to make new functionality available only if it is
>>>>>>>> disabled, and at that point, we'll have to choose between one or the
>>>>>>>> other in defconfig, which is annoying.
>>>>>>>>
>>>>>>>>> Scott; an alternative for you is to do a boot wrapper that bundles
>>>>>>>>> a
>>>>>>>>> DT and kernel, and boot that instead of the kernel image (outside
>>>>>>>>> of
>>>>>>>>> the kernel tree). Some 32-bit platforms from Marvell use that. That
>>>>>>>>> way the kernel will just see it as a normally passed in DT.
>>>>>>>>>
>>>>>>>> Or use GRUB. It comes wired up in all the distros, and let's you
>>>>>>>> load
>>>>>>>> a DT binary from anywhere you can imagine, as opposed to the EFI
>>>>>>>> stub
>>>>>>>> which can only load it if it happens to reside in the same file
>>>>>>>> system
>>>>>>>> (or even directory - I can't remember) as the kernel image. Note
>>>>>>>> that
>>>>>>>> the same reservations apply to doing that - the firmware is no
>>>>>>>> longer
>>>>>>>> able to describe itself to the OS via the DT, which is really the
>>>>>>>> only
>>>>>>>> conduit it has available on an arm64 system..
>>>>>>>
>>>>>>> So, I've looked at the history here a bit, and dtb= support was
>>>>>>> introduced in 2014. Nowhere does it say that it isn't a recommended
>>>>>>> way of booting.
>>>>>>>
>>>>>>> There are some firmware stacks today that modify and provide a
>>>>>>> runtime-updated devicetree to the kernel, but there are also a bunch
>>>>>>> who don't. Most "real" products will want a firmware that knows how
>>>>>>> to
>>>>>>> pass in things such as firmware environment variables, or MAC
>>>>>>> addresses, etc, to the kernel, but not all of them need it.
>>>>>>>
>>>>>>> In particular, in a world where you want EFI to be used on embedded
>>>>>>> platforms, requiring another bootloader step such as GRUB to be able
>>>>>>> to reasonably boot said platforms seems like a significant and
>>>>>>> unfortunate new limitation. Documentation/efi-stub.txt has absolutely
>>>>>>> no indication that it is a second-class option that isn't expected to
>>>>>>> be available everywhere. It doesn't really matter what _your_
>>>>>>> intention was around it, if those who use it never found out and now
>>>>>>> rely on it.
>>>>>>>
>>>>>>> Unfortunately the way forward here is to revert 3d7ee348aa4127a.
>>>>
>>>> What's the path forward? Revert, defconfig change (this patch), or
>>>> Kconfig
>>>> default addition?
>>>
>>> Revert or Kconfig select, and a Kconfig select means that the option
>>> is a dead one anyway so we might as well revert.
>>>
>> I disagree. Making it default y is fine by me, but please don't remove it.
>>
>>> Ard, do you have other fixes lined up or should we take the patch
>>> through arm-soc?
>>>
>> I don't have any fixes but either way is fine.
>
> I submitted the version of the patch Ard requested here for somebody to pick
> up.
> https://lore.kernel.org/patchwork/patch/984521/
>
Thanks Scott. I will pick it up as a EFI fix.
^ permalink raw reply [flat|nested] 25+ messages in thread