diff mbox

[1/3] linux-user: introduce functions to detect CPU type

Message ID 20180113144847.8403-2-laurent@vivier.eu (mailing list archive)
State New, archived
Headers show

Commit Message

Laurent Vivier Jan. 13, 2018, 2:48 p.m. UTC 
From: YunQiang Su <syq@debian.org>

Move CPU type name selection to a function,
and add a function to return ELF e_flags.

[lv: splitted the patch and some cleanup in get_elf_eflags()]
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---

Notes:
    YunQiang Su, please add your Signed-off-by that was
    missing in your original patch.

 linux-user/elfload.c |  37 +++++++++++++++++++
 linux-user/main.c    | 101 +++++++++++++++++++++++++++------------------------
 linux-user/qemu.h    |   1 +
 3 files changed, 91 insertions(+), 48 deletions(-)

Comments

Richard Henderson Jan. 15, 2018, 10:04 p.m. UTC | #1 
On 01/13/2018 06:48 AM, Laurent Vivier wrote:
> From: YunQiang Su <syq@debian.org>
> 
> Move CPU type name selection to a function,
> and add a function to return ELF e_flags.
> 
> [lv: splitted the patch and some cleanup in get_elf_eflags()]
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---

This needs to be split.

> +int get_elf_eflags(int fd, uint32_t *eflags)
> +{
> +    struct elfhdr ehdr;
> +    off_t offset;
> +    int ret;
> +
> +    /* Read ELF header */
> +    offset = lseek(fd, 0, SEEK_SET);
> +    if (offset == (off_t) -1) {
> +        return -1;
> +    }
> +    ret = read(fd, &ehdr, sizeof(ehdr));
> +    if (ret < sizeof(ehdr)) {
> +        return -1;
> +    }

There is no reason to read the elf header twice -- e_flags has already been
stored in the struct image_info.

> +static const char *get_cpu_model(int fd)
> +{
> +#if defined(TARGET_I386)
> +#ifdef TARGET_X86_64
> +    return "qemu64";
> +#else
> +    return "qemu32";
> +#endif

This should be our opportunity to split this ifdef chain into small inline
functions within linux-user/*/target_cpu.h.  Pass the e_flags value directly
instead of a file descriptor.


r~
Laurent Vivier Jan. 16, 2018, 2:13 p.m. UTC | #2 
Le 15/01/2018 à 23:04, Richard Henderson a écrit :
> On 01/13/2018 06:48 AM, Laurent Vivier wrote:
>> From: YunQiang Su <syq@debian.org>
>>
>> Move CPU type name selection to a function,
>> and add a function to return ELF e_flags.
>>
>> [lv: splitted the patch and some cleanup in get_elf_eflags()]
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
> 
> This needs to be split.
> 
>> +int get_elf_eflags(int fd, uint32_t *eflags)
>> +{
>> +    struct elfhdr ehdr;
>> +    off_t offset;
>> +    int ret;
>> +
>> +    /* Read ELF header */
>> +    offset = lseek(fd, 0, SEEK_SET);
>> +    if (offset == (off_t) -1) {
>> +        return -1;
>> +    }
>> +    ret = read(fd, &ehdr, sizeof(ehdr));
>> +    if (ret < sizeof(ehdr)) {
>> +        return -1;
>> +    }
> 
> There is no reason to read the elf header twice -- e_flags has already been
> stored in the struct image_info.

When we set cpu_model, image_info is not initialized.

Do you propose to move cpu_init() after loader_exec()?

>> +static const char *get_cpu_model(int fd)
>> +{
>> +#if defined(TARGET_I386)
>> +#ifdef TARGET_X86_64
>> +    return "qemu64";
>> +#else
>> +    return "qemu32";
>> +#endif
> 
> This should be our opportunity to split this ifdef chain into small inline
> functions within linux-user/*/target_cpu.h.  Pass the e_flags value directly
> instead of a file descriptor.
> 

Good idea.

Thanks,
Laurent
Richard Henderson Jan. 16, 2018, 3:54 p.m. UTC | #3 
On 01/16/2018 06:13 AM, Laurent Vivier wrote:
>> There is no reason to read the elf header twice -- e_flags has already been
>> stored in the struct image_info.
> 
> When we set cpu_model, image_info is not initialized.
> Do you propose to move cpu_init() after loader_exec()?

Sure.


r~
Laurent Vivier Jan. 16, 2018, 4:56 p.m. UTC | #4 
Le 16/01/2018 à 16:54, Richard Henderson a écrit :
> On 01/16/2018 06:13 AM, Laurent Vivier wrote:
>>> There is no reason to read the elf header twice -- e_flags has already been
>>> stored in the struct image_info.
>>
>> When we set cpu_model, image_info is not initialized.
>> Do you propose to move cpu_init() after loader_exec()?
> 
> Sure.

After checking, I think we can't move cpu_init() after loader_exec():

load_elf_binary() needs to fill AT_HWCAP and in the case of i386 it
depends on cpu->env.features[FEAT_1_EDX] that comes from the cpu model.

Thanks,
Laurent
diff mbox

Patch

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 20f3d8c2c3..03244f5e97 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2376,6 +2376,43 @@  give_up:
     g_free(syms);
 }
 
+int get_elf_eflags(int fd, uint32_t *eflags)
+{
+    struct elfhdr ehdr;
+    off_t offset;
+    int ret;
+
+    /* Read ELF header */
+    offset = lseek(fd, 0, SEEK_SET);
+    if (offset == (off_t) -1) {
+        return -1;
+    }
+    ret = read(fd, &ehdr, sizeof(ehdr));
+    if (ret < sizeof(ehdr)) {
+        return -1;
+    }
+    offset = lseek(fd, offset, SEEK_SET);
+    if (offset == (off_t) -1) {
+        return -1;
+    }
+
+    /* Check ELF signature */
+    if (!elf_check_ident(&ehdr)) {
+        return -1;
+    }
+
+    /* check header */
+    bswap_ehdr(&ehdr);
+    if (!elf_check_ehdr(&ehdr)) {
+        return -1;
+    }
+
+    /* return architecture id */
+    *eflags = ehdr.e_flags;
+
+    return 0;
+}
+
 int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
 {
     struct image_info interp_info;
diff --git a/linux-user/main.c b/linux-user/main.c
index 450eb3ce65..9ce90ae634 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4254,6 +4254,49 @@  static int parse_args(int argc, char **argv)
     return optind;
 }
 
+static const char *get_cpu_model(int fd)
+{
+#if defined(TARGET_I386)
+#ifdef TARGET_X86_64
+    return "qemu64";
+#else
+    return "qemu32";
+#endif
+#elif defined(TARGET_ARM)
+    return "any";
+#elif defined(TARGET_UNICORE32)
+    return "any";
+#elif defined(TARGET_M68K)
+    return "any";
+#elif defined(TARGET_SPARC)
+#ifdef TARGET_SPARC64
+    return "TI UltraSparc II";
+#else
+    return "Fujitsu MB86904";
+#endif
+#elif defined(TARGET_MIPS)
+#if defined(TARGET_ABI_MIPSN32) || defined(TARGET_ABI_MIPSN64)
+    return "5KEf";
+#else
+    return "24Kf";
+#endif
+#elif defined TARGET_OPENRISC
+    return "or1200";
+#elif defined(TARGET_PPC)
+# ifdef TARGET_PPC64
+    return "POWER8";
+# else
+    return "750";
+# endif
+#elif defined TARGET_SH4
+    return "sh7785";
+#elif defined TARGET_S390X
+    return "qemu";
+#else
+    return "any";
+#endif
+}
+
 int main(int argc, char **argv, char **envp)
 {
     struct target_pt_regs regs1, *regs = &regs1;
@@ -4318,46 +4361,17 @@  int main(int argc, char **argv, char **envp)
 
     init_qemu_uname_release();
 
+    execfd = qemu_getauxval(AT_EXECFD);
+    if (execfd == 0) {
+        execfd = open(filename, O_RDONLY);
+        if (execfd < 0) {
+            printf("Error while loading %s: %s\n", filename, strerror(errno));
+            _exit(EXIT_FAILURE);
+        }
+    }
+
     if (cpu_model == NULL) {
-#if defined(TARGET_I386)
-#ifdef TARGET_X86_64
-        cpu_model = "qemu64";
-#else
-        cpu_model = "qemu32";
-#endif
-#elif defined(TARGET_ARM)
-        cpu_model = "any";
-#elif defined(TARGET_UNICORE32)
-        cpu_model = "any";
-#elif defined(TARGET_M68K)
-        cpu_model = "any";
-#elif defined(TARGET_SPARC)
-#ifdef TARGET_SPARC64
-        cpu_model = "TI UltraSparc II";
-#else
-        cpu_model = "Fujitsu MB86904";
-#endif
-#elif defined(TARGET_MIPS)
-#if defined(TARGET_ABI_MIPSN32) || defined(TARGET_ABI_MIPSN64)
-        cpu_model = "5KEf";
-#else
-        cpu_model = "24Kf";
-#endif
-#elif defined TARGET_OPENRISC
-        cpu_model = "or1200";
-#elif defined(TARGET_PPC)
-# ifdef TARGET_PPC64
-        cpu_model = "POWER8";
-# else
-        cpu_model = "750";
-# endif
-#elif defined TARGET_SH4
-        cpu_model = "sh7785";
-#elif defined TARGET_S390X
-        cpu_model = "qemu";
-#else
-        cpu_model = "any";
-#endif
+        cpu_model = get_cpu_model(execfd);
     }
     tcg_exec_init(0);
     /* NOTE: we need to init the CPU at this stage to get
@@ -4450,15 +4464,6 @@  int main(int argc, char **argv, char **envp)
     cpu->opaque = ts;
     task_settid(ts);
 
-    execfd = qemu_getauxval(AT_EXECFD);
-    if (execfd == 0) {
-        execfd = open(filename, O_RDONLY);
-        if (execfd < 0) {
-            printf("Error while loading %s: %s\n", filename, strerror(errno));
-            _exit(EXIT_FAILURE);
-        }
-    }
-
     ret = loader_exec(execfd, filename, target_argv, target_environ, regs,
         info, &bprm);
     if (ret != 0) {
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 4edd7d0c08..c2a701163c 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -188,6 +188,7 @@  int loader_exec(int fdexec, const char *filename, char **argv, char **envp,
              struct target_pt_regs * regs, struct image_info *infop,
              struct linux_binprm *);
 
+int get_elf_eflags(int fd, uint32_t *eflags);
 int load_elf_binary(struct linux_binprm *bprm, struct image_info *info);
 int load_flt_binary(struct linux_binprm *bprm, struct image_info *info);