Re: [Qemu-devel] [PATCH v7 1/3] loader: Allow ELF loader to auto-detect the ELF arch

[Alistair Francis]

On Thu, Jun 9, 2016 at 10:38 AM, Peter Maydell <address@hidden> wrote:
> On 25 May 2016 at 19:49, Alistair Francis <address@hidden> wrote:
>> If the caller didn't specify an architecture for the ELF machine
>> the load_elf() function will auto detect it based on the ELF file.
>>
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>> V7:
>>  - Fix typo
>>
>>  hw/core/loader.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 53e0e41..a8a372d 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -419,6 +419,7 @@ int load_elf(const char *filename, uint64_t 
>> (*translate_fn)(void *, uint64_t),
>>  {
>>      int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>>      uint8_t e_ident[EI_NIDENT];
>> +    uint16_t e_machine;
>>
>>      fd = open(filename, O_RDONLY | O_BINARY);
>>      if (fd < 0) {
>> @@ -451,6 +452,15 @@ int load_elf(const char *filename, uint64_t 
>> (*translate_fn)(void *, uint64_t),
>>          goto fail;
>>      }
>>
>> +    if (elf_machine < 1) {
>> +        /* The caller didn't specify an ARCH, we can figure it out */
>> +        lseek(fd, 0x12, SEEK_SET);
>> +        if (read(fd, &e_machine, sizeof(e_machine)) != sizeof(e_machine)) {
>> +            goto fail;
>> +        }
>> +        elf_machine = e_machine;
>> +    }
>
> Isn't there an endianness problem here, given that e_machine is a 16
> bit field? In load_elf32()/load_elf64() we will byteswap the e_machine
> field we read off the disk if must_swab is true, which will mean it won't
> match the value we've read here and not byteswapped.
>
> I think you're better off pushing the "allow architecture to be
> unspecified" support down into load_elf32()/load_elf64(), where
> it can just become
>
>     if (elf_machine < 1) {
>         elf_machine = ehdr.e_machine;
>     }
>
> once the load_elf code has read and byteswapped the header for you.

Good point, I didn't realise it was the same code for both. I have
moved it into load_elf64/load_elf32.

Thanks,

Alistair

>
> thanks
> -- PMM
>