[Qemu-devel] [PATCH v3 8/8] target/mips: Check ELPA flag only in some cases of MFHC0 and MTHC0

Aleksandar Markovic posted 8 patches 7 years, 4 months ago
[Qemu-devel] [PATCH v3 8/8] target/mips: Check ELPA flag only in some cases of MFHC0 and MTHC0
Posted by Aleksandar Markovic 7 years, 4 months ago
From: Yongbok Kim <yongbok.kim@mips.com>

MFHC0 and MTHC0 used to handle EntryLo0 and EntryLo1 registers only,
and placing ELPA flag checks befor switch statement were technically
correct. However, after adding handling more registers, these checks
should be moved to act only in cases of handling EntryLo0 and
EntryLo1.

Signed-off-by: Yongbok Kim <yongbok.kim@mips.com>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
---
 target/mips/translate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index fa1da7e..c01cafe 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -4885,12 +4885,11 @@ static void gen_mfhc0(DisasContext *ctx, TCGv arg, int reg, int sel)
 {
     const char *rn = "invalid";
 
-    CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
-
     switch (reg) {
     case 2:
         switch (sel) {
         case 0:
+            CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
             gen_mfhc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo0));
             rn = "EntryLo0";
             break;
@@ -4901,6 +4900,7 @@ static void gen_mfhc0(DisasContext *ctx, TCGv arg, int reg, int sel)
     case 3:
         switch (sel) {
         case 0:
+            CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
             gen_mfhc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo1));
             rn = "EntryLo1";
             break;
@@ -4965,12 +4965,11 @@ static void gen_mthc0(DisasContext *ctx, TCGv arg, int reg, int sel)
     const char *rn = "invalid";
     uint64_t mask = ctx->PAMask >> 36;
 
-    CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
-
     switch (reg) {
     case 2:
         switch (sel) {
         case 0:
+            CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
             tcg_gen_andi_tl(arg, arg, mask);
             gen_mthc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo0));
             rn = "EntryLo0";
@@ -4982,6 +4981,7 @@ static void gen_mthc0(DisasContext *ctx, TCGv arg, int reg, int sel)
     case 3:
         switch (sel) {
         case 0:
+            CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
             tcg_gen_andi_tl(arg, arg, mask);
             gen_mthc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo1));
             rn = "EntryLo1";
-- 
2.7.4


Re: [Qemu-devel] [PATCH v3 8/8] target/mips: Check ELPA flag only in some cases of MFHC0 and MTHC0
Posted by Aleksandar Markovic 7 years, 4 months ago
> From: Yongbok Kim <yongbok.kim@mips.com>
>
> MFHC0 and MTHC0 used to handle EntryLo0 and EntryLo1 registers only,
> and placing ELPA flag checks befor switch statement were technically
> correct. However, after adding handling more registers, these checks
> should be moved to act only in cases of handling EntryLo0 and
> EntryLo1.
>
> Signed-off-by: Yongbok Kim <yongbok.kim@mips.com>
> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> ---
>  target/mips/translate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

(ctx->hflags & MIPS_HFLAG_ELPA) is set to be a flag that depends on both
LPA and ELPA. This does not seem to me the best way to do this - I would rather have separate bits for this and similar occasions. It would be easier to check the code against the documentation - and that is more imortant IMHO than having spent extra bit, or having slightly more to check in tha tcode. But, this is ooutside of the scope of this patch. The outcome of the patch seems to be in accirdance with the documentation. So:

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

>
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index fa1da7e..c01cafe 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -4885,12 +4885,11 @@ static void gen_mfhc0(DisasContext *ctx, TCGv arg, int > reg, int sel)
>  {
>      const char *rn = "invalid";
>
> -    CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
> -
>      switch (reg) {
>      case 2:
>          switch (sel) {
>          case 0:
> +            CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
>              gen_mfhc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo0));
>              rn = "EntryLo0";
>              break;
> @@ -4901,6 +4900,7 @@ static void gen_mfhc0(DisasContext *ctx, TCGv arg, int reg, > int sel)
>      case 3:
>          switch (sel) {
>          case 0:
> +            CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
>              gen_mfhc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo1));
>              rn = "EntryLo1";
>              break;
> @@ -4965,12 +4965,11 @@ static void gen_mthc0(DisasContext *ctx, TCGv arg, int > reg, int sel)
>      const char *rn = "invalid";
>      uint64_t mask = ctx->PAMask >> 36;
>
> -    CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
> -
>      switch (reg) {
>      case 2:
>          switch (sel) {
>          case 0:
> +            CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
>              tcg_gen_andi_tl(arg, arg, mask);
>              gen_mthc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo0));
>              rn = "EntryLo0";
> @@ -4982,6 +4981,7 @@ static void gen_mthc0(DisasContext *ctx, TCGv arg, int reg, > int sel)
>      case 3:
>          switch (sel) {
>          case 0:
> +            CP0_CHECK(ctx->hflags & MIPS_HFLAG_ELPA);
>              tcg_gen_andi_tl(arg, arg, mask);
>              gen_mthc0_entrylo(arg, offsetof(CPUMIPSState, CP0_EntryLo1));
>              rn = "EntryLo1";
> --
> 2.7.4