[Qemu-devel] [PATCH 0/6] target/mips: Add support for prctl() PR_GET_FP_MODE and PR_SET_FP_MODE

[Qemu-devel] [PATCH 0/6] target/mips: Add support for prctl() PR_GET_FP_MODE and PR_SET_FP_MODE

[Stefan Markovic]

From: Stefan Markovic <smarkovic@wavecomp.com>

This series includes support for prctl() PR_GET_FP_MODE and PR_SET_FP_MODE. This requires
extracting MIPS.abiflags section from ELF file and fp_abi value handling.

Stefan Markovic (6):
  Define MIPS_ABI_FP_UNKNOWN macro
  Extend image_info struct with MIPS specific fp_abi and    
    interp_fp_abi fields
  Extract MIPS abiflags from ELF file
  Read and set FP ABI value from MIPS abiflags
  Determine the desired FPU mode
  Add prctl() PR_SET_FP_MODE and PR_GET_FP_MODE implementations

 include/elf.h                      |  2 +
 linux-user/elfload.c               | 37 +++++++++++++++++++
 linux-user/mips/cpu_loop.c         | 75 ++++++++++++++++++++++++++++++++++++++
 linux-user/mips/target_syscall.h   |  2 +
 linux-user/mips64/target_syscall.h |  2 +
 linux-user/qemu.h                  |  4 ++
 linux-user/syscall.c               | 62 +++++++++++++++++++++++++++++--
 7 files changed, 180 insertions(+), 4 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/6] Define MIPS_ABI_FP_UNKNOWN macro

[Stefan Markovic]

From: Stefan Markovic <smarkovic@wavecomp.com>

Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
---
 include/elf.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/elf.h b/include/elf.h
index 5f45f9b..c151164 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -87,6 +87,8 @@ typedef int64_t  Elf64_Sxword;
 #define EF_MIPS_MACH_LS3A     0x00a20000  /* ST Microelectronics Loongson 3A */
 #define EF_MIPS_MACH          0x00ff0000  /* EF_MIPS_MACH_xxx selection mask */
 
+#define MIPS_ABI_FP_UNKNOWN   (-1)        /* Unknown FP ABI (internal)       */
+
 #define MIPS_ABI_FP_ANY       0x0         /* FP ABI doesn't matter           */
 #define MIPS_ABI_FP_DOUBLE    0x1         /* -mdouble-float                  */
 #define MIPS_ABI_FP_SINGLE    0x2         /* -msingle-float                  */
-- 
1.9.1

[Qemu-devel] [PATCH 2/6] Extend image_info struct with MIPS specific fp_abi and interp_fp_abi fields

[Stefan Markovic]

From: Stefan Markovic <smarkovic@wavecomp.com>

Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
---
 linux-user/qemu.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 1beb6a2..a752c1c 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -61,6 +61,10 @@ struct image_info {
         abi_ulong       interpreter_loadmap_addr;
         abi_ulong       interpreter_pt_dynamic_addr;
         struct image_info *other_info;
+#ifdef TARGET_MIPS
+        int             fp_abi;
+        int             interp_fp_abi;
+#endif
 };
 
 #ifdef TARGET_I386
-- 
1.9.1

[Qemu-devel] [PATCH 3/6] Extract MIPS abiflags from ELF file

[Stefan Markovic]

From: Stefan Markovic <smarkovic@wavecomp.com>

Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
---
 linux-user/elfload.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 055f6a9..5881233 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1517,11 +1517,25 @@ static void bswap_sym(struct elf_sym *sym)
     bswaptls(&sym->st_size);
     bswap16s(&sym->st_shndx);
 }
+
+#ifdef TARGET_MIPS
+static void bswap_mips_abiflags(Mips_elf_abiflags_v0 *abiflags)
+{
+    bswap16s(&abiflags->version);
+    bswap32s(&abiflags->ases);
+    bswap32s(&abiflags->isa_ext);
+    bswap32s(&abiflags->flags1);
+    bswap32s(&abiflags->flags2);
+}
+#endif
 #else
 static inline void bswap_ehdr(struct elfhdr *ehdr) { }
 static inline void bswap_phdr(struct elf_phdr *phdr, int phnum) { }
 static inline void bswap_shdr(struct elf_shdr *shdr, int shnum) { }
 static inline void bswap_sym(struct elf_sym *sym) { }
+#ifdef TARGET_MIPS
+static inline void bswap_mips_abiflags(Mips_elf_abiflags_v0 *abiflags) { }
+#endif
 #endif
 
 #ifdef USE_ELF_CORE_DUMP
@@ -2364,6 +2378,25 @@ static void load_elf_image(const char *image_name, int image_fd,
                 goto exit_errmsg;
             }
             *pinterp_name = interp_name;
+#ifdef TARGET_MIPS
+        } else if (eppnt->p_type == PT_MIPS_ABIFLAGS) {
+            Mips_elf_abiflags_v0 abiflags;
+            if (eppnt->p_filesz < sizeof(Mips_elf_abiflags_v0)) {
+                errmsg = "Invalid PT_MIPS_ABIFLAGS entry";
+                goto exit_errmsg;
+            }
+            if (eppnt->p_offset + eppnt->p_filesz <= BPRM_BUF_SIZE) {
+                memcpy(&abiflags, bprm_buf + eppnt->p_offset,
+                       sizeof(Mips_elf_abiflags_v0));
+            } else {
+                retval = pread(image_fd, &abiflags, sizeof(Mips_elf_abiflags_v0),
+                               eppnt->p_offset);
+                if (retval != sizeof(Mips_elf_abiflags_v0)) {
+                    goto exit_perror;
+                }
+            }
+            bswap_mips_abiflags(&abiflags);
+#endif
         }
     }
 
-- 
1.9.1

[Qemu-devel] [PATCH 4/6] Read and set FP ABI value from MIPS abiflags

[Stefan Markovic]

From: Stefan Markovic <smarkovic@wavecomp.com>

Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
---
 linux-user/elfload.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 5881233..5bccd2e 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2396,6 +2396,7 @@ static void load_elf_image(const char *image_name, int image_fd,
                 }
             }
             bswap_mips_abiflags(&abiflags);
+            info->fp_abi = abiflags.fp_abi;
 #endif
         }
     }
@@ -2708,6 +2709,9 @@ int load_elf_binary(struct linux_binprm *bprm, struct image_info *info)
             target_mmap(0, qemu_host_page_size, PROT_READ | PROT_EXEC,
                         MAP_FIXED | MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
         }
+#ifdef TARGET_MIPS
+        info->interp_fp_abi = interp_info.fp_abi;
+#endif
     }
 
     bprm->p = create_elf_tables(bprm->p, bprm->argc, bprm->envc, &elf_ex,
-- 
1.9.1

[Qemu-devel] [PATCH 5/6] Determine the desired FPU mode

[Stefan Markovic]

From: Stefan Markovic <smarkovic@wavecomp.com>

Floating-point mode is calculated from MIPS.abiflags FP ABI value
(based on kernel implementation). Illegal combinations are rejected.

Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
---
 linux-user/mips/cpu_loop.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/linux-user/mips/cpu_loop.c b/linux-user/mips/cpu_loop.c
index c9c20cf..fd96e46 100644
--- a/linux-user/mips/cpu_loop.c
+++ b/linux-user/mips/cpu_loop.c
@@ -740,6 +740,34 @@ void target_cpu_copy_regs(CPUArchState *env, struct target_pt_regs *regs)
     struct image_info *info = ts->info;
     int i;
 
+    struct mode_req {
+        bool single;
+        bool soft;
+        bool fr1;
+        bool frdefault;
+        bool fre;
+    };
+
+    static const struct mode_req fpu_reqs[] = {
+        [MIPS_ABI_FP_ANY]    = { true,  true,  true,  true,  true  },
+        [MIPS_ABI_FP_DOUBLE] = { false, false, false, true,  true  },
+        [MIPS_ABI_FP_SINGLE] = { true,  false, false, false, false },
+        [MIPS_ABI_FP_SOFT]   = { false, true,  false, false, false },
+        [MIPS_ABI_FP_OLD_64] = { false, false, false, false, false },
+        [MIPS_ABI_FP_XX]     = { false, false, true,  true,  true  },
+        [MIPS_ABI_FP_64]     = { false, false, true,  false, false },
+        [MIPS_ABI_FP_64A]    = { false, false, true,  false, true  }
+    };
+
+    /*
+     * Mode requirements when .MIPS.abiflags is not present in the ELF.
+     * Not present means that everything is acceptable except FR1.
+     */
+    static struct mode_req none_req = { true, true, false, true, true };
+
+    struct mode_req prog_req;
+    struct mode_req interp_req;
+
     for(i = 0; i < 32; i++) {
         env->active_tc.gpr[i] = regs->regs[i];
     }
@@ -747,6 +775,53 @@ void target_cpu_copy_regs(CPUArchState *env, struct target_pt_regs *regs)
     if (regs->cp0_epc & 1) {
         env->hflags |= MIPS_HFLAG_M16;
     }
+
+#ifdef TARGET_ABI_MIPSO32
+# define MAX_FP_ABI MIPS_ABI_FP_64A
+#else
+# define MAX_FP_ABI MIPS_ABI_FP_SOFT
+#endif
+     if ((info->fp_abi > MAX_FP_ABI && info->fp_abi != MIPS_ABI_FP_UNKNOWN)
+        || (info->interp_fp_abi > MAX_FP_ABI &&
+            info->interp_fp_abi != MIPS_ABI_FP_UNKNOWN)) {
+        fprintf(stderr, "qemu: Program and interpreter have "
+                        "unexpected FPU modes\n");
+        exit(137);
+    }
+
+    prog_req = (info->fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
+                                            : fpu_reqs[info->fp_abi];
+    interp_req = (info->interp_fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
+                                            : fpu_reqs[info->interp_fp_abi];
+
+    prog_req.single &= interp_req.single;
+    prog_req.soft &= interp_req.soft;
+    prog_req.fr1 &= interp_req.fr1;
+    prog_req.frdefault &= interp_req.frdefault;
+    prog_req.fre &= interp_req.fre;
+
+    bool cpu_has_mips_r2_r6 = env->insn_flags & ISA_MIPS32R2 ||
+                              env->insn_flags & ISA_MIPS64R2 ||
+                              env->insn_flags & ISA_MIPS32R6 ||
+                              env->insn_flags & ISA_MIPS64R6;
+
+    if (prog_req.fre && !prog_req.frdefault && !prog_req.fr1) {
+        env->CP0_Config5 |= (1 << CP0C5_FRE);
+        if (env->active_fpu.fcr0 & (1 << FCR0_FREP)) {
+            env->hflags |= MIPS_HFLAG_FRE;
+        }
+    } else if ((prog_req.fr1 && prog_req.frdefault) ||
+         (prog_req.single && !prog_req.frdefault)) {
+        if ((env->active_fpu.fcr0 & (1 << FCR0_F64)
+            && cpu_has_mips_r2_r6) || prog_req.fr1) {
+            env->CP0_Status |= (1 << CP0St_FR);
+            env->hflags |= MIPS_HFLAG_F64;
+        }
+    } else  if (!prog_req.fre && !prog_req.frdefault &&
+          !prog_req.fr1 && !prog_req.single && !prog_req.soft) {
+        exit(137);
+    }
+
     if (env->insn_flags & ISA_NANOMIPS32) {
         return;
     }
-- 
1.9.1

[Qemu-devel] [PATCH 6/6] Add prctl() PR_SET_FP_MODE and PR_GET_FP_MODE implementations

[Stefan Markovic]

From: Stefan Markovic <smarkovic@wavecomp.com>

Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
---
 linux-user/mips/target_syscall.h   |  2 ++
 linux-user/mips64/target_syscall.h |  2 ++
 linux-user/syscall.c               | 62 +++++++++++++++++++++++++++++++++++---
 3 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/linux-user/mips/target_syscall.h b/linux-user/mips/target_syscall.h
index 33177af..fa075c9 100644
--- a/linux-user/mips/target_syscall.h
+++ b/linux-user/mips/target_syscall.h
@@ -247,5 +247,7 @@ static inline abi_ulong target_shmlba(CPUMIPSState *env)
 /* MIPS-specific prctl() options */
 #define TARGET_PR_SET_FP_MODE  45
 #define TARGET_PR_GET_FP_MODE  46
+#define TARGET_PR_FP_MODE_FR  (1 << 0)
+#define TARGET_PR_FP_MODE_FRE (1 << 1)
 
 #endif /* MIPS_TARGET_SYSCALL_H */
diff --git a/linux-user/mips64/target_syscall.h b/linux-user/mips64/target_syscall.h
index c1160e6..c8a9027 100644
--- a/linux-user/mips64/target_syscall.h
+++ b/linux-user/mips64/target_syscall.h
@@ -244,5 +244,7 @@ static inline abi_ulong target_shmlba(CPUMIPSState *env)
 /* MIPS-specific prctl() options */
 #define TARGET_PR_SET_FP_MODE  45
 #define TARGET_PR_GET_FP_MODE  46
+#define TARGET_PR_FP_MODE_FR  (1 << 0)
+#define TARGET_PR_FP_MODE_FRE (1 << 1)
 
 #endif /* MIPS64_TARGET_SYSCALL_H */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 15b03e1..810a58b 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9529,11 +9529,65 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
 #endif
 #ifdef TARGET_MIPS
         case TARGET_PR_GET_FP_MODE:
-            /* TODO: Implement TARGET_PR_SET_FP_MODE handling.*/
-            return -TARGET_EINVAL;
+        {
+            CPUMIPSState *env = ((CPUMIPSState *)cpu_env);
+            ret = 0;
+            if (env->CP0_Status & (1 << CP0St_FR)) {
+                ret |= TARGET_PR_FP_MODE_FR;
+            }
+            if (env->CP0_Config5 & (1 << CP0C5_FRE)) {
+                ret |= TARGET_PR_FP_MODE_FRE;
+            }
+            return ret;
+        }
         case TARGET_PR_SET_FP_MODE:
-            /* TODO: Implement TARGET_PR_GET_FP_MODE handling.*/
-            return -TARGET_EINVAL;
+        {
+            CPUMIPSState *env = ((CPUMIPSState *)cpu_env);
+            bool old_fr = env->CP0_Status & (1 << CP0St_FR);
+            bool new_fr = arg2 & TARGET_PR_FP_MODE_FR;
+            bool new_fre = arg2 & TARGET_PR_FP_MODE_FRE;
+
+            if (new_fr && !(env->active_fpu.fcr0 & (1 << FCR0_F64))) {
+                /* FR1 is not supported */
+                return -TARGET_EOPNOTSUPP;
+            }
+            if (!new_fr && (env->active_fpu.fcr0 & (1 << FCR0_F64))
+                && !(env->CP0_Status_rw_bitmask & (1 << CP0St_FR))) {
+                /* cannot set FR=0 */
+                return -TARGET_EOPNOTSUPP;
+            }
+            if (new_fre && !(env->active_fpu.fcr0 & (1 << FCR0_FREP))) {
+                /* Cannot set FRE=1 */
+                return -TARGET_EOPNOTSUPP;
+            }
+
+            int i;
+            fpr_t *fpr = env->active_fpu.fpr;
+            for (i = 0; i < 32 ; i += 2) {
+                if (!old_fr && new_fr) {
+                    fpr[i].w[!FP_ENDIAN_IDX] = fpr[i + 1].w[FP_ENDIAN_IDX];
+                } else if (old_fr && !new_fr) {
+                    fpr[i + 1].w[FP_ENDIAN_IDX] = fpr[i].w[!FP_ENDIAN_IDX];
+                }
+            }
+
+            if (new_fr) {
+                env->CP0_Status |= (1 << CP0St_FR);
+                env->hflags |= MIPS_HFLAG_F64;
+            } else {
+                env->CP0_Status &= ~(1 << CP0St_FR);
+            }
+            if (new_fre) {
+                env->CP0_Config5 |= (1 << CP0C5_FRE);
+                if (env->active_fpu.fcr0 & (1 << FCR0_FREP)) {
+                    env->hflags |= MIPS_HFLAG_FRE;
+                }
+            } else {
+                env->CP0_Config5 &= ~(1 << CP0C5_FRE);
+            }
+
+            return 0;
+        }
 #endif /* MIPS */
 #ifdef TARGET_AARCH64
         case TARGET_PR_SVE_SET_VL:
-- 
1.9.1

Re: [Qemu-devel] [PATCH 6/6] Add prctl() PR_SET_FP_MODE and PR_GET_FP_MODE implementations

[Aleksandar Markovic]

> From: Stefan Markovic <stefan.markovic@rt-rk.com>
> Subject: [PATCH 6/6] Add prctl() PR_SET_FP_MODE and PR_GET_FP_MODE implementations
> 
> From: Stefan Markovic <smarkovic@wavecomp.com>
> 
> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>

>  #define TARGET_PR_SET_FP_MODE  45
>  #define TARGET_PR_GET_FP_MODE  46
> +#define TARGET_PR_FP_MODE_FR  (1 << 0)
> +#define TARGET_PR_FP_MODE_FRE (1 << 1)

There should be one space more to the left of (1 << 0) and (1 << 1) to achieve nicer alignment.

A short commit message is needed too. Other than that:

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

Re: [Qemu-devel] [PATCH 5/6] Determine the desired FPU mode

[Aleksandar Markovic]

> Subject: [PATCH 5/6] Determine the desired FPU mode
> 
> From: Stefan Markovic <smarkovic@wavecomp.com>
> 
> Floating-point mode is calculated from MIPS.abiflags FP ABI value
> (based on kernel implementation). Illegal combinations are rejected.
> 
> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> ---

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

Re: [Qemu-devel] [PATCH 4/6] Read and set FP ABI value from MIPS abiflags

[Aleksandar Markovic]

> Subject: [PATCH 4/6] Read and set FP ABI value from MIPS abiflags
>
> From: Stefan Markovic <smarkovic@wavecomp.com>
> 
> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> ---

A short commit message is needed. Other than that:

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

Re: [Qemu-devel] [PATCH 3/6] Extract MIPS abiflags from ELF file

[Aleksandar Markovic]

> Subject: [PATCH 3/6] Extract MIPS abiflags from ELF file
> 
> From: Stefan Markovic <smarkovic@wavecomp.com>
> 
> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> ---

A brief commit message is needed. Other than that:

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

Re: [Qemu-devel] [PATCH 2/6] Extend image_info struct with MIPS specific fp_abi and interp_fp_abi fields

[Aleksandar Markovic]

> Subject: [PATCH 2/6] Extend image_info struct with MIPS specific fp_abi and interp_fp_abi fields
> 
> From: Stefan Markovic <smarkovic@wavecomp.com>
> 
> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> ---

A brief commit message is needed. Perhaps with the description of the role/purpose of two fields introduced in this patch. Other than that:

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

Re: [Qemu-devel] [PATCH 1/6] Define MIPS_ABI_FP_UNKNOWN macro

[Aleksandar Markovic]

> Subject: [PATCH 1/6] Define MIPS_ABI_FP_UNKNOWN macro
> 
> From: Stefan Markovic <smarkovic@wavecomp.com>
> 
> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> ---

A brief commit message is needed. From what kernel or glibc header is this constant copied? Other than that:

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

Re: [Qemu-devel] [PATCH 0/6] target/mips: Add support for prctl() PR_GET_FP_MODE and PR_SET_FP_MODE

[Aleksandar Markovic]

> Subject: [PATCH 0/6] target/mips: Add support for prctl() PR_GET_FP_MODE and PR_SET_FP_MODE
> 
> From: Stefan Markovic <smarkovic@wavecomp.com>
> 
> This series includes support for prctl() PR_GET_FP_MODE and PR_SET_FP_MODE.
> This requires extracting MIPS.abiflags section from ELF file and fp_abi value handling.
> 
> Stefan Markovic (6):
>   Define MIPS_ABI_FP_UNKNOWN macro
>   Extend image_info struct with MIPS specific fp_abi and interp_fp_abi fields
>   Extract MIPS abiflags from ELF file
>   Read and set FP ABI value from MIPS abiflags
>   Determine the desired FPU mode
>   Add prctl() PR_SET_FP_MODE and PR_GET_FP_MODE implementations
> 
>  include/elf.h                      |  2 +
>  linux-user/elfload.c               | 37 +++++++++++++++++++
>  linux-user/mips/cpu_loop.c         | 75 ++++++++++++++++++++++++++++++++++++++
>  linux-user/mips/target_syscall.h   |  2 +
>  linux-user/mips64/target_syscall.h |  2 +
>  linux-user/qemu.h                  |  4 ++
>  linux-user/syscall.c               | 62 +++++++++++++++++++++++++++++--
>  7 files changed, 180 insertions(+), 4 deletions(-)
> 
> --
> 1.9.1
> 


Hi, Laurent,

Here is a mini-series about a MIPS-specific linux-user feature. Its code is almost entirely either in MIPS-specific files or under "#ifdef MIPS" directives. I think it makes sense that I should be able to integrate them via MIPS queue. Please let me know if you object to this.

You probably wonder why so much code for such obscure feature. The answer is that the solution needs to follow and mimic corresponding solution in MIPS-specific parts of the kernel, and its complexity is determined by that.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH 5/6] Determine the desired FPU mode

[Peter Maydell]

On 26 October 2018 at 15:21, Stefan Markovic <stefan.markovic@rt-rk.com> wrote:
> From: Stefan Markovic <smarkovic@wavecomp.com>
>
> Floating-point mode is calculated from MIPS.abiflags FP ABI value
> (based on kernel implementation). Illegal combinations are rejected.
>
> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> ---
>  linux-user/mips/cpu_loop.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)

> +     if ((info->fp_abi > MAX_FP_ABI && info->fp_abi != MIPS_ABI_FP_UNKNOWN)
> +        || (info->interp_fp_abi > MAX_FP_ABI &&
> +            info->interp_fp_abi != MIPS_ABI_FP_UNKNOWN)) {
> +        fprintf(stderr, "qemu: Program and interpreter have "
> +                        "unexpected FPU modes\n");
> +        exit(137);

Why are we exit()ing with a funny exit status code here?

If this is a "can't happen" case, then we should assert(). If
it is a "can happen if fed an odd binary" case, then we should just
exit(1) as we do already in this function for an unsupported NaN mode.

> +    }
> +
> +    prog_req = (info->fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
> +                                            : fpu_reqs[info->fp_abi];
> +    interp_req = (info->interp_fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
> +                                            : fpu_reqs[info->interp_fp_abi];
> +
> +    prog_req.single &= interp_req.single;
> +    prog_req.soft &= interp_req.soft;
> +    prog_req.fr1 &= interp_req.fr1;
> +    prog_req.frdefault &= interp_req.frdefault;
> +    prog_req.fre &= interp_req.fre;
> +
> +    bool cpu_has_mips_r2_r6 = env->insn_flags & ISA_MIPS32R2 ||
> +                              env->insn_flags & ISA_MIPS64R2 ||
> +                              env->insn_flags & ISA_MIPS32R6 ||
> +                              env->insn_flags & ISA_MIPS64R6;
> +
> +    if (prog_req.fre && !prog_req.frdefault && !prog_req.fr1) {
> +        env->CP0_Config5 |= (1 << CP0C5_FRE);
> +        if (env->active_fpu.fcr0 & (1 << FCR0_FREP)) {
> +            env->hflags |= MIPS_HFLAG_FRE;
> +        }
> +    } else if ((prog_req.fr1 && prog_req.frdefault) ||
> +         (prog_req.single && !prog_req.frdefault)) {
> +        if ((env->active_fpu.fcr0 & (1 << FCR0_F64)
> +            && cpu_has_mips_r2_r6) || prog_req.fr1) {
> +            env->CP0_Status |= (1 << CP0St_FR);
> +            env->hflags |= MIPS_HFLAG_F64;
> +        }
> +    } else  if (!prog_req.fre && !prog_req.frdefault &&
> +          !prog_req.fr1 && !prog_req.single && !prog_req.soft) {
> +        exit(137);
> +    }

Ditto here (and we haven't printed any error message here...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/6] Determine the desired FPU mode

[Aleksandar Markovic]

> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: [Qemu-devel] [PATCH 5/6] Determine the desired FPU mode
> 
> On 26 October 2018 at 15:21, Stefan Markovic <stefan.markovic@rt-rk.com> wrote:
> > From: Stefan Markovic <smarkovic@wavecomp.com>
> >
> > Floating-point mode is calculated from MIPS.abiflags FP ABI value
> > (based on kernel implementation). Illegal combinations are rejected.
> >
> > Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> > ---
> >  linux-user/mips/cpu_loop.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> 
> > +     if ((info->fp_abi > MAX_FP_ABI && info->fp_abi != MIPS_ABI_FP_UNKNOWN)
> > +        || (info->interp_fp_abi > MAX_FP_ABI &&
> > +            info->interp_fp_abi != MIPS_ABI_FP_UNKNOWN)) {
> > +        fprintf(stderr, "qemu: Program and interpreter have "
> > +                        "unexpected FPU modes\n");
> > +        exit(137);
> 
> Why are we exit()ing with a funny exit status code here?
> 
> If this is a "can't happen" case, then we should assert(). If
> it is a "can happen if fed an odd binary" case, then we should just
> exit(1) as we do already in this function for an unsupported NaN mode.
> 

Thanks for the review.

This is a "can happen if fed an odd binary" case. Or, in other words, and more precisely, an executable compiled with one FP option attempts to load a library compiled with another, incompatible, FP option.

Kernel counterpart lines are:

https://elixir.bootlin.com/linux/v4.19/source/arch/mips/kernel/elf.c#L211
https://elixir.bootlin.com/linux/v4.19/source/arch/mips/kernel/elf.c#L263

I think the error code is important for MIPS loader to work as designed in such cases. Stefan should be best positioned to explain and analyze the cases, since he worked on verifying and fixing involved scenarios, not only from QEMU perspective. However, he will be back most likely only on Monday.

Thanks again,
Aleksandar

> > +    }
> > +
> > +    prog_req = (info->fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
> > +                                            : fpu_reqs[info->fp_abi];
> > +    interp_req = (info->interp_fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
> > +                                            : fpu_reqs[info->interp_fp_abi];
> > +
> > +    prog_req.single &= interp_req.single;
> > +    prog_req.soft &= interp_req.soft;
> > +    prog_req.fr1 &= interp_req.fr1;
> > +    prog_req.frdefault &= interp_req.frdefault;
> > +    prog_req.fre &= interp_req.fre;
> > +
> > +    bool cpu_has_mips_r2_r6 = env->insn_flags & ISA_MIPS32R2 ||
> > +                              env->insn_flags & ISA_MIPS64R2 ||
> > +                              env->insn_flags & ISA_MIPS32R6 ||
> > +                              env->insn_flags & ISA_MIPS64R6;
> > +
> > +    if (prog_req.fre && !prog_req.frdefault && !prog_req.fr1) {
> > +        env->CP0_Config5 |= (1 << CP0C5_FRE);
> > +        if (env->active_fpu.fcr0 & (1 << FCR0_FREP)) {
> > +            env->hflags |= MIPS_HFLAG_FRE;
> > +        }
> > +    } else if ((prog_req.fr1 && prog_req.frdefault) ||
> > +         (prog_req.single && !prog_req.frdefault)) {
> > +        if ((env->active_fpu.fcr0 & (1 << FCR0_F64)
> > +            && cpu_has_mips_r2_r6) || prog_req.fr1) {
> > +            env->CP0_Status |= (1 << CP0St_FR);
> > +            env->hflags |= MIPS_HFLAG_F64;
> > +        }
> > +    } else  if (!prog_req.fre && !prog_req.frdefault &&
> > +          !prog_req.fr1 && !prog_req.single && !prog_req.soft) {
> > +        exit(137);
> > +    }
> 
> Ditto here (and we haven't printed any error message here...)
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] ?==?utf-8?q? ?==?utf-8?q? [PATCH 5/6] Determine the desired FPU mode

[Stefan Markovic]

exit() error codes are taken and left over from related kernel code. Will be set to 1 in next series version. Also, appropriate error messages printing will be added.

Regards,
Stefan

-------- Original Message --------
Subject: Re: [Qemu-devel] [PATCH 5/6] Determine the desired FPU mode
Date: Friday, October 26, 2018 20:12 CEST
From: Peter Maydell <peter.maydell@linaro.org>
To: Stefan Markovic <stefan.markovic@rt-rk.com>
CC: QEMU Developers <qemu-devel@nongnu.org>, Petar Jovanovic <pjovanovic@wavecomp.com>, Riku Voipio <riku.voipio@iki.fi>, Aleksandar Markovic <amarkovic@wavecomp.com>, Aurelien Jarno <aurelien@aurel32.net>, Laurent Vivier <laurent@vivier.eu>
References: <1540563667-23300-1-git-send-email-stefan.markovic@rt-rk.com> <1540563667-23300-6-git-send-email-stefan.markovic@rt-rk.com>


 On 26 October 2018 at 15:21, Stefan Markovic <stefan.markovic@rt-rk.com> wrote:
> From: Stefan Markovic <smarkovic@wavecomp.com>
>
> Floating-point mode is calculated from MIPS.abiflags FP ABI value
> (based on kernel implementation). Illegal combinations are rejected.
>
> Signed-off-by: Stefan Markovic <smarkovic@wavecomp.com>
> ---
> linux-user/mips/cpu_loop.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 75 insertions(+)

> + if ((info->fp_abi > MAX_FP_ABI && info->fp_abi != MIPS_ABI_FP_UNKNOWN)
> + || (info->interp_fp_abi > MAX_FP_ABI &&
> + info->interp_fp_abi != MIPS_ABI_FP_UNKNOWN)) {
> + fprintf(stderr, "qemu: Program and interpreter have "
> + "unexpected FPU modes\n");
> + exit(137);

Why are we exit()ing with a funny exit status code here?

If this is a "can't happen" case, then we should assert(). If
it is a "can happen if fed an odd binary" case, then we should just
exit(1) as we do already in this function for an unsupported NaN mode.

> + }
> +
> + prog_req = (info->fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
> + : fpu_reqs[info->fp_abi];
> + interp_req = (info->interp_fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
> + : fpu_reqs[info->interp_fp_abi];
> +
> + prog_req.single &= interp_req.single;
> + prog_req.soft &= interp_req.soft;
> + prog_req.fr1 &= interp_req.fr1;
> + prog_req.frdefault &= interp_req.frdefault;
> + prog_req.fre &= interp_req.fre;
> +
> + bool cpu_has_mips_r2_r6 = env->insn_flags & ISA_MIPS32R2 ||
> + env->insn_flags & ISA_MIPS64R2 ||
> + env->insn_flags & ISA_MIPS32R6 ||
> + env->insn_flags & ISA_MIPS64R6;
> +
> + if (prog_req.fre && !prog_req.frdefault && !prog_req.fr1) {
> + env->CP0_Config5 |= (1 << CP0C5_FRE);
> + if (env->active_fpu.fcr0 & (1 << FCR0_FREP)) {
> + env->hflags |= MIPS_HFLAG_FRE;
> + }
> + } else if ((prog_req.fr1 && prog_req.frdefault) ||
> + (prog_req.single && !prog_req.frdefault)) {
> + if ((env->active_fpu.fcr0 & (1 << FCR0_F64)
> + && cpu_has_mips_r2_r6) || prog_req.fr1) {
> + env->CP0_Status |= (1 << CP0St_FR);
> + env->hflags |= MIPS_HFLAG_F64;
> + }
> + } else if (!prog_req.fre && !prog_req.frdefault &&
> + !prog_req.fr1 && !prog_req.single && !prog_req.soft) {
> + exit(137);
> + }

Ditto here (and we haven't printed any error message here...)

thanks
-- PMM