qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 26/40] target/mips: Implement CP0 Config0.WR


From: Richard Henderson
Subject: Re: [Qemu-devel] [PATCH v3 26/40] target/mips: Implement CP0 Config0.WR bit functionality
Date: Mon, 23 Jul 2018 09:46:58 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

On 07/19/2018 05:54 AM, Stefan Markovic wrote:
> From: Stefan Markovic <address@hidden>
> 
> Add testing Config0.WR bit into watch exception handling logic.

Config1, here and in the subject.

> 
> Signed-off-by: Aleksandar Markovic <address@hidden>
> Signed-off-by: Stefan Markovic <address@hidden>
> ---
>  target/mips/helper.c    | 12 +++++++++++-
>  target/mips/translate.c | 22 ++++++++++++++++------
>  2 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/target/mips/helper.c b/target/mips/helper.c
> index 9535131..dc8f2a5 100644
> --- a/target/mips/helper.c
> +++ b/target/mips/helper.c
> @@ -749,6 +749,14 @@ void mips_cpu_do_interrupt(CPUState *cs)
>          (env->hflags & MIPS_HFLAG_DM)) {
>          cs->exception_index = EXCP_DINT;
>      }
> +
> +    if ((cs->exception_index == EXCP_DWATCH ||
> +        cs->exception_index == EXCP_DFWATCH ||
> +        cs->exception_index == EXCP_IWATCH) &&
> +        (env->CP0_Config1 & (1 << CP0C1_WR))) {
> +        cs->exception_index = EXCP_NONE;
> +    }

This will cause the switch below to abort.

In any case, I think this test is a mistake -- you should have (and probably
did, given a presumed lack of abort during testing) prevent these exceptions
from being triggered in the first place.

>      case EXCP_SRESET:
>          env->CP0_Status |= (1 << CP0St_SR);
> -        memset(env->CP0_WatchLo, 0, sizeof(env->CP0_WatchLo));
> +        if (env->CP0_Config1 & (1 << CP0C1_WR)) {
> +            memset(env->CP0_WatchLo, 0, sizeof(env->CP0_WatchLo));
> +        }

If it's unused/missing, why does it hurt to reset to 0?

> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -5622,6 +5622,7 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 5:
>          case 6:
>          case 7:
> +            CP0_CHECK(ctx->CP0_Config1 & (1 << CP0C1_WR));
>              gen_helper_1e0i(mfc0_watchlo, arg, sel);
>              rn = "WatchLo";
>              break;
> @@ -5639,6 +5640,7 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 5:
>          case 6:
>          case 7:
> +            CP0_CHECK(ctx->CP0_Config1 & (1 << CP0C1_WR));
>              gen_helper_1e0i(mfc0_watchhi, arg, sel);
>              rn = "WatchHi";
>              break;
> @@ -6321,6 +6323,7 @@ static void gen_mtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 5:
>          case 6:
>          case 7:
> +            CP0_CHECK(ctx->CP0_Config1 & (1 << CP0C1_WR));
>              gen_helper_0e1i(mtc0_watchlo, arg, sel);
>              rn = "WatchLo";
>              break;
> @@ -6338,6 +6341,7 @@ static void gen_mtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 5:
>          case 6:
>          case 7:
> +            CP0_CHECK(ctx->CP0_Config1 & (1 << CP0C1_WR));
>              gen_helper_0e1i(mtc0_watchhi, arg, sel);
>              rn = "WatchHi";
>              break;
> @@ -7024,6 +7028,7 @@ static void gen_dmfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 5:
>          case 6:
>          case 7:
> +            CP0_CHECK(ctx->CP0_Config1 & (1 << CP0C1_WR));
>              gen_helper_1e0i(dmfc0_watchlo, arg, sel);
>              rn = "WatchLo";
>              break;
> @@ -7041,6 +7046,7 @@ static void gen_dmfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 5:
>          case 6:
>          case 7:
> +            CP0_CHECK(ctx->CP0_Config1 & (1 << CP0C1_WR));
>              gen_helper_1e0i(mfc0_watchhi, arg, sel);
>              rn = "WatchHi";
>              break;
> @@ -7705,6 +7711,7 @@ static void gen_dmtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 5:
>          case 6:
>          case 7:
> +            CP0_CHECK(ctx->CP0_Config1 & (1 << CP0C1_WR));
>              gen_helper_0e1i(mtc0_watchlo, arg, sel);
>              rn = "WatchLo";
>              break;
> @@ -7722,6 +7729,7 @@ static void gen_dmtc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 5:
>          case 6:
>          case 7:
> +            CP0_CHECK(ctx->CP0_Config1 & (1 << CP0C1_WR));
>              gen_helper_0e1i(mtc0_watchhi, arg, sel);
>              rn = "WatchHi";
>              break;

These are probably the only changes required -- preventing the missing
registers from being set at runtime.


> @@ -25281,14 +25289,16 @@ void cpu_state_reset(CPUMIPSState *env)
>         no performance counters. */
>      env->CP0_IntCtl = 0xe0000000;
>      {
> -        int i;
> +        if (env->CP0_Config1 & (1 << CP0C1_WR)) {
> +            int i;
>  
> -        for (i = 0; i < 7; i++) {
> -            env->CP0_WatchLo[i] = 0;
> -            env->CP0_WatchHi[i] = 0x80000000;
> +            for (i = 0; i < 7; i++) {
> +                env->CP0_WatchLo[i] = 0;
> +                env->CP0_WatchHi[i] = 0x80000000;
> +            }
> +            env->CP0_WatchLo[7] = 0;
> +            env->CP0_WatchHi[7] = 0;
>          }
> -        env->CP0_WatchLo[7] = 0;
> -        env->CP0_WatchHi[7] = 0;
>      }

Again, what difference does the reset value make?


r~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]