Re: [Qemu-devel] [PATCH] target-mips: Tighten ISA level checks

2014-12-03 Thread Leon Alrae
On 19/11/2014 14:20, Maciej W. Rozycki wrote:
 -env-hflags |= MIPS_HFLAG_DM | MIPS_HFLAG_64 | MIPS_HFLAG_CP0;
 +if (env-insn_flags  ISA_MIPS3)
 +env-hflags |= MIPS_HFLAG_64;

According to the CODING_STYLE braces are required even for a single
statement if block. This also applies in other places in this patch.

Otherwise,

Reviewed-by: Leon Alrae leon.al...@imgtec.com




Re: [Qemu-devel] [PATCH] target-mips: Tighten ISA level checks

2014-12-03 Thread Maciej W. Rozycki
On Wed, 3 Dec 2014, Leon Alrae wrote:

  -env-hflags |= MIPS_HFLAG_DM | MIPS_HFLAG_64 | MIPS_HFLAG_CP0;
  +if (env-insn_flags  ISA_MIPS3)
  +env-hflags |= MIPS_HFLAG_64;
 
 According to the CODING_STYLE braces are required even for a single
 statement if block. This also applies in other places in this patch.

 These slipped through somehow, I must have forgotten to run checkpatch 
on this change.  Resending an updated version right away, thanks for 
catching this up and for your review.

  Maciej



[Qemu-devel] [PATCH] target-mips: Tighten ISA level checks

2014-11-19 Thread Maciej W. Rozycki
Tighten ISA level checks down to MIPS II that many of our instructions
are missing.  Also make sure any 64-bit instruction enables are only
applied to 64-bit processors, that is ones that implement at least the
MIPS III ISA.

Signed-off-by: Maciej W. Rozycki ma...@codesourcery.com
---
Hi,

 As usually with changes that require manual code review I may have 
missed something here, but I believe I did not.  I went thoroughly 
through all the code and checked all the instructions.  Of course that 
was before MIPSr6, but I do hope for the latter the right checks have 
been implemented from the beginning.

 I rigorously tested this change by running full GCC, G++ and glibc 
mips-linux-gnu toolchain test suites under Linux (Debian Wheezy) run in 
QEMU in the system emulation mode, for the following multilibs:

-EB
-EB -mips16
-EB -mmicromips
-EB -mabi=n32
-EB -mabi=64

and the -EL variants of same.  Of these standard MIPS o32 testing was 
run on a 24Kf processor and n64/n32 testing was run on a 5KEf processor, 
using a 32-bit and a 64-bit kernel respectively.  MIPS16 o32 testing was 
run on an artificial 5KEf-mips16 processor -- like a real 5KEf one, but 
with the MIPS16 ASE enabled, and a 64-bit kernel.  Finally microMIPS o32 
testing was run on an artificial 24Kf-micromips processor -- like a real 
24Kf one, but with the microMIPS ASE enabled, and a 32-bit kernel built 
as microMIPS code itself.

 That should have executed enough paths in the emulator, both for user 
instructions and privileged (CP0) instructions, as both the user side 
and the kernel side were tested at the same time.

 The original test result counts were as follows:

Result   Count
ERROR   20
FAIL   300
PASS   1732076
XPASS  335
XFAIL 6527
UNRESOLVED  26
UNSUPPORTED  50854
Total  1790138

After the change they were:

Result   Count
ERROR   20
FAIL   303
PASS   1732073
XPASS  336
XFAIL 6526
UNRESOLVED  26
UNSUPPORTED  50854
Total  1790138

The changes in results were as follows:

PASS  - FAIL:  qemu-64/glibc.sum:malloc/tst-mallocfork
PASS  - FAIL:  qemu-mips16/glibc.sum:nptl/tst-setuid3
PASS  - FAIL:  qemu-n32-el/glibc.sum:malloc/tst-malloc-usable
PASS  - FAIL:  qemu-n32/glibc.sum:   nptl/tst-cancel17
FAIL  - PASS:  qemu-micromips/glibc.sum: nptl/tst-setuid3
XFAIL - XPASS: qemu-micromips/glibc.sum: nptl/tst-cancel7

-- which I qualify as intermittent failures that unfortunately happen 
all the time, also on real hardware, so they do not appear to be issues 
arising from this change to QEMU.

 I believe this satisfies quality verification requirements for such an 
extensive change.  Please apply.

  Maciej

qemu-mips-isa-check.diff
Index: qemu-git-trunk/target-mips/cpu.h
===
--- qemu-git-trunk.orig/target-mips/cpu.h   2014-11-12 07:38:07.577674675 
+
+++ qemu-git-trunk/target-mips/cpu.h2014-11-12 07:38:24.077713983 +
@@ -831,9 +831,10 @@ static inline void compute_hflags(CPUMIP
 env-hflags |= (env-CP0_Status  CP0St_KSU)  MIPS_HFLAG_KSU;
 }
 #if defined(TARGET_MIPS64)
-if (((env-hflags  MIPS_HFLAG_KSU) != MIPS_HFLAG_UM) ||
-(env-CP0_Status  (1  CP0St_PX)) ||
-(env-CP0_Status  (1  CP0St_UX))) {
+if ((env-insn_flags  ISA_MIPS3) 
+(((env-hflags  MIPS_HFLAG_KSU) != MIPS_HFLAG_UM) ||
+ (env-CP0_Status  (1  CP0St_PX)) ||
+ (env-CP0_Status  (1  CP0St_UX {
 env-hflags |= MIPS_HFLAG_64;
 }
 
Index: qemu-git-trunk/target-mips/helper.c
===
--- qemu-git-trunk.orig/target-mips/helper.c2014-11-12 07:33:08.548361020 
+
+++ qemu-git-trunk/target-mips/helper.c 2014-11-12 07:38:24.077713983 +
@@ -527,7 +527,9 @@ void mips_cpu_do_interrupt(CPUState *cs)
 env-CP0_DEPC = exception_resume_pc(env);
 env-hflags = ~MIPS_HFLAG_BMASK;
  enter_debug_mode:
-env-hflags |= MIPS_HFLAG_DM | MIPS_HFLAG_64 | MIPS_HFLAG_CP0;
+if (env-insn_flags  ISA_MIPS3)
+env-hflags |= MIPS_HFLAG_64;
+env-hflags |= MIPS_HFLAG_DM | MIPS_HFLAG_CP0;
 env-hflags = ~(MIPS_HFLAG_KSU);
 /* EJTAG probe trap enable is not implemented... */
 if (!(env-CP0_Status  (1  CP0St_EXL)))
@@ -548,7 +550,9 @@ void mips_cpu_do_interrupt(CPUState *cs)
 env-CP0_ErrorEPC = exception_resume_pc(env);
 env-hflags = ~MIPS_HFLAG_BMASK;
 env-CP0_Status |= (1  CP0St_ERL) | (1  CP0St_BEV);
-env-hflags |= MIPS_HFLAG_64 | MIPS_HFLAG_CP0;
+if (env-insn_flags  ISA_MIPS3)
+env-hflags |= MIPS_HFLAG_64;
+env-hflags |= MIPS_HFLAG_CP0;
 env-hflags = ~(MIPS_HFLAG_KSU);
 if (!(env-CP0_Status  (1  CP0St_EXL)))
 env-CP0_Cause = ~(1U  CP0Ca_BD);
@@ -726,7 +730,9 @@