Re: [PATCH v2 2/2] Hexagon: implement machine flag check
Hi,
On Tue, 2024-04-02 at 21:38 +, Brian Cain wrote:
> > diff --git a/backends/hexagon_symbol.c b/backends/hexagon_symbol.c
> > index b341243e..1e681e9f 100644
> > --- a/backends/hexagon_symbol.c
> > +++ b/backends/hexagon_symbol.c
> > @@ -56,3 +56,11 @@ hexagon_reloc_simple_type (Ebl *ebl __attribute__
> > ((unused)), int type,
> >return ELF_T_NUM;
> > }
> > }
> > +
> > +bool
> > +hexagon_machine_flag_check (GElf_Word flags)
> > +{
> > + GElf_Word arch_variant = flags &~ EF_HEXAGON_MACH;
> > + /* 0x8000 covers the "tiny core" arch variants. */
> > + return arch_variant == 0 || arch_variant == 0x8000;
> > +}
> > --
> > 2.37.2
>
> What about this instead?
>
> bool hexagon_machine_flag_check(GElf_Word flags) {
> GElf_Word reserved_flags = ~(EF_HEXAGON_TINY | EF_HEXAGON_MACH);
>
> return (flags & reserved_flags) == 0;
> }
>
> ... implies a new EF_HEXAGON_TINY 0x8000 definition BTW.
You obviously know this architecture better than me. But is the TINY
flag (bit 15) appropriate for all machines? It looks like it can only
be set on V67 and V71. If so maybe just check those two explicitly?
Cheers,
Mark
RE: [PATCH v2 2/2] Hexagon: implement machine flag check
> -Original Message-
> From: Mark Wielaard
> Sent: Thursday, April 4, 2024 11:29 AM
> To: Brian Cain ; Matheus Bernardino (QUIC)
> ; elfutils-devel@sourceware.org
> Cc: Sid Manning ; Andrew Pinski (QUIC)
>
> Subject: Re: [PATCH v2 2/2] Hexagon: implement machine flag check
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> Hi,
>
> On Tue, 2024-04-02 at 21:38 +, Brian Cain wrote:
> > > diff --git a/backends/hexagon_symbol.c b/backends/hexagon_symbol.c
> > > index b341243e..1e681e9f 100644
> > > --- a/backends/hexagon_symbol.c
> > > +++ b/backends/hexagon_symbol.c
> > > @@ -56,3 +56,11 @@ hexagon_reloc_simple_type (Ebl *ebl __attribute__
> > > ((unused)), int type,
> > >return ELF_T_NUM;
> > > }
> > > }
> > > +
> > > +bool
> > > +hexagon_machine_flag_check (GElf_Word flags)
> > > +{
> > > + GElf_Word arch_variant = flags &~ EF_HEXAGON_MACH;
> > > + /* 0x8000 covers the "tiny core" arch variants. */
> > > + return arch_variant == 0 || arch_variant == 0x8000;
> > > +}
> > > --
> > > 2.37.2
> >
> > What about this instead?
> >
> > bool hexagon_machine_flag_check(GElf_Word flags) {
> > GElf_Word reserved_flags = ~(EF_HEXAGON_TINY |
> EF_HEXAGON_MACH);
> >
> > return (flags & reserved_flags) == 0;
> > }
> >
> > ... implies a new EF_HEXAGON_TINY 0x8000 definition BTW.
>
> You obviously know this architecture better than me. But is the TINY
> flag (bit 15) appropriate for all machines? It looks like it can only
> be set on V67 and V71. If so maybe just check those two explicitly?
So far, V67T and V71T are the only ones with this variation. It could also be
specified on yet-to-be-created future devices (some theoretical V99T might
exist). What should the behavior be in those cases?
-Brian
Re: [PATCH v2 1/2] Add support for Hexagon
Hi Matheus, On Tue, 2024-04-02 at 16:55 -0300, Matheus Tavares Bernardino wrote: > This implements initial support for the Hexagon architecture. The > Hexagon ABI spec can be seen at > https://lists.llvm.org/pipermail/llvm-dev/attachments/20190916/21516a52/attachment-0001.pdf > > A hello_hexagon.ko test is also added. > > $ head tests/test-suite.log > [...] > # TOTAL: 275 > # PASS: 269 > # SKIP: 6 > # XFAIL: 0 > # FAIL: 0 > # XPASS: 0 > # ERROR: 0 > > $ cat tests/run-strip-reloc-ko.sh.log > [...] > runtest hello_hexagon.ko > PASS run-strip-reloc-ko.sh (exit status: 0) Very nice. Thanks for including the new testcase. Now we have at least a little test coverage that can be run on any arch to check hexagon is supported. The only thing I did before pushing this was to add a ChangeLog entry to the commit message: * backends/Makefile.am (modules): Add hexagon. (hexagon_SRCS): New var for hexagon_init.c and hexagon_symbol.c. (libebl_backends_a_SOURCES): Add hexagon_SRCS. * backends/hexagon_init.c: New file. * backends/hexagon_reloc.def: Likewise. * backends/hexagon_symbol.c: Likewise. * libebl/eblopenbackend.c (hexagon_init): Declare. (machines): Add hexagon. * libelf/elf-knowledge.h: Add hexagon e_flags values, section indices and and relocs. * src/elflint.c (valid_e_machine): Add EM_QDSP6. * tests/Makefile.am (EXTRA_DIST): Add hello_hexagon.ko.bz2. * tests/hello_hexagon.ko.bz2: New test file. * tests/run-strip-reloc-ko.sh: Add hello_hexagon.ko. This is mainly for my own review, so I know all changes were actually intended. Pushed, Mark
Re: [PATCH v2 2/2] Hexagon: implement machine flag check
Hi Brian, On Thu, 2024-04-04 at 16:31 +, Brian Cain wrote: > > > ... implies a new EF_HEXAGON_TINY 0x8000 definition BTW. > > > > You obviously know this architecture better than me. But is the TINY > > flag (bit 15) appropriate for all machines? It looks like it can only > > be set on V67 and V71. If so maybe just check those two explicitly? > > So far, V67T and V71T are the only ones with this variation. > It could also be specified on yet-to-be-created future devices > (some theoretical V99T might exist). > What should the behavior be in those cases? OK, if there could be other TINY variants then either the check suggested by Matheus or you would be more correct. I do like the idea to have a self documenting EF_HEXAGON_TINY constant for this. Thanks, Mark
[PATCH v3] Hexagon: implement machine flag check
This fixes the "invalid machine flag" error from eu-elflint when passing
hexagon binaries.
* backends/hexagon_init.c (hexagon_init): Hook
machine_flag_check
* backends/hexagon_symbol.c (hexagon_machine_flag_check):
new function
* libelf/elf-knowledge.h: add EF_HEXAGON_TINY constant
Signed-off-by: Matheus Tavares Bernardino
---
v2: https://sourceware.org/pipermail/elfutils-devel/2024q2/006987.html
Changes in v3:
- Added ChangeLog to commit message.
- Implemented better machine_flag_check operation, as suggested by
bcain.
- Extracted only patch 2/2 as 1/2 was already merged.
backends/hexagon_init.c | 1 +
backends/hexagon_symbol.c | 7 +++
libelf/elf-knowledge.h| 1 +
3 files changed, 9 insertions(+)
diff --git a/backends/hexagon_init.c b/backends/hexagon_init.c
index 9c8c6d8d..1cd27513 100644
--- a/backends/hexagon_init.c
+++ b/backends/hexagon_init.c
@@ -45,6 +45,7 @@ hexagon_init (Elf *elf __attribute__ ((unused)),
{
hexagon_init_reloc (eh);
HOOK (eh, reloc_simple_type);
+ HOOK (eh, machine_flag_check);
return eh;
}
diff --git a/backends/hexagon_symbol.c b/backends/hexagon_symbol.c
index b341243e..5f6e0fe5 100644
--- a/backends/hexagon_symbol.c
+++ b/backends/hexagon_symbol.c
@@ -56,3 +56,10 @@ hexagon_reloc_simple_type (Ebl *ebl __attribute__
((unused)), int type,
return ELF_T_NUM;
}
}
+
+bool
+hexagon_machine_flag_check (GElf_Word flags)
+{
+ GElf_Word reserved_flags = ~(EF_HEXAGON_TINY | EF_HEXAGON_MACH);
+ return (flags & reserved_flags) == 0;
+}
diff --git a/libelf/elf-knowledge.h b/libelf/elf-knowledge.h
index 71535934..23e34ca1 100644
--- a/libelf/elf-knowledge.h
+++ b/libelf/elf-knowledge.h
@@ -119,6 +119,7 @@
#define EF_HEXAGON_MACH_V71T 0x8071 /* Hexagon V71T */
#define EF_HEXAGON_MACH_V73 0x0073 /* Hexagon V73 */
#define EF_HEXAGON_MACH 0x03ff /* Hexagon V.. */
+#define EF_HEXAGON_TINY 0x8000 /* Hexagon V..T */
/* Special section indices. */
#define SHN_HEXAGON_SCOMMON0xff00 /* Other access sizes */
--
2.37.2
Re: [PATCH v3] Hexagon: implement machine flag check
Hi Matheus, On Thu, Apr 04, 2024 at 02:19:40PM -0300, Matheus Tavares Bernardino wrote: > This fixes the "invalid machine flag" error from eu-elflint when passing > hexagon binaries. > > * backends/hexagon_init.c (hexagon_init): Hook > machine_flag_check > * backends/hexagon_symbol.c (hexagon_machine_flag_check): > new function > * libelf/elf-knowledge.h: add EF_HEXAGON_TINY constant > > Signed-off-by: Matheus Tavares Bernardino > --- > > v2: https://sourceware.org/pipermail/elfutils-devel/2024q2/006987.html Note that we also have a public-inbox instance, which is useful in some cases since you can address patches by message-id: https://inbox.sourceware.org/elfutils-devel/d198f1806a486bd5129c996f10680b947ecdc581.1712087613.git.quic_mathb...@quicinc.com/ > Changes in v3: > - Added ChangeLog to commit message. > - Implemented better machine_flag_check operation, as suggested by > bcain. > - Extracted only patch 2/2 as 1/2 was already merged. This looks good. Pushed, Mark
Re: [PATCH v3] Hexagon: implement machine flag check
On Thu, Apr 04, 2024 at 21:43:11 +0200, Mark Wielaard wrote: > > > Note that we also have a public-inbox instance, which is useful in > some cases since you can address patches by message-id: > https://inbox.sourceware.org/elfutils-devel/d198f1806a486bd5129c996f10680b947ecdc581.1712087613.git.quic_mathb...@quicinc.com/ Awesome! I much rather prefer the public-inbox interface :) Thanks! BTW, just out of curiosity, since the last incident with xz's backdoor (which apparently involved malicious code disguised as a test binary), has the elfutils community already considered using something like Dockerfiles to generate the tests/*.ko.bz2 binaries instead of checking than in the git repo? Just something that crossed my mind while I was developing these patches.
RE: [PATCH v3] Hexagon: implement machine flag check
> -Original Message- > From: Matheus Bernardino (QUIC) > Sent: Thursday, April 4, 2024 12:57 PM > To: m...@klomp.org > Cc: Brian Cain ; elfutils-devel@sourceware.org; Andrew > Pinski (QUIC) ; Matheus Bernardino (QUIC) > ; Sid Manning > Subject: Re: [PATCH v3] Hexagon: implement machine flag check > > On Thu, Apr 04, 2024 at 21:43:11 +0200, Mark Wielaard wrote: > > > > > > Note that we also have a public-inbox instance, which is useful in > > some cases since you can address patches by message-id: > > https://inbox.sourceware.org/elfutils- > devel/d198f1806a486bd5129c996f10 > > 680b947ecdc581.1712087613.git.quic_mathb...@quicinc.com/ > > Awesome! I much rather prefer the public-inbox interface :) Thanks! > > BTW, just out of curiosity, since the last incident with xz's backdoor (which > apparently involved malicious code disguised as a test binary), has the > elfutils > community already considered using something like Dockerfiles to generate > the tests/*.ko.bz2 binaries instead of checking than in the git repo? Just > something that crossed my mind while I was developing these patches. That came across my mind too. Especially after Mark wrote backdoor on what might need to be improved for sourceware and what tooling is needed for the projects hosted on sourceware (which is most of the GNU toolchain): https://inbox.sourceware.org/gcc/20240329203909.gs9...@gnu.wildebeest.org/T/#mc980204081b149132aeace4bf3d83cf35dad72d8 Thanks, Andrew Pinski
