Re: [Qemu-devel] [PATCH v1 4/4] elf: move ELF_ARCH definition to elf-arch.h

[Richard Henderson]

On 9/11/19 5:26 AM, Alex Bennée wrote:
> 
> Aleksandar Markovic <address@hidden> writes:
> 
>> 10.09.2019. 21.34, "Alex Bennée" <address@hidden> је написао/ла:
>>>
>>> This is preparatory for plugins which will want to report the
>>> architecture to plugins. Move the ELF_ARCH definition out of the
>>> loader and into its own header.
>>>
>>> Signed-off-by: Alex Bennée <address@hidden>
>>> --
>>
>> Hi, Alex.
>>
>> I advice some caution here
>> . For example, EM_NANOMIPS, and some other EM_xxx constants are not
>> included in the new header.
> 
> EM_ARCH was never set to EM_NANOMIPS. In fact I can see e_machine is
> checked against it so I guess it is only ever used to checking the
> loading elf directly.
> 
> In fact EM_ARCH is only really the default fallback case for checking
> the elf type if there is not a more "forgiving" check done by arch
> specific code (elf_check_arch). The only other cace is the fallback for
> EM_MACHINE unless PPC does something with it.
> 
>> I am not sure what exactly you want to achieve
>> with this refactoring. But you may miss your goal, since some EM_xxx will
>> be missing from your new header. Is this the right way to achieve what you
>> want, and could you unintentionally introduce bugs? Can you outline in more
>> details your intentions around the new header? Is this refactoring the only
>> way?
> 
> As mentioned in the other reply maybe exposing a value from configure
> into config-target.[mak|h] would be a better approach?

I think using EM_FOO to tell a plugin about the architecture is just going to
be confusing.  As seen here wrt EM_NANOMIPS, but arguably elsewhere with
EM_SPARC vs EM_SPARC64.

You need to decide what it is that you want the plugin to know.  It is just be
gross architecture?  In which case QEMU_ARCH_FOO is probably enough.  Is it the
instruction set currently executing?  In which case neither EM_FOO nor
QEMU_ARCH_FOO is sufficient -- e.g. arm thumb -- and you'll have to invent
something new, because no two architectures handle this in the same way.  The
closest you might be able to get without invention from whole cloth is the
capstone cap_arch+cap_mode values from cc->disas_set_info().  But that only
handles the most popular of targets.

In any case, a single header that every target must touch is the wrong
approach.  If you want to do some cleanup, move these defines into e.g.
linux-user/$TARGET/target_elf.h.  (And I wouldn't bother touching bsd-user in
its current condition.)


r~