Re: Passing boot logs to Linux?
Hi, On 03.01.24 13:44, Heinrich Schuchardt wrote: > On 03.01.24 11:11, Csókás Bence wrote: >> Hi, >> >> 2023. 12. 31. 0:54 keltezéssel, Heinrich Schuchardt írta: >>> On 12/20/23 05:11, Simon Glass wrote: +Heinrich Schuchardt On Tue, 19 Dec 2023 at 13:33, Daniel Golle wrote: > > Hi Bence, > > On Tue, Dec 19, 2023 at 08:08:48PM +, Csókás Bence wrote: >> Hi! >> >> Is passing the U-Boot boot log to Linux supported yet? We are working >> with a third-party solution, which works, but is a bit hacky, so I was >> wondering if an official solution has been merged yet. >>> >>> U-Boot supports writing log messages to an rsyslog server via broadcast >>> to UDP port 514 (CONFIG_LOG_SYSLOG=y). >>> >>> The device name can be set via environment variable log_hostname. >>> >>> If you have a log server for your devices in the same network segment, >>> you can consolidate all your logs there. >>> >>> A driver writing log messages to a file could be created in U-Boot. >> >> I am aware, however, this is not what we want to do right now. We want >> to pass the logs to the booted up OS on the *same* machine. >> >> I saw that there was an option CONFIG_CONSOLE_RECORD that saves >> everything to a membuff, but I don't know if that can be exported to >> Linux yet. And if not in the tree yet, would such a patch be welcome? >>> >>> It is not enough to export a memory buffer. There must be support in the >>> operating system to read it. >> >> Of course. But it should be trivial to add a driver to Linux and other >> FOSS OSes to support reading these records from memory. > > Please, have a look at Linux' > > Documentation/admin-guide/ramoops.rst > Documentation/devicetree/bindings/reserved-memory/ramoops.yaml > > If U-Boot had a log driver writing to the ramoops buffer we would not > need any change in Linux. > > The current U-Boot drivers are > > common/log_syslog.c > common/log_console.c Yes, that's what I would suggest as well. For reference, barebox supports pstore with ramoops backend to share its dmesg buffer with Linux. You can take a look there for pointers. The code may not directly portable to U-Boot though barebox implements pstore as a file system. The problem of interactive console I/O filling up the log is resolved by having only logged output (pr_info, dev_info and friends) output to pstore and plain printf only prints to enabled serial consoles. Cheers, Ahmad > > Best regards > > Heinrich > >> >>> Linux supports the common platform error record (CPER) format defined in >>> the UEFI specification. See >>> >>> * https://www.dmtf.org/sites/default/files/PMCI_CPEREvent_Proposal_v3.pdf >> >> According to page 16, records can be of the following types: >> * error, recovered >> * previous error >> * simulated error >> >> There is no type that is an operational condition (even though you can >> append informational sections to an error record). So what we could do >> is mark everything as "recovered", but that is misleading, booting is >> not an "error condition". >> >> > * >> https://uefi.org/sites/default/files/resources/Spike%20Yuan-%20Server%20RAS%20and%20UEFI%20CPER_final.pdf >> >> This looks like some promo material, not very useful from an >> implementation standpoint. Plus, this talks about Intel-based cloud >> computing/HPC, not exactly the embedded systems we are targeting. >> >>> Boot errors can be reported via the ACPI BERT table. But that won't help >>> you in OpenWRT which uses device-trees. >> >> Exactly. I would avoid depending on ACPI/UEFI. Having a simple in-memory >> struct can be supported on all platforms, even without ACPI, UEFI, >> writable filesystem, even DT is not needed if you can pass the base >> pointer to the OS via other means (ie. command line). >> >>> Best regards >>> >>> Heinrich >> >> Bence >> > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v9 2/2] arm64: boot: Support Flat Image Tree
Hello Yamada-san,
On 14.12.23 08:33, Masahiro Yamada wrote:
>> The FIT spec allows the "fdt" property to list
>> multiple image nodes.
>>
>>
>> o config-1
>> |- description = "configuration description"
>> |- kernel = "kernel sub-node unit name"
>> |- fdt = "fdt sub-node unit-name" [, "fdt overlay sub-node unit-name", ...]
>> |- loadables = "loadables sub-node unit-name"
>> |- script = "
>> |- compatible = "vendor
>
>
>
>
>
> This is a question for U-Boot (and barebox).
>
>
>
>
>images {
> base {
> ...
> };
>
> addon1 {
> ...
> };
>
> addon2 {
> ...
> };
> };
>
> configurations {
> ...
> fdt = "base", "addon1", "addon2";
> };
>
>
>
>
> Is U-Boot's "bootm" command able to dynamically construct
> the full DTB from "base" + "addon1" + "addon2"
> and pass to the kernel?
barebox can apply overlays to the DT, but doesn't do so yet from
the extra entries in configuration fdt properties.
This should be straight-forward to add though, if the need arises.
> Is U-Boot able to handle FIT (includes kernel + DTs)
> and a separate initrd?
>
> # bootm :
This is possible in barebox, provided that the FIT image doesn't
already have a ramdisk and that CONFIG_BOOTM_FORCE_SIGNED_IMAGES=n:
bootm -r /mnt/nfs/ramdisk.gz /mnt/nfs/image.fit
(Or the equivalent variables if not wanting to use the shell.)
> Presumably, it would be difficult to inject initramdisk
> into image.fit later, so I am hoping bootm would work like that,
> but I did not delve into U-Boot code.
>
>
>
> If it works, is it possible to verify the integrity of initrd?
> The kernel and DTs inside FIT will be verified, but not sure
> if it is possible for ramdisk.
If one wants to preclude mix & match attacks, the configuration needs
to be verified fully, so if signing is required, it's probably better to
amend the FIT later on with the new configuration instead of signing
the initrd separately and combining them at runtime.
Cheers,
Ahmad
>
>
>
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v9 2/2] arm64: boot: Support Flat Image Tree
Hello Simon,
On 02.12.23 04:54, Simon Glass wrote:
> Add a script which produces a Flat Image Tree (FIT), a single file
> containing the built kernel and associated devicetree files.
> Compression defaults to gzip which gives a good balance of size and
> performance.
>
> The files compress from about 86MB to 24MB using this approach.
>
> The FIT can be used by bootloaders which support it, such as U-Boot
> and Linuxboot. It permits automatic selection of the correct
> devicetree, matching the compatible string of the running board with
> the closest compatible string in the FIT. There is no need for
> filenames or other workarounds.
>
> Add a 'make image.fit' build target for arm64, as well. Use
> FIT_COMPRESSION to select a different algorithm.
>
> The FIT can be examined using 'dumpimage -l'.
>
> This features requires pylibfdt (use 'pip install libfdt'). It also
> requires compression utilities for the algorithm being used. Supported
> compression options are the same as the Image.xxx files. For now there
> is no way to change the compression other than by editing the rule for
> $(obj)/image.fit
>
> While FIT supports a ramdisk / initrd, no attempt is made to support
> this here, since it must be built separately from the Linux build.
>
> Signed-off-by: Simon Glass
kernel_noload support is now in barebox next branch and I tested this
series against it:
Tested-by: Ahmad Fatoum # barebox
> +"""Build a FIT containing a lot of devicetree files
> +
> +Usage:
> +make_fit.py -A arm64 -n 'Linux-6.6' -O linux
> +-f arch/arm64/boot/image.fit -k /tmp/kern/arch/arm64/boot/image.itk
> +/tmp/kern/arch/arm64/boot/dts/ -E -c gzip
> +
> +Creates a FIT containing the supplied kernel and a directory containing the
> +devicetree files.
> +
> +Use -E to generate an external FIT (where the data is placed after the
> +FIT data structure). This allows parsing of the data without loading
> +the entire FIT.
> +
> +Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
> +zstd algorithms.
> +
> +The resulting FIT can be booted by bootloaders which support FIT, such
> +as U-Boot, Linuxboot, Tianocore, etc.
Feel free to add barebox to the list. Did you check whether Linuxboot and
Tianocore support kernel_noload?
> +fsw.property_u32('load', 0)
> +fsw.property_u32('entry', 0)
I still think load and entry dummy values are confusing and should be dropped.
> +with fsw.add_node(f'fdt-{seq}'):
> +# Get the compatible / model information
> +with open(fname, 'rb') as inf:
> +data = inf.read()
> +fdt = libfdt.FdtRo(data)
> +model = fdt.getprop(0, 'model').as_str()
> +compat = fdt.getprop(0, 'compatible')
> +
> +fsw.property_string('description', model)
> +fsw.property_string('type', 'flat_dt')
> +fsw.property_string('arch', arch)
> +fsw.property_string('compression', compress)
> +fsw.property('compatible', bytes(compat))
> +
> +with open(fname, 'rb') as inf:
> +compressed = compress_data(inf, compress)
> +fsw.property('data', compressed)
> +return model, compat
After Doug's elaboration, extracting multiple compatibles is fine by me.
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree
Hello, On 04.12.23 18:52, Doug Anderson wrote:> On Sat, Dec 2, 2023 at 8:37 AM Simon Glass wrote: >> On Thu, 30 Nov 2023 at 19:04, Ahmad Fatoum wrote: >>> On 30.11.23 21:30, Simon Glass wrote: >>>> On Wed, 29 Nov 2023 at 12:54, Ahmad Fatoum wrote: >>>>> On 29.11.23 20:44, Simon Glass wrote: >>>> I don't have an example to hand, but this is the required mechanism of >>>> FIT. This feature has been in place for many years and is used by >>>> ChromeOS, at least. >>> >>> I see the utility of a FIT configuration with >>> >>> compatible = "vendor,board-rev-a", "vendor,board-rev-b"; >>> >>> I fail to see a utility for a configuration with >>> >>> compatible = "vendor,board", "vendor,SoM", "vendor,SoC"; >>> >>> Any configuration that ends up being booted because "vendor,SoC" was >>> matched is >>> most likely doomed to fail. Therefore, I would suggest that only the top >>> level >>> configuration is written into the FIT configurations automatically. >> >> Firstly, I am not an expert on this. >> >> Say you have a board with variants. The compatible string in U-Boot >> may be something like: >> >> "google,veyron-brain-rev1", "google,veyron-brain", "google,veyron", >> "rockchip,rk3288"; >> >> If you then have several FIT configurations, they may be something like: >> >> "google,veyron-brain-rev0", "google,veyron-brain", "google,veyron", >> "rockchip,rk3288"; >> "google,veyron-brain-rev1", "google,veyron-brain", "google,veyron", >> "rockchip,rk3288"; >> "google,veyron-brain-rev2", "google,veyron-brain", "google,veyron", >> "rockchip,rk3288"; >> >> You want to choose the second one, since it is a better match than the >> others. Now imagine, you are building a kernel that has no DT support for the Veyron, but instead has support for the Phytec RK3288 PCM-947: phytec,rk3288-pcm-947", "phytec,rk3288-phycore-som", "rockchip,rk3288 As far as I understand U-Boot code, A veyron U-Boot would boot the Phytec DT if CONFIG_FIT_BEST_MATCH is set, although it's a bad match and a boot failure should rather have occurred. >> +Doug Anderson who knows a lot more about this than me. > > Hopefully this is all explained by: > > https://docs.kernel.org/arch/arm/google/chromebook-boot-flow.html Thanks Doug, this was helpful. The missing information to me was that depthcharge only compares the top-level board compatible in addition to runtime generated board compatibles, unlike what U-Boot seems to do. barebox only compares its top-level compatible against the FIT configuration compatibles, so adding a full compatible list to the configuration nodes as done by this series should be ok there as well. I think U-Boot could run into issues though as described above. Out of curiosity: I only heard about Depthcharge before as exploitation toolkit for U-Boot. Can you point me at some documentation on what the Depthcharge bootloader does what U-Boot (which was presumably used before?) doesn't? Thanks, Ahmad > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree
Hello Simon, On 30.11.23 21:30, Simon Glass wrote: > On Wed, 29 Nov 2023 at 12:54, Ahmad Fatoum wrote: >> On 29.11.23 20:44, Simon Glass wrote: >>> On Wed, 29 Nov 2023 at 12:33, Ahmad Fatoum wrote: >>>> >>>> On 29.11.23 20:27, Simon Glass wrote: >>>>> On Wed, 29 Nov 2023 at 12:15, Ahmad Fatoum >>>>> wrote: >>>>>> On 29.11.23 20:02, Simon Glass wrote: >>>>>>> On Wed, 29 Nov 2023 at 11:59, Ahmad Fatoum >>>>>>> wrote: >>>>>>>> The specification says that this is the root U-Boot compatible, >>>>>>>> which I presume to mean the top-level compatible, which makes sense to >>>>>>>> me. >>>>>>>> >>>>>>>> The code here though adds all compatible strings from the device tree >>>>>>>> though, >>>>>>>> is this intended? >>>>>>> >>>>>>> Yes, since it saves needing to read in each DT just to get the >>>>>>> compatible stringlist. >>>>>> >>>>>> The spec reads as if only one string (root) is supposed to be in the >>>>>> list. >>>>>> The script adds all compatibles though. This is not really useful as a >>>>>> bootloader >>>>>> that's compatible with e.g. fsl,imx8mm would just take the first device >>>>>> tree >>>>>> with that SoC, which is most likely to be wrong. It would be better to >>>>>> just >>>>>> specify the top-level compatible, so the bootloader fails instead of >>>>>> taking >>>>>> the first DT it finds. >>>>> >>>>> We do need to have a list, since we have to support different board revs, >>>>> etc. >>>> >>>> Can you give me an example? The way I see it, a bootloader with >>>> compatible "vendor,board" and a FIT with configuration with compatibles: >>>> >>>> "vendor,board-rev-a", "vendor,board" >>>> "vendor,board-rev-b", "vendor,board" >>>> >>>> would just result in the bootloader booting the first configuration, even >>>> if >>>> the device is actually rev-b. >>> >>> You need to find the best match, not just any match. This is >>> documented in the function comment for fit_conf_find_compat(). >> >> In my above example, both configuration are equally good. >> Can you give me an example where it makes sense to have multiple >> compatibles automatically extracted from the device tree compatible? >> >> The way I see it having more than one compatible here just has >> downsides. > > I don't have an example to hand, but this is the required mechanism of > FIT. This feature has been in place for many years and is used by > ChromeOS, at least. I see the utility of a FIT configuration with compatible = "vendor,board-rev-a", "vendor,board-rev-b"; I fail to see a utility for a configuration with compatible = "vendor,board", "vendor,SoM", "vendor,SoC"; Any configuration that ends up being booted because "vendor,SoC" was matched is most likely doomed to fail. Therefore, I would suggest that only the top level configuration is written into the FIT configurations automatically. Cheers, Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree
Hi Simon, On 29.11.23 20:00, Simon Glass wrote: > On Wed, 29 Nov 2023 at 11:35, Ahmad Fatoum wrote: >> Doesn't hardcoding a load address and entry address here defeat the point >> of having FIT as generic portable image format? >> >> At least barebox will try to place the kernel image at physical address 0 and >> will exit with an error message if no SDRAM is located at that address. >> The recommendation in that case is to omit load and entry address altogether >> to have barebox find a suitable location, but I see now that the FIT >> specification >> requires a load and entry address. What would happen if U-Boot tries to load >> this >> FIT image on a board that has no DRAM at address 0? > > The 'kernel_noload' type indicates that the load/exec address are ignored. Can the script not insert load/exec addresses with dummy values to avoid confusion? >> Please Cc me on subsequent revisions. I am interested in testing that this >> works for barebox >> too. > > There has been some discussion about this recently in U-Boot too, > along with a series [1] which you could try if you like. Thanks for the pointer. I have just sent out a first patch to add support for kernel_noload to the barebox mailing list[1]. With that change applied, barebox can boot the FIT images generated by this series. Once that's accepted, I'll reply with a Tested-by. [1]: https://lore.barebox.org/barebox/20231129203106.2417486-1-a.fat...@pengutronix.de/T/#u > The FIT spec[2] does not provide enough detail on exactly what > kernel_noload means and we should improve this at some point. Yes, that would be nice. Also straight references to e.g. U-Boot configuration symbols could use some rewording. Thanks, Ahmad > > Regards, > Simon > > [1] https://patchwork.ozlabs.org/project/uboot/list/?series=382849 > [2] https://github.com/open-source-firmware/flat-image-tree > > > >> >> Thanks, >> Ahmad >> >> -- >> Pengutronix e.K. | | >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | >> > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree
Hello Simon, On 29.11.23 20:44, Simon Glass wrote: > Hi Ahmad, > > On Wed, 29 Nov 2023 at 12:33, Ahmad Fatoum wrote: >> >> On 29.11.23 20:27, Simon Glass wrote: >>> On Wed, 29 Nov 2023 at 12:15, Ahmad Fatoum wrote: >>>> On 29.11.23 20:02, Simon Glass wrote: >>>>> On Wed, 29 Nov 2023 at 11:59, Ahmad Fatoum >>>>> wrote: >>>>>> The specification says that this is the root U-Boot compatible, >>>>>> which I presume to mean the top-level compatible, which makes sense to >>>>>> me. >>>>>> >>>>>> The code here though adds all compatible strings from the device tree >>>>>> though, >>>>>> is this intended? >>>>> >>>>> Yes, since it saves needing to read in each DT just to get the >>>>> compatible stringlist. >>>> >>>> The spec reads as if only one string (root) is supposed to be in the list. >>>> The script adds all compatibles though. This is not really useful as a >>>> bootloader >>>> that's compatible with e.g. fsl,imx8mm would just take the first device >>>> tree >>>> with that SoC, which is most likely to be wrong. It would be better to just >>>> specify the top-level compatible, so the bootloader fails instead of taking >>>> the first DT it finds. >>> >>> We do need to have a list, since we have to support different board revs, >>> etc. >> >> Can you give me an example? The way I see it, a bootloader with >> compatible "vendor,board" and a FIT with configuration with compatibles: >> >> "vendor,board-rev-a", "vendor,board" >> "vendor,board-rev-b", "vendor,board" >> >> would just result in the bootloader booting the first configuration, even if >> the device is actually rev-b. > > You need to find the best match, not just any match. This is > documented in the function comment for fit_conf_find_compat(). In my above example, both configuration are equally good. Can you give me an example where it makes sense to have multiple compatibles automatically extracted from the device tree compatible? The way I see it having more than one compatible here just has downsides. >> The configuration already has a compatible entry. What extra use is the >> compatible >> entry in the FDT node? > > It allows seeing the compatible stringlist without having to read the > FDT itself. I don't believe it is necessary though, so long as we are > scanning the configurations and not the FDT nodes. I think it's better to drop this if it has no use. Cheers, Ahmad > > Regards, > Simon > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree
On 29.11.23 20:27, Simon Glass wrote:
> On Wed, 29 Nov 2023 at 12:15, Ahmad Fatoum wrote:
>> On 29.11.23 20:02, Simon Glass wrote:
>>> On Wed, 29 Nov 2023 at 11:59, Ahmad Fatoum wrote:
>>>> The specification says that this is the root U-Boot compatible,
>>>> which I presume to mean the top-level compatible, which makes sense to me.
>>>>
>>>> The code here though adds all compatible strings from the device tree
>>>> though,
>>>> is this intended?
>>>
>>> Yes, since it saves needing to read in each DT just to get the
>>> compatible stringlist.
>>
>> The spec reads as if only one string (root) is supposed to be in the list.
>> The script adds all compatibles though. This is not really useful as a
>> bootloader
>> that's compatible with e.g. fsl,imx8mm would just take the first device tree
>> with that SoC, which is most likely to be wrong. It would be better to just
>> specify the top-level compatible, so the bootloader fails instead of taking
>> the first DT it finds.
>
> We do need to have a list, since we have to support different board revs, etc.
Can you give me an example? The way I see it, a bootloader with
compatible "vendor,board" and a FIT with configuration with compatibles:
"vendor,board-rev-a", "vendor,board"
"vendor,board-rev-b", "vendor,board"
would just result in the bootloader booting the first configuration, even if
the device is actually rev-b.
>>>>> +fsw.property_string('description', model)
>>>>> +fsw.property_string('type', 'flat_dt')
>>>>> +fsw.property_string('arch', arch)
>>>>> +fsw.property_string('compression', compress)
>>>>> +fsw.property('compatible', bytes(compat))
>>>>
>>>> I think I've never seen a compatible for a fdt node before.
>>>> What use does this serve?
>>>
>>> It indicates the machine that the DT is for.
>>
>> Who makes use of this information?
>
> U-Boot uses it, I believe. There is an optimisation to use this
> instead of reading the DT itself.
The configuration already has a compatible entry. What extra use is the
compatible
entry in the FDT node?
Thanks,
Ahmad
>
> Regards,
> Simon
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree
Hello Tom,
On 29.11.23 20:02, Tom Rini wrote:
> On Wed, Nov 29, 2023 at 07:59:00PM +0100, Ahmad Fatoum wrote:
>> Hi,
>>
>> a few more comments after decompiling the FIT image:
>>
>> On 29.11.23 18:21, Simon Glass wrote:
>>> +with fsw.add_node('kernel'):
>>> +fsw.property_string('description', args.name)
>>> +fsw.property_string('type', 'kernel_noload')
>>
>> The specification only says no loading done, but doesn't explain what it
>> means for a bootloader to _not_ load an image. Looking into the U-Boot commit
>> b9b50e89d317 ("image: Implement IH_TYPE_KERNEL_NOLOAD") that introduces this,
>> apparently no loading means ignoring load and entry address?
>>
>> I presume missing load and entry is something older U-Boot versions
>> were unhappy about? Please let me know if the semantics are as I understood,
>> so I can prepare a barebox patch supporting it.
>
> So the matching side for this series in U-Boot is:
> https://patchwork.ozlabs.org/project/uboot/list/?series=382849=*
>
> And in short, for IH_TYPE_KERNEL_NOLOAD we do our best to use it
> in-place. For decompression we allocate some space to decompress to.
Thanks. I am still curious why "kernel" couldn't have been used back then
with missing entry and load address to arrive at the same result?
Thanks,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree
Hello Simon,
On 29.11.23 20:02, Simon Glass wrote:
> Hi Ahmad,
>
> On Wed, 29 Nov 2023 at 11:59, Ahmad Fatoum wrote:
>>
>> Hi,
>>
>> a few more comments after decompiling the FIT image:
>>
>> On 29.11.23 18:21, Simon Glass wrote:
>>> +with fsw.add_node('kernel'):
>>> +fsw.property_string('description', args.name)
>>> +fsw.property_string('type', 'kernel_noload')
>>
>> The specification only says no loading done, but doesn't explain what it
>> means for a bootloader to _not_ load an image. Looking into the U-Boot commit
>> b9b50e89d317 ("image: Implement IH_TYPE_KERNEL_NOLOAD") that introduces this,
>> apparently no loading means ignoring load and entry address?
>>
>> I presume missing load and entry is something older U-Boot versions
>> were unhappy about? Please let me know if the semantics are as I understood,
>> so I can prepare a barebox patch supporting it.
>
> Oh, see my previous email.
Thanks.
>
>>
>>> +fsw.property_string('arch', args.arch)
>>> +fsw.property_string('os', args.os)
>>> +fsw.property_string('compression', args.compress)
>>> +fsw.property('data', data)
>>> +fsw.property_u32('load', 0)
>>> +fsw.property_u32('entry', 0)
>>> +
>>> +
>>> +def finish_fit(fsw, entries):
>>> +"""Finish the FIT ready for use
>>> +
>>> +Writes the /configurations node and subnodes
>>> +
>>> +Args:
>>> +fsw (libfdt.FdtSw): Object to use for writing
>>> +entries (list of tuple): List of configurations:
>>> +str: Description of model
>>> +str: Compatible stringlist
>>> +"""
>>> +fsw.end_node()
>>> +seq = 0
>>> +with fsw.add_node('configurations'):
>>> +for model, compat in entries:
>>> +seq += 1
>>> +with fsw.add_node(f'conf-{seq}'):
>>> +fsw.property('compatible', bytes(compat))
>>
>> The specification says that this is the root U-Boot compatible,
>> which I presume to mean the top-level compatible, which makes sense to me.
>>
>> The code here though adds all compatible strings from the device tree though,
>> is this intended?
>
> Yes, since it saves needing to read in each DT just to get the
> compatible stringlist.
The spec reads as if only one string (root) is supposed to be in the list.
The script adds all compatibles though. This is not really useful as a
bootloader
that's compatible with e.g. fsl,imx8mm would just take the first device tree
with that SoC, which is most likely to be wrong. It would be better to just
specify the top-level compatible, so the bootloader fails instead of taking
the first DT it finds.
>>> +fsw.property_string('description', model)
>>> +fsw.property_string('type', 'flat_dt')
>>> +fsw.property_string('arch', arch)
>>> +fsw.property_string('compression', compress)
>>> +fsw.property('compatible', bytes(compat))
>>
>> I think I've never seen a compatible for a fdt node before.
>> What use does this serve?
>
> It indicates the machine that the DT is for.
Who makes use of this information?
Thanks,
Ahmad
>
> Regards,
> Simon
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree
Hi,
a few more comments after decompiling the FIT image:
On 29.11.23 18:21, Simon Glass wrote:
> +with fsw.add_node('kernel'):
> +fsw.property_string('description', args.name)
> +fsw.property_string('type', 'kernel_noload')
The specification only says no loading done, but doesn't explain what it
means for a bootloader to _not_ load an image. Looking into the U-Boot commit
b9b50e89d317 ("image: Implement IH_TYPE_KERNEL_NOLOAD") that introduces this,
apparently no loading means ignoring load and entry address?
I presume missing load and entry is something older U-Boot versions
were unhappy about? Please let me know if the semantics are as I understood,
so I can prepare a barebox patch supporting it.
> +fsw.property_string('arch', args.arch)
> +fsw.property_string('os', args.os)
> +fsw.property_string('compression', args.compress)
> +fsw.property('data', data)
> +fsw.property_u32('load', 0)
> +fsw.property_u32('entry', 0)
> +
> +
> +def finish_fit(fsw, entries):
> +"""Finish the FIT ready for use
> +
> +Writes the /configurations node and subnodes
> +
> +Args:
> +fsw (libfdt.FdtSw): Object to use for writing
> +entries (list of tuple): List of configurations:
> +str: Description of model
> +str: Compatible stringlist
> +"""
> +fsw.end_node()
> +seq = 0
> +with fsw.add_node('configurations'):
> +for model, compat in entries:
> +seq += 1
> +with fsw.add_node(f'conf-{seq}'):
> +fsw.property('compatible', bytes(compat))
The specification says that this is the root U-Boot compatible,
which I presume to mean the top-level compatible, which makes sense to me.
The code here though adds all compatible strings from the device tree though,
is this intended?
> +fsw.property_string('description', model)
> +fsw.property_string('type', 'flat_dt')
> +fsw.property_string('arch', arch)
> +fsw.property_string('compression', compress)
> +fsw.property('compatible', bytes(compat))
I think I've never seen a compatible for a fdt node before.
What use does this serve?
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree
Hello Simon, On 29.11.23 18:21, Simon Glass wrote: > Add a script which produces a Flat Image Tree (FIT), a single file > containing the built kernel and associated devicetree files. > Compression defaults to gzip which gives a good balance of size and > performance. Thanks for working on this. I think it's useful to have the kernel generate a FIT image out of the box. More complex use cases are always free to call mkimage with a custom ITS. > The files compress from about 86MB to 24MB using this approach. > > The FIT can be used by bootloaders which support it, such as U-Boot > and Linuxboot. It permits automatic selection of the correct > devicetree, matching the compatible string of the running board with > the closest compatible string in the FIT. There is no need for > filenames or other workarounds. > > Add a 'make image.fit' build target for arm64, as well. not that it matters much, but should this maybe called Image.fit as the other Image types are capitalized too? > EFI_ZBOOT_PAYLOAD:= Image > EFI_ZBOOT_BFD_TARGET := elf64-littleaarch64 > EFI_ZBOOT_MACH_TYPE := ARM64 > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index 1a965fe68e01..e1c06ca3c847 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -496,6 +496,19 @@ quiet_cmd_uimage = UIMAGE $@ > -a $(UIMAGE_LOADADDR) -e $(UIMAGE_ENTRYADDR) \ > -n '$(UIMAGE_NAME)' -d $< $@ Doesn't hardcoding a load address and entry address here defeat the point of having FIT as generic portable image format? At least barebox will try to place the kernel image at physical address 0 and will exit with an error message if no SDRAM is located at that address. The recommendation in that case is to omit load and entry address altogether to have barebox find a suitable location, but I see now that the FIT specification requires a load and entry address. What would happen if U-Boot tries to load this FIT image on a board that has no DRAM at address 0? Please Cc me on subsequent revisions. I am interested in testing that this works for barebox too. Thanks, Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 1/1] virtio: add driver for virtio_console devices
Hello,
On 06.06.23 15:25, Ying-Chun Liu (PaulLiu) wrote:
> From: "A. Cody Schuffelen"
>
> This is an implementation of single-character virtio-console. Part of the
> patch is based on barebox implementations.
Cool to see others found this useful. I had originally written this for the demo
at https://barebox.org/jsbarebox/ :-)
Back then I had trouble getting more than one console to work, whether through
multiport consoles or via having one virtio device per console. The original
barebox source code has a comment about what I tried. Did you manage to resolve
this limitation?
If not, you should add some check into your patch to prevent registering more
than
one console with the driver.
Cheers,
Ahmad
>
> Signed-off-by: A. Cody Schuffelen
> [ Paul: pick from the Android tree. Rebase to the upstream. Small fixes. ]
> Signed-off-by: Ying-Chun Liu (PaulLiu)
> Link:
> https://android.googlesource.com/platform/external/u-boot/+/bc68da98ebf835a50d869cf01c535ac936bfcb11
> Link:
> https://git.pengutronix.de/cgit/barebox/tree/drivers/serial/virtio_console.c?id=180a551d542844b70012d7b94a415aacdcf31d45
> Link:
> https://android.googlesource.com/platform/external/u-boot/+/4581f7c6e96d7332b534ae20de356a4554594d08
> Cc: Bin Meng
> ---
> drivers/virtio/Kconfig | 8 ++
> drivers/virtio/Makefile | 1 +
> drivers/virtio/virtio-uclass.c | 1 +
> drivers/virtio/virtio_console.c | 158
> include/virtio.h| 2 +
> 5 files changed, 170 insertions(+)
> create mode 100644 drivers/virtio/virtio_console.c
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 852f6735b6..94aaf70ab9 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -68,6 +68,14 @@ config VIRTIO_BLK
> This is the virtual block driver for virtio. It can be used with
> QEMU based targets.
>
> +config VIRTIO_CONSOLE
> + bool "virtio console driver"
> + depends on VIRTIO
> + default y
> + help
> + This is the virtual console driver for virtio. It can be used
> + with QEMU based targets.
> +
> config VIRTIO_RNG
> bool "virtio rng driver"
> depends on DM_RNG
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 4c63a6c690..c816f66aa6 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -10,4 +10,5 @@ obj-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
> obj-$(CONFIG_VIRTIO_SANDBOX) += virtio_sandbox.o
> obj-$(CONFIG_VIRTIO_NET) += virtio_net.o
> obj-$(CONFIG_VIRTIO_BLK) += virtio_blk.o
> +obj-$(CONFIG_VIRTIO_CONSOLE) += virtio_console.o
> obj-$(CONFIG_VIRTIO_RNG) += virtio_rng.o
> diff --git a/drivers/virtio/virtio-uclass.c b/drivers/virtio/virtio-uclass.c
> index 31bb21c534..9c54c67572 100644
> --- a/drivers/virtio/virtio-uclass.c
> +++ b/drivers/virtio/virtio-uclass.c
> @@ -30,6 +30,7 @@
> static const char *const virtio_drv_name[VIRTIO_ID_MAX_NUM] = {
> [VIRTIO_ID_NET] = VIRTIO_NET_DRV_NAME,
> [VIRTIO_ID_BLOCK] = VIRTIO_BLK_DRV_NAME,
> + [VIRTIO_ID_CONSOLE] = VIRTIO_CONSOLE_DRV_NAME,
> [VIRTIO_ID_RNG] = VIRTIO_RNG_DRV_NAME,
> };
>
> diff --git a/drivers/virtio/virtio_console.c b/drivers/virtio/virtio_console.c
> new file mode 100644
> index 00..1d4b319b08
> --- /dev/null
> +++ b/drivers/virtio/virtio_console.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018, Tuomas Tynkkynen
> + * Copyright (C) 2018, Bin Meng
> + * Copyright (C) 2006, 2007, 2009 Rusty Russell, IBM Corporation
> + * Copyright (C) 2009, 2010, 2011 Red Hat, Inc.
> + * Copyright (C) 2009, 2010, 2011 Amit Shah
> + * Copyright (C) 2021 Ahmad Fatoum
> + * Copyright (C) 2021, Google LLC, schuffe...@google.com (A. Cody Schuffelen)
> + * Copyright (C) 2023, Linaro
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "virtio_blk.h"
> +
> +struct virtio_console_priv {
> + struct virtqueue *receiveq_port0;
> + struct virtqueue *transmitq_port0;
> + unsigned char inbuf[1] __aligned(4);
> +};
> +
> +static int virtio_console_bind(struct udevice *dev)
> +{
> + struct virtio_dev_priv *uc_priv = dev_get_uclass_priv(dev->parent);
> +
> + /* Indicate what driver features we support */
> + virtio_driver_features_init(uc_priv, NULL, 0, NULL, 0);
> +
> + return 0;
> +}
> +
> +/*
> + * Create a scatter-gather list representing our input buffer and put
> + * it in the queue.
> + */
> +static void add_i
Re: [PATCH 1/6] nvmem: core: add nvmem_dev_size() helper
On 10.01.23 11:54, Rafał Miłecki wrote:
> From: Rafał Miłecki
>
> This is required by layouts that need to read whole NVMEM space. It
> applies to NVMEM devices without hardcoded layout (like U-Boot
> environment data block).
>
> Signed-off-by: Rafał Miłecki
> ---
> drivers/nvmem/core.c | 13 +
> include/linux/nvmem-consumer.h | 1 +
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 1f05f0a50d86..81743ae8793b 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -2062,6 +2062,19 @@ void nvmem_del_cell_lookups(struct nvmem_cell_lookup
> *entries, size_t nentries)
> }
> EXPORT_SYMBOL_GPL(nvmem_del_cell_lookups);
>
> +/**
> + * nvmem_dev_size() - Get the size of a given nvmem device.
> + *
> + * @nvmem: nvmem device.
> + *
> + * Return: size of the nvmem device.
> + */
> +const size_t nvmem_dev_size(struct nvmem_device *nvmem)
The const here is quite unusual. You can make the parameter
a const struct nvmem_device though.
> +{
> + return nvmem->size;
> +}
> +EXPORT_SYMBOL_GPL(nvmem_dev_size);
> +
> /**
> * nvmem_dev_name() - Get the name of a given nvmem device.
> *
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index fa030d93b768..d88294ddf562 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -78,6 +78,7 @@ ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
> int nvmem_device_cell_write(struct nvmem_device *nvmem,
> struct nvmem_cell_info *info, void *buf);
>
> +const size_t nvmem_dev_size(struct nvmem_device *nvmem);
> const char *nvmem_dev_name(struct nvmem_device *nvmem);
>
> void nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries,
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] nvmem: u-boot-env: fix crc32 casting type
Hello Rafał,
On 18.08.22 06:38, Rafał Miłecki wrote:
> From: Rafał Miłecki
>
> This fixes:
> drivers/nvmem/u-boot-env.c:141:17: sparse: sparse: cast to restricted __le32
>
> Reported-by: kernel test robot
> Fixes: f955dc1445069 ("nvmem: add driver handling U-Boot environment
> variables")
> Signed-off-by: Rafał Miłecki
> ---
> drivers/nvmem/u-boot-env.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
> index 9b9abfb8f187..fb993ef4249f 100644
> --- a/drivers/nvmem/u-boot-env.c
> +++ b/drivers/nvmem/u-boot-env.c
> @@ -138,7 +138,7 @@ static int u_boot_env_parse(struct u_boot_env *priv)
> data_offset = offsetof(struct u_boot_env_image_redundant, data);
> break;
> }
> - crc32 = le32_to_cpu(*(uint32_t *)(buf + crc32_offset));
> + crc32 = le32_to_cpu(*(__le32 *)(buf + crc32_offset));
I had to do a double take to make sure alignment is not violated.
It's not, because crc32_offset is always zero.
How about rewriting this as
/* CRC32 is always at offset 0 */
crc32 = le32_to_cpup(buf);
To make the alignment clear to the reader?
> crc32_data_len = priv->mtd->size - crc32_data_offset;
> data_len = priv->mtd->size - data_offset;
>
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH V4 1/2] mtd: allow getting MTD device associated with a specific DT node
On 15.06.22 21:42, Rafał Miłecki wrote:
> From: Rafał Miłecki
>
> MTD subsystem API allows interacting with MTD devices (e.g. reading,
> writing, handling bad blocks). So far a random driver could get MTD
> device only by its name (get_mtd_device_nm()). This change allows
> getting them also by a DT node.
>
> This API is required for drivers handling DT defined MTD partitions in a
> specific way (e.g. U-Boot (sub)partition with environment variables).
>
> Signed-off-by: Rafał Miłecki
> Acked-by: Miquel Raynal
Reviewed-by: Ahmad Fatoum
> ---
> V3: First introduction of of_get_mtd_device_by_node()
> V4: Use EPROBE_DEFER
>
> Srinivas: in V3 Miquel said it's OK to push this patch through NVMEM
> ---
> drivers/mtd/mtdcore.c | 28
> include/linux/mtd/mtd.h | 1 +
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 9eb0680db312..3613cc142f25 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1154,6 +1154,34 @@ int __get_mtd_device(struct mtd_info *mtd)
> }
> EXPORT_SYMBOL_GPL(__get_mtd_device);
>
> +/**
> + * of_get_mtd_device_by_node - obtain an MTD device associated with a given
> node
> + *
> + * @np: device tree node
> + */
> +struct mtd_info *of_get_mtd_device_by_node(struct device_node *np)
> +{
> + struct mtd_info *mtd = NULL;
> + struct mtd_info *tmp;
> + int err;
> +
> + mutex_lock(_table_mutex);
> +
> + err = -EPROBE_DEFER;
> + mtd_for_each_device(tmp) {
> + if (mtd_get_of_node(tmp) == np) {
> + mtd = tmp;
> + err = __get_mtd_device(mtd);
> + break;
> + }
> + }
> +
> + mutex_unlock(_table_mutex);
> +
> + return err ? ERR_PTR(err) : mtd;
> +}
> +EXPORT_SYMBOL_GPL(of_get_mtd_device_by_node);
> +
> /**
> * get_mtd_device_nm - obtain a validated handle for an MTD device by
> * device name
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 955aee14b0f7..6fc841ceef31 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -677,6 +677,7 @@ extern int mtd_device_unregister(struct mtd_info *master);
> extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
> extern int __get_mtd_device(struct mtd_info *mtd);
> extern void __put_mtd_device(struct mtd_info *mtd);
> +extern struct mtd_info *of_get_mtd_device_by_node(struct device_node *np);
> extern struct mtd_info *get_mtd_device_nm(const char *name);
> extern void put_mtd_device(struct mtd_info *mtd);
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH V3 1/2] mtd: allow getting MTD device associated with a specific DT node
Hello Rafał,
On 11.06.22 22:46, Rafał Miłecki wrote:
> From: Rafał Miłecki
>
> MTD subsystem API allows interacting with MTD devices (e.g. reading,
> writing, handling bad blocks). So far a random driver could get MTD
> device only by its name (get_mtd_device_nm()). This change allows
> getting them also by a DT node.
>
> This API is required for drivers handling DT defined MTD partitions in a
> specific way (e.g. U-Boot (sub)partition with environment variables).
>
> Signed-off-by: Rafał Miłecki
> ---
> V3: First introduction of of_get_mtd_device_by_node()
>
> mtd maintainers: please let know how would you like this patch
> processed. Would that be OK for you to Review/Ack it and let it go
> through NVMEM tree?
> ---
> drivers/mtd/mtdcore.c | 28
> include/linux/mtd/mtd.h | 1 +
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 9eb0680db312..7dc214271c85 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1154,6 +1154,34 @@ int __get_mtd_device(struct mtd_info *mtd)
> }
> EXPORT_SYMBOL_GPL(__get_mtd_device);
>
> +/**
> + * of_get_mtd_device_by_node - obtain an MTD device associated with a given
> node
> + *
> + * @np: device tree node
> + */
> +struct mtd_info *of_get_mtd_device_by_node(struct device_node *np)
> +{
> + struct mtd_info *mtd = NULL;
> + struct mtd_info *tmp;
> + int err;
> +
> + mutex_lock(_table_mutex);
> +
> + err = -ENODEV;
Shouldn't this be -EPROBE_DEFER? That way drivers making
use of this function can defer probe until the device
is probed.
> + mtd_for_each_device(tmp) {
> + if (mtd_get_of_node(tmp) == np) {
> + mtd = tmp;
> + err = __get_mtd_device(mtd);
> + break;
> + }
> + }
> +
> + mutex_unlock(_table_mutex);
> +
> + return err ? ERR_PTR(err) : mtd;
> +}
> +EXPORT_SYMBOL_GPL(of_get_mtd_device_by_node);
> +
> /**
> * get_mtd_device_nm - obtain a validated handle for an MTD device by
> * device name
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 955aee14b0f7..6fc841ceef31 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -677,6 +677,7 @@ extern int mtd_device_unregister(struct mtd_info *master);
> extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
> extern int __get_mtd_device(struct mtd_info *mtd);
> extern void __put_mtd_device(struct mtd_info *mtd);
> +extern struct mtd_info *of_get_mtd_device_by_node(struct device_node *np);
> extern struct mtd_info *get_mtd_device_nm(const char *name);
> extern void put_mtd_device(struct mtd_info *mtd);
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH V3 2/2] nvmem: add driver handling U-Boot environment variables
Hello Rafał,
On 11.06.22 22:46, Rafał Miłecki wrote:
> From: Rafał Miłecki
>
> U-Boot stores its setup as environment variables. It's a list of
> key-value pairs stored on flash device with a custom header.
>
> This commit adds an NVMEM driver that:
> 1. Provides NVMEM access to environment vars binary data
> 2. Extracts variables as NVMEM cells
>
> Current Linux's NVMEM sysfs API allows reading whole NVMEM data block.
> It can be used by user-space tools for reading U-Boot env vars block
> without the hassle of finding its location. Parsing will still need to
> be re-done there.
>
> Kernel-parsed NVMEM cells can be read however by Linux drivers. This may
> be uesful for Ethernet drivers for reading device MAC address which is
useful.
> often stored as U-Boot env variable.
>
> Signed-off-by: Rafał Miłecki
> ---
> V3: Use of_get_mtd_device_by_node() (thanks Ahmad) & update description
Looks better now!
> V2: Drop ARCH_BCM4908 dependency as there are plenty architectures using
> U-Boot bootloader. Thanks Srinivas.
>
> As noticed by Ahmad a missing NVMEM subsystem feature is user-space
> access to parsed NVMEM cells. That is something I started working on
> some time ago and I'm planning to get back to at some point, please
> check:
> [PATCH 2/2] nvmem: expose NVMEM cells in sysfs
> https://lore.kernel.org/lkml/20211220064730.28806-2-zaj...@gmail.com/
Thanks for the link.
> ---
> MAINTAINERS| 1 +
> drivers/nvmem/Kconfig | 11 ++
> drivers/nvmem/Makefile | 2 +
> drivers/nvmem/u-boot-env.c | 231 +
> 4 files changed, 245 insertions(+)
> create mode 100644 drivers/nvmem/u-boot-env.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 475e28365385..43b427fa76b0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20411,6 +20411,7 @@ U-BOOT ENVIRONMENT VARIABLES
> M: Rafał Miłecki
> S: Maintained
> F: Documentation/devicetree/bindings/nvmem/u-boot,env.yaml
> +F: drivers/nvmem/u-boot-env.c
>
> UACCE ACCELERATOR FRAMEWORK
> M: Zhangfei Gao
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index d72d879a6d34..5f1b32b953b9 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -344,4 +344,15 @@ config NVMEM_APPLE_EFUSES
> This driver can also be built as a module. If so, the module will
> be called nvmem-apple-efuses.
>
> +config NVMEM_U_BOOT_ENV
> + tristate "U-Boot environment variables support"
> + depends on OF && MTD
> + select CRC32
> + help
> + U-Boot stores its setup as environment variables. This driver adds
> + support for verifying & exporting such data. It also exposes variables
> + as NVMEM cells so they can be referenced by other drivers.
> +
> + If compiled as module it will be called nvmem_u-boot-env.
You should probably mention that this is tied to MTD somewhere
in the help text as u-boot env on EEPROM/raw eMMC is not covered by this.
> +
> endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index c710b64f9fe4..399f9972d45b 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -69,3 +69,5 @@ obj-$(CONFIG_NVMEM_APPLE_EFUSES)+= nvmem-apple-efuses.o
> nvmem-apple-efuses-y := apple-efuses.o
> obj-$(CONFIG_MICROCHIP_OTPC) += nvmem-microchip-otpc.o
> nvmem-microchip-otpc-y := microchip-otpc.o
> +obj-$(CONFIG_NVMEM_U_BOOT_ENV) += nvmem_u-boot-env.o
> +nvmem_u-boot-env-y := u-boot-env.o
> diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
> new file mode 100644
> index ..92c2dd11d99f
> --- /dev/null
> +++ b/drivers/nvmem/u-boot-env.c
> @@ -0,0 +1,231 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Rafał Miłecki
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +enum u_boot_env_format {
> + U_BOOT_FORMAT_SINGLE,
> + U_BOOT_FORMAT_REDUNDANT,
> +};
> +
> +struct u_boot_env {
> + struct device *dev;
> + enum u_boot_env_format format;
> +
> + /* Parent device */
> + struct mtd_info *mtd;
> + size_t offset;
> + size_t size;
> +
> + /* Cells */
> + struct nvmem_cell_info *cells;
> + int ncells;
> +};
> +
> +struct u_boot_env_image_single {
> + __le32 crc32;
> + uint8_t data[0];
GCC zero-length arrays are being phased out in favor
of flexible array members. Just replace the [0] with [].
See Documentation/process/deprecated.rst for more information.
> +} __packed;
> +
> +struct u_boot_env_image_redundant {
> + __le32 crc32;
> + u8 mark;
> + uint8_t data[0];
Same here
> +} __packed;
> +
> +static int u_boot_env_read(void *context, unsigned int offset, void *val,
> +size_t bytes)
> +{
> + struct u_boot_env *priv = context;
> + struct device *dev = priv->dev;
> + size_t bytes_read;
> +
Re: [PATCH V2] nvmem: add driver handling U-Boot environment variables
Hello Rafał,
On 05.05.22 07:46, Rafał Miłecki wrote:
> On 4.05.2022 11:23, Ahmad Fatoum wrote:
>> Hello Rafał,
>>
>> On 03.05.22 18:56, Rafał Miłecki wrote:
>>> From: Rafał Miłecki
>>>
>>> U-Boot stores its setup as environment variables. It's a list of
>>> key-value pairs stored on flash device with a custom header.
>>>
>>> This commit adds an NVMEM driver that:
>>> 1. Provides NVMEM access to environment vars binary data
>>> 2. Extracts variables as NVMEM cells
>>>
>>> It can be used for:
>>> 1. Accessing env variables from user-space
>>
>> Is this already possible? The only interface I know of is the /nvmem
>> file in sysfs, but that one is not per cell, but per device.
>
> Maybe that wasn't precise enough, I should probably write:
> 1. Parsing binary data from user-space
>
> In future I'd like to extend U-Boot's "printenv" tool to support reading
> env variables blob using Linux's sysfs as documented in the
> Documentation/ABI/stable/sysfs-bus-nvmem
So, would you use this interface just to save fw_printenv the hassle
of finding the environment (but redoing parsing) or do you intend
to preprocess the data too? (e.g. only show the active environment)
For your use case, it sound like teaching NVMEM core to export
cells as binary sysfs files would be very useful.
>>> + label = of_get_property(np->parent, "label", NULL);
>>> + if (!label)
>>> + label = np->parent->name;
>>> +
>>> + priv->mtd = get_mtd_device_nm(label);
>>> + if (IS_ERR(priv->mtd)) {
>>> + dev_err(dev, "Failed to find \"%s\" MTD device: %ld\n", label,
>>> PTR_ERR(priv->mtd));
>>> + return PTR_ERR(priv->mtd);
>>> + }
>>
>> I am trying to make sense of this using the binding, but I can't.
>> Do you have an example device tree fragment?
>
> This comes from unreleased yet board I'm working on.
>
> It stores U-Boot env variables in the middle of U-Boot binary.
Huh, that's an odd layout. I am not sure whether of_get_property to
arrive at the parent is such a good idea though. Doesn't it enforce
a limitation that there must not exist two partitions with the same label?
Some systems can have a second recovery bootloader for example.
Given that these are device tree nodes, wouldn't it be possible
to find the MTD by device tree parent instead of via its label?
> partitions {
> compatible = "fixed-partitions";
> #address-cells = <1>;
> #size-cells = <1>;
>
> partition@0 {
> label = "loader";
> reg = <0x0 0x10>;
>
> partition@4 {
> compatible = "u-boot,env";
> label = "u-boot-env";
> reg = <0x4 0x4000>;
> };
> };
>
> partition@10 {
> label = "image";
> reg = <0x10 0x1fe0>;
> };
> };
Thanks,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH V2] nvmem: add driver handling U-Boot environment variables
Hello Rafał,
On 03.05.22 18:56, Rafał Miłecki wrote:
> From: Rafał Miłecki
>
> U-Boot stores its setup as environment variables. It's a list of
> key-value pairs stored on flash device with a custom header.
>
> This commit adds an NVMEM driver that:
> 1. Provides NVMEM access to environment vars binary data
> 2. Extracts variables as NVMEM cells
>
> It can be used for:
> 1. Accessing env variables from user-space
Is this already possible? The only interface I know of is the /nvmem
file in sysfs, but that one is not per cell, but per device.
> + label = of_get_property(np->parent, "label", NULL);
> + if (!label)
> + label = np->parent->name;
> +
> + priv->mtd = get_mtd_device_nm(label);
> + if (IS_ERR(priv->mtd)) {
> + dev_err(dev, "Failed to find \"%s\" MTD device: %ld\n", label,
> PTR_ERR(priv->mtd));
> + return PTR_ERR(priv->mtd);
> + }
I am trying to make sense of this using the binding, but I can't.
Do you have an example device tree fragment?
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH v2 2/5] arm: stm32mp: handle the OP-TEE nodes in DT with FIP support
Hello Patrick, On 15.07.21 17:22, Patrick Delaunay wrote: > With FIP support in TF-A (when CONFIG_STM32MP15x_STM32IMAGE > is not activated), the DT nodes needed by OP-TEE are added by OP-TEE > firmware in U-Boot device tree, present in FIP. What about the SCMI nodes. Who will fix up those? > These nodes are only required in trusted boot, when TF-A load the file > u-boot.stm32, including the U-Boot device tree with STM32IMAGE header, > in this case OP-TEE can't update the U-Boot device tree. [snip] Cheers, Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH] usb: dwc2: change compatible st, stm32mp1-hsotg to st, stm32mp15-hsotg
Hello Patrick,
On 19.02.21 14:30, Patrick DELAUNAY wrote:
> Hi Ahmad,
>
> On 2/11/21 12:14 PM, Ahmad Fatoum wrote:
>> Hi,
>>
>> On 10.02.21 20:59, Tom Rini wrote:
>>> On Tue, Feb 09, 2021 at 08:51:26PM +0100, Patrick DELAUNAY wrote:
>>>> On 2/9/21 11:39 AM, Marek Vasut wrote:
>>>>> On 2/9/21 11:14 AM, Patrick Delaunay wrote:
>>>>> Hi,
>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/drivers/usb/gadget/dwc2_udc_otg.c
>>>>>> b/drivers/usb/gadget/dwc2_udc_otg.c
>>>>>> index e3871e381e..ecac80fc11 100644
>>>>>> --- a/drivers/usb/gadget/dwc2_udc_otg.c
>>>>>> +++ b/drivers/usb/gadget/dwc2_udc_otg.c
>>>>>> @@ -1176,7 +1176,7 @@ static int dwc2_udc_otg_remove(struct udevice
>>>>>> *dev)
>>>>>> static const struct udevice_id dwc2_udc_otg_ids[] = {
>>>>>> { .compatible = "snps,dwc2" },
>>>>>> { .compatible = "brcm,bcm2835-usb" },
>>>>>> - { .compatible = "st,stm32mp1-hsotg",
>>>>>> + { .compatible = "st,stm32mp15-hsotg",
>>>>>> .data = (ulong)dwc2_set_stm32mp1_hsotg_params },
>>>>> I have to point out the obvious, DT is ABI, this breaks ABI. However, do
>>>>> we care about out-of-tree DTs here ?
>>>>
>>>> I know that the binding backward compatibility and "binary compatible" the
>>>> is a key element of DT
>>>>
>>>> for the Linux kernel (for example the latest kernel image should work with
>>>> a
>>>> old device tree).
>>> The way we use DTs in U-Boot we don't enforce ABI because we allow for
>>> DTS and bindings to come in before they're fully stabilized in
>>> linux-next/similar and then it's required to re-sync them once they are
>>> final.
>> I think platforms like the STM32MP1 should be handled specially, because
>> they support having an external device tree passed from the FSBL at runtime.
>> See
>> https://github.com/trini/u-boot/blob/master/arch/arm/mach-stm32mp/boot_params.c#L32
>>
>> @Patrick, wouldn't this change break booting newer U-Boot with older TF-A in
>> some configurations? Or is this reusing-fsbl-fdt feature unused?
>
>> Cheers,
>> Ahmad
>
> I introduce this patch to prepare the FIP support in STM32MP15x TF-A
>
> but it is not yet ready today in downstream or in upstream (STOpenLinux v2.1).
>
>
> In this FIP use-case, the DT provided by TF-A to U-Boot is the "u-boot.dtb"
>
> generated by U-Boot compilation (including the required "u-boot,dm-pre-reloc"
>
> add-on).
>
>
> The files "u-boot-nodtb.bin" and "u-boot.dtb" are provided to TF-A makefile
>
> when the FIP is create; I hope these files are aligned as resulting of the
>
> U-Boot compilation.
>
>
> And update of existing FIP is possible with command:
>
> PC $> fiptool update --nt-fw u-boot-nodtb.bin --hw-config u-boot.dtb fip.bin
>
>
> Here I hope the DT and U-Boot binary are updated at the same time
>
> to avoid any dependency issue (even if it is not mandatory).
>
>
> But as this FIP feature is not yet used, I don't break the current or the FIP
> STM32MP1
>
> configurations, except if I miss something, as the 2 U-Boot files in FIP
>
> are updated at the same time.
I think too it should cause no problem then. Thanks for the explanation!
Cheers,
Ahmad
>
>
> Regards,
>
> Patrick
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH] usb: dwc2: change compatible st, stm32mp1-hsotg to st, stm32mp15-hsotg
On 11.02.21 14:02, Tom Rini wrote: > On Thu, Feb 11, 2021 at 12:14:51PM +0100, Ahmad Fatoum wrote: >> I think platforms like the STM32MP1 should be handled specially, because >> they support having an external device tree passed from the FSBL at runtime. >> See >> https://github.com/trini/u-boot/blob/master/arch/arm/mach-stm32mp/boot_params.c#L32 >> >> @Patrick, wouldn't this change break booting newer U-Boot with older TF-A in >> some configurations? Or is this reusing-fsbl-fdt feature unused? > > The long stated policy of U-Boot is to allow non-final bindings to be > used until they're finalized in Linux in order to address the "chicken > and egg" problem, since it's already a terrible idea to go to production > with a Linux kernel that's using non-final bindings. Any older TF-A > that doesn't work with this newer binding should be on a developer board > and they can just upgrade. Linux says "DT is ABI" and allows the ABI to > break when there's a bug in the DT. We don't say "DT is ABI" we say "we > use the Linux kernel binding". I see. Thanks for the clarification. I am still curious what configurations use the TF-A-provided device tree for U-Boot. Patrick? -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH] usb: dwc2: change compatible st, stm32mp1-hsotg to st, stm32mp15-hsotg
Hi,
On 10.02.21 20:59, Tom Rini wrote:
> On Tue, Feb 09, 2021 at 08:51:26PM +0100, Patrick DELAUNAY wrote:
>>
>> On 2/9/21 11:39 AM, Marek Vasut wrote:
>>> On 2/9/21 11:14 AM, Patrick Delaunay wrote:
>>> Hi,
>>>
>>> [...]
>>>
diff --git a/drivers/usb/gadget/dwc2_udc_otg.c
b/drivers/usb/gadget/dwc2_udc_otg.c
index e3871e381e..ecac80fc11 100644
--- a/drivers/usb/gadget/dwc2_udc_otg.c
+++ b/drivers/usb/gadget/dwc2_udc_otg.c
@@ -1176,7 +1176,7 @@ static int dwc2_udc_otg_remove(struct udevice
*dev)
static const struct udevice_id dwc2_udc_otg_ids[] = {
{ .compatible = "snps,dwc2" },
{ .compatible = "brcm,bcm2835-usb" },
- { .compatible = "st,stm32mp1-hsotg",
+ { .compatible = "st,stm32mp15-hsotg",
.data = (ulong)dwc2_set_stm32mp1_hsotg_params },
>>>
>>> I have to point out the obvious, DT is ABI, this breaks ABI. However, do
>>> we care about out-of-tree DTs here ?
>>
>>
>> I know that the binding backward compatibility and "binary compatible" the
>> is a key element of DT
>>
>> for the Linux kernel (for example the latest kernel image should work with a
>> old device tree).
>
> The way we use DTs in U-Boot we don't enforce ABI because we allow for
> DTS and bindings to come in before they're fully stabilized in
> linux-next/similar and then it's required to re-sync them once they are
> final.
I think platforms like the STM32MP1 should be handled specially, because
they support having an external device tree passed from the FSBL at runtime.
See
https://github.com/trini/u-boot/blob/master/arch/arm/mach-stm32mp/boot_params.c#L32
@Patrick, wouldn't this change break booting newer U-Boot with older TF-A in
some configurations? Or is this reusing-fsbl-fdt feature unused?
Cheers,
Ahmad
>
>
> ___
> Uboot-stm32 mailing list
> uboot-st...@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/uboot-stm32
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On 10/9/20 7:12 PM, Ahmad Fatoum wrote: > to do within normal world is mapping it XN, cacheable and not be in manager > domain. s/cacheable/uncacheable/ of course. > Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't > using either. > Why handle OP-TEE DRAM specially?) > > Cheers > Ahmad > >> >>> Cheers >>> Ahmad >>> >>> -- >>> Pengutronix e.K. | | >>> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >>> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| >>> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Hello Patrick, On 10/9/20 5:52 PM, Patrick DELAUNAY wrote: > I checked DACR behavior and CheckDomain / CheckPermission > > In my case the cortex A7 try to access to part of DDR / mapped cacheable and > bufferable, protected by firewall. > > So to use DACR I always need to configure the MMU with several Domain > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC) > - secure part of DDR as Domain 1 (DCACHE_OFF) > > For other part of MMU region, the I/O registers are mapped as register with > Domain 0 (D_CACHE_OFF) > > Then I can set DACR = 0x > => Client Accesses are checked against the access permission bits in the TLB > entry > > You ar right, the access permission is check only for domain configurated as > client in DACR > > But in ARM architecture > > B2.4.8 Access permission checking > > CheckPermission() pseudo code > Only check perms.ap is checked > And perms.xp is not checked > > But as the secure memory is mapped cacheable by secure OS, OP-TEE > I think to avoid to map the region is the ARM preconized solution > As explain in my answer to ard in [1] > > [1] > http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958 I don't think the aliasing described in "A3.5.7 Memory access restrictions" applies if the same memory is mapped twice only, once in secure and another in normal world. Data is already segregated in the caches by means of a NS bit. Only thing you should need to do within normal world is mapping it XN, cacheable and not be in manager domain. Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't using either. Why handle OP-TEE DRAM specially?) Cheers Ahmad > >> Cheers >> Ahmad >> >> -- >> Pengutronix e.K. | | >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Hello Ard, Patrick, On 10/7/20 12:26 PM, Ard Biesheuvel wrote: >> The issue is solved only when the region reserved by OP-TEE is no more >> mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is >> already done in Linux kernel. >> > > Spurious peculative accesses to device regions would be a severe > silicon bug, so I wonder what is going on here. > > (Apologies if we are rehashing stuff here that has already been > discussed - I don't remember the details) > > Are you sure that the speculative accesses were not caused by > misconfigured CPU or page tables, missing TLB maintenance, etc etc? > Because it really does smell like a software issue not a hardware > issue. I debugged a similar issue a year ago on an i.MX6 UltraLite (also a Cortex-A7) that turned to ultimately be caused by barebox not mapping I/O memory as non-executable. This led to very interesting effects. My findings[1] back then were that U-Boot did set the eXecute Never bit only on OMAP, but not for other platforms. So I could imagine this being the root cause of Patrick's issues as well: The CPU is speculatively executing from the region that the firewalled DRAM is mapped at. barebox now configures XN for non-RAM before it turns on the MMU. You should do that as well (in ARM arch code, not only for stm32mp1). Additionally, you will want to XN map the region where your OP-TEE sits at. [1]: https://community.nxp.com/thread/511925 Cheers Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Hello,
On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> My findings[1] back then were that U-Boot did set the eXecute Never bit only
> on
> OMAP, but not for other platforms. So I could imagine this being the root
> cause
> of Patrick's issues as well:
Rereading my own link, my memory is a little less fuzzy: eXecute Never was being
set, but was without effect due Manager mode being set in the DACR:
> The ARM Architecture Reference Manual notes[1]:
> > When using the Short-descriptor translation table format, the XN
> > attribute is not checked for domains marked as Manager.
> > Therefore, the system must not include read-sensitive memory in
> > domains marked as Manager, because the XN bit does not prevent
> > speculative fetches from a Manager domain.
> To avoid speculative access to read-sensitive memory-mapped peripherals
> on ARMv7, we'll need U-Boot to use client domain permissions, so the XN
> bit can function.
> This issue has come up before and was fixed in de63ac278
> ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> It's equally applicable to all ARMv7-A platforms where caches are
> enabled.
> [1]: B3.7.2 - Execute-never restrictions on instruction fetching
Hope this helps,
Ahmad
> The CPU is speculatively executing from the region that the firewalled DRAM
> is mapped at.
>
> barebox now configures XN for non-RAM before it turns on the MMU. You should
> do that as well (in ARM arch code, not only for stm32mp1). Additionally,
> you will want to XN map the region where your OP-TEE sits at.
>
> [1]: https://community.nxp.com/thread/511925
>
> Cheers
> Ahmad
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
