Re: [PATCH 5/6] build: Add mkimage support for powerpc/qoriq
On 04.03.24 08:22, Chris Johns wrote:
diff --git a/spec/build/bsps/powerpc/qoriq/mkimage.yml
b/spec/build/bsps/powerpc/qoriq/mkimage.yml
new file mode 100644
index 00..712fd237b1
--- /dev/null
+++ b/spec/build/bsps/powerpc/qoriq/mkimage.yml
@@ -0,0 +1,39 @@
+SPDX-License-Identifier: CC-BY-SA-4.0 OR BSD-2-Clause
+build-type: mkimage
+content: |
+ #!${PYTHON}
+
+ import gzip
+ import os
+ import shutil
+ import subprocess
+ import sys
+ import tempfile
+
+ with tempfile.TemporaryDirectory() as tmp_dir:
+ bin_path = os.path.join(tmp_dir, "bin")
+ gz_path = os.path.join(tmp_dir, "gz")
+ subprocess.run([
+ "${OBJCOPY}",
+ "-O", "binary", sys.argv[1], bin_path
+ ],
+ check=True)
+ with open(bin_path, "rb") as f_bin:
+ with gzip.open(gz_path, "wb") as f_gz:
+ shutil.copyfileobj(f_bin, f_gz)
+ subprocess.run([
+ "${U_BOOT_MKIMAGE}",
+ "-A", "ppc", "-O", "linux", "-T", "kernel", "-a", "0x4000", "-e",
+ "0x4000", "-n", "RTEMS", "-d", gz_path, sys.argv[2]
+ ],
+ check=True)
Sorry this patch is a no from me and adding python like this with such limited
error checking is something I am not comfortable with.
I am OK wih a python module that something robust can import and validate giving
the user consistent and meaningful error messages but as I have just said whole
programs in spec files like this, sorry thet is no from me.
Python exceptions usually give a lot of context, but sure you always can
improve things. If someone doesn't like the error handling in a mkimage
script he can improve it through patches.
The script may have to know details of the BSP configuration, so what
would be your approach to address this? With the script in the build
specification item you can simply use the variable substitution. I don't
think these scripts will be super complex, just a sequence of commands.
--
embedded brains GmbH & Co. KG
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.hu...@embedded-brains.de
phone: +49-89-18 94 741 - 16
fax: +49-89-18 94 741 - 08
Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 5/6] build: Add mkimage support for powerpc/qoriq
On 28/2/2024 2:15 am, Sebastian Huber wrote:
> Update #4272.
> ---
> spec/build/bsps/optpython.yml | 14
> spec/build/bsps/optubootmkimage.yml | 20
> spec/build/bsps/powerpc/qoriq/grp.yml | 6
> spec/build/bsps/powerpc/qoriq/mkimage.yml | 39 +++
> 4 files changed, 79 insertions(+)
> create mode 100644 spec/build/bsps/optpython.yml
> create mode 100644 spec/build/bsps/optubootmkimage.yml
> create mode 100644 spec/build/bsps/powerpc/qoriq/mkimage.yml
>
> diff --git a/spec/build/bsps/optpython.yml b/spec/build/bsps/optpython.yml
> new file mode 100644
> index 00..15e0e500e6
> --- /dev/null
> +++ b/spec/build/bsps/optpython.yml
> @@ -0,0 +1,14 @@
> +SPDX-License-Identifier: CC-BY-SA-4.0 OR BSD-2-Clause
> +actions:
> +- script: |
> +value = sys.executable
> +- env-assign: PYTHON
> +build-type: option
> +copyrights:
> +- Copyright (C) 2024 embedded brains GmbH & Co. KG
> +default: []
> +description: ''
> +enabled-by: true
> +links: []
> +name: PYTHON
> +type: build
> diff --git a/spec/build/bsps/optubootmkimage.yml
> b/spec/build/bsps/optubootmkimage.yml
> new file mode 100644
> index 00..65a996be50
> --- /dev/null
> +++ b/spec/build/bsps/optubootmkimage.yml
> @@ -0,0 +1,20 @@
> +SPDX-License-Identifier: CC-BY-SA-4.0 OR BSD-2-Clause
> +actions:
> +- get-string: null
> +- substitute: null
> +- find-optional-program: null
> +- env-assign: U_BOOT_MKIMAGE
> +build-type: option
> +copyrights:
> +- Copyright (C) 2024 embedded brains GmbH & Co. KG
> +default:
> +- enabled-by: true
> + value: mkimage
> +description: |
> + This build option defines the name of the U-Boot boot loader tool to make
> an
> + image.
> +enabled-by: true
> +format: '{}'
> +links: []
> +name: U_BOOT_MKIMAGE
> +type: build
> diff --git a/spec/build/bsps/powerpc/qoriq/grp.yml
> b/spec/build/bsps/powerpc/qoriq/grp.yml
> index 65e623fdbd..cb96682722 100644
> --- a/spec/build/bsps/powerpc/qoriq/grp.yml
> +++ b/spec/build/bsps/powerpc/qoriq/grp.yml
> @@ -18,6 +18,10 @@ links:
>uid: ../../objirq
> - role: build-dependency
>uid: ../../optconsolebaud
> +- role: build-dependency
> + uid: ../../optobjcopy
> +- role: build-dependency
> + uid: ../../optubootmkimage
> - role: build-dependency
>uid: ../crti
> - role: build-dependency
> @@ -114,6 +118,8 @@ links:
>uid: optuartirq
> - role: build-dependency
>uid: start
> +- role: build-dependency
> + uid: mkimage
> - role: build-dependency
>uid: ../../bspopts
> type: build
> diff --git a/spec/build/bsps/powerpc/qoriq/mkimage.yml
> b/spec/build/bsps/powerpc/qoriq/mkimage.yml
> new file mode 100644
> index 00..712fd237b1
> --- /dev/null
> +++ b/spec/build/bsps/powerpc/qoriq/mkimage.yml
> @@ -0,0 +1,39 @@
> +SPDX-License-Identifier: CC-BY-SA-4.0 OR BSD-2-Clause
> +build-type: mkimage
> +content: |
> + #!${PYTHON}
> +
> + import gzip
> + import os
> + import shutil
> + import subprocess
> + import sys
> + import tempfile
> +
> + with tempfile.TemporaryDirectory() as tmp_dir:
> + bin_path = os.path.join(tmp_dir, "bin")
> + gz_path = os.path.join(tmp_dir, "gz")
> + subprocess.run([
> + "${OBJCOPY}",
> + "-O", "binary", sys.argv[1], bin_path
> + ],
> + check=True)
> + with open(bin_path, "rb") as f_bin:
> + with gzip.open(gz_path, "wb") as f_gz:
> + shutil.copyfileobj(f_bin, f_gz)
> + subprocess.run([
> + "${U_BOOT_MKIMAGE}",
> + "-A", "ppc", "-O", "linux", "-T", "kernel", "-a", "0x4000", "-e",
> + "0x4000", "-n", "RTEMS", "-d", gz_path, sys.argv[2]
> + ],
> + check=True)
Sorry this patch is a no from me and adding python like this with such limited
error checking is something I am not comfortable with.
I am OK wih a python module that something robust can import and validate giving
the user consistent and meaningful error messages but as I have just said whole
programs in spec files like this, sorry thet is no from me.
Chris
> +copyrights:
> +- Copyright (C) 2024 embedded brains GmbH & Co. KG
> +enabled-by:
> + and:
> + - HAVE_OBJCOPY
> + - HAVE_U_BOOT_MKIMAGE
> +links:
> +- role: build-dependency
> + uid: ../../optpython
> +type: build
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 4/6] build: Add support to make bootloader images
On 1/3/2024 9:57 pm, Sebastian Huber wrote:
> On 29.02.24 00:05, Chris Johns wrote:
If it is will the details be exported in the pkgconfig file and made
available
for users building applications in a consistent and easy to use way?
>>> Application build systems can query the tool using the RTEMS_MKIMAGE package
>>> configuration varible, for example:
>>>
>>> pkg-config --variable=RTEMS_MKIMAGE
>>> ${prefix}/lib/pkgconfig/${ARCH}-${BSP_NAME}.pc
>> Nice. This is my preferred way of handling this.
>>
>>> If the BSP does not provide a tool, then the variable RTEMS_MKIMAGE is set
>>> to
>>> "false".
>> So the process has to be a single command?
>
> Yes, a single command which is written in Python. For the U-Boot image it
> converts the ELF file to binary, then to a gz file, then to the U-Boot image.
I see it is in a YAML spec file. Sorry that is a no from me. See below.
I have learnt the hard way from the RSB and others we need to be precise with
these interfaces and it is best we consider and review them before we launch
them.
>>> It could help to export also EXEEXT and BOOT_IMAGE_EXTENSION in the package
>>> configuration file. For RTEMS 6, we should have a look how our package
>>> configuration support can be used to build applications on some commonly
>>> used
>>> build systems. We are currently not able to produce build images.
>> Yes we should. I also wonder if base addresses and other values that get used
>> should be prov
>>
Is this output created along side the ELF file?
>>> Yes.
>> +1
>>
Does this approach handle all BSPs that need this?
>>> The BSP can use Python, so I would say yes.
>> I am sorry I do not follow.
>
> The script is written in Python, so this should be good enough to generate
> boot
> images for all kinds of BSPs.
Python is good but I am not convinced we should allow all the code (or any code)
to resides in the spec YML file as you have. For example the code you have
posted for this command uses uboot's mkimage command and I am not comfortable we
make a host OS specific dependency that has no configure check (I can see), ie
does this patch set build on an M class MacOS machine without brew or macports
help. [1]
I cautious of host code being included in spec files. It is too easy to sneak in
code we depend that creates untracked dependencies and then who cleans that up?
Reverting a patch like this because we find one later would be counter
productive to everyone.
I am happy if we add a command to the rtems-tool repo that can find and import a
python module installed by a BSP or resident in the build tree but it needs to
be python end to end and not reliant on a subprocess call unless the subprocess
call uses a tool installed by the RSB. The import interface would need to
specified so it can be supported long term.
And the problem with your code is a lack of error handling. We should avoid
python exceptions for user errors. They should be for program errors.
[1] https://git.rtems.org/rtems-tools/tree/misc/tools/mkimage.py
Will you be converting all BSPs that need this type of support?
>>> I will add support for the BSPs using U-Boot.
>> Could you please provide the high level view of how this is to be handled? I
>> have not reviewed all the detail in the patches to understand this and even
>> then
>> I may get things wrong.
>
> The BSP provides an optional script to convert an ELF file into a boot image.
> In
> a Makefile it could be used like this:
>
> EXEEXT = $(shell pkg-config --variable=EXEEXT $(PKG_CONFIG))
> RTEMS_MKIMAGE = $(shell pkg-config --variable=RTEMS_MKIMAGE $(PKG_CONFIG))
>
> ifeq ($(RTEMS_MKIMAGE), false)
> APP = $(BUILDDIR)/app$(EXEEXT)
> else
> BOOT_IMAGE_EXTENSION = $(shell pkg-config --variable=BOOT_IMAGE_EXTENSION
> $(PKG_CONFIG))
> APP = $(BUILDDIR)/app$(BOOT_IMAGE_EXTENSION)
>
> %$(BOOT_IMAGE_EXTENSION): %$(EXEEXT)
> $(RTEMS_MKIMAGE) $^ $@
> endif
>
>> Should we create a GSoC project to review and support the other BSPs?
>
> I would add the boot image support only if needed. There are probably more
> important things to do.
The idea of the project would be investigating which other BSPs need this. An an
example
https://github.com/epics-base/epics-base/blob/7.0/configure/os/CONFIG.Common.RTEMS-mvme2700.
There are other example we need to clean up.
Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [RFC] libdl: Make Elf_Sym::st_other available
On 1/3/2024 6:05 pm, Sebastian Huber wrote:
> On 29.02.24 00:27, Chris Johns wrote:
>> On 27/2/2024 6:46 pm, Sebastian Huber wrote:
>>> The 64-bit PowerPC ELFv2 relocation support needs access to the
>>> Elf_Sym::st_other symbol information. The machine-specific relocation
>>> handler
>>> had only access to the Elf_Sym::st_info symbol information. This change
>>> extends the 8-bit syminfo parameter to 16-bit and uses the additional
>>> 8-bits to provide Elf_Sym::st_other. Another approach could be to pass
>>> a pointer to an Elf_Sym object instead of symname, syminfo, and
>>> symvalue.
>>
>> I think symname and symvalue have to stay or the code needed to support them
>> moves out to all reloc handlers [1]. I agree there is a limit to the number
>> args
>> to keep adding. If there is a need for more fields then it may pay to pass in
>> Elf_Sym* or rtems_rtl_obj_sym* which is the symbol table reference?
>>
>> Chris
>>
>> [1] https://git.rtems.org/rtems/tree/cpukit/libdl/rtl-elf.c#n429
>
> What do you prefer, a new st_other parameter, use Elf_Sym*, or use
> rtems_rtl_obj_sym*?
>
> The
>
> /**
> * An object file symbol.
> */
> typedef struct rtems_rtl_obj_sym
> {
> rtems_chain_node node; /**< The node's link in the chain. */
> const char* name; /**< The symbol's name. */
> void* value; /**< The value of the symbol. */
> uint32_t data; /**< Format specific data. */
> } rtems_rtl_obj_sym;
>
> has no st_info and st_other members. I tend to pass a Elf_Sym* pointer.
Ah thanks. I think Elf_Sym* as changes to sizeof(rtems_rtl_obj_sym) effects the
size of the runtime symbol table. The data you need is only for resolving that
obj's relocs and nothing more if I understand things correctly.
Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
