Re: [Qemu-devel] [PATCH v3 5/8] target/mips: Add CP0 BadInstrX register

[Stefan Markovic]

Hi Philippe,


With solved build issues caused by solution previously proposed, this would be 
the final fix:


--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -5354,6 +5354,11 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
         case 3:
             CP0_CHECK(ctx->bi);
             gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_BadInstrX));
+#if defined(TARGET_MIPS64)
+            tcg_gen_andi_i64(arg, arg, ~0xffff);
+#else
+            tcg_gen_andi_i32(arg, arg, ~0xffff);
+#endif
             rn = "BadInstrX";
             break;
         default:
@@ -6719,6 +6724,11 @@ static void gen_dmfc0(DisasContext *ctx, TCGv arg, int 
reg, int sel)
         case 3:
             CP0_CHECK(ctx->bi);
             gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_BadInstrX));
+#if defined(TARGET_MIPS64)
+            tcg_gen_andi_i64(arg, arg, ~0xffff);
+#else
+            tcg_gen_andi_i32(arg, arg, ~0xffff);
+#endif
             rn = "BadInstrX";
             break;
         default:



Regards,

Stefan

________________________________
From: Philippe Mathieu-Daudé <address@hidden> on behalf of Philippe 
Mathieu-Daudé <address@hidden>
Sent: Thursday, July 5, 2018 5:13:42 PM
To: Stefan Markovic; Aleksandar Markovic; address@hidden; Richard Henderson
Cc: Petar Jovanovic; Aleksandar Markovic; address@hidden; Paul Burton
Subject: Re: [Qemu-devel] [PATCH v3 5/8] target/mips: Add CP0 BadInstrX register

On 07/05/2018 10:27 AM, Stefan Markovic wrote:
> Hi Philippe,
>
> Following fix will be added in v4:
>
>
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 79a59fd..98ff8d0 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -5354,6 +5354,7 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 3:
>              CP0_CHECK(ctx->bi);
>              gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_BadInstrX));
> +            tcg_gen_andi_i32(arg, arg, 0xffff0000);

Correct, maybe easier to read as:

tcg_gen_andi_i32(arg, arg, ~0xffff);

With these changes in your v4:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>              rn = "BadInstrX";
>              break;
>          default:
> @@ -6719,6 +6720,7 @@ static void gen_dmfc0(DisasContext *ctx, TCGv arg, int 
> reg, int sel)
>          case 3:
>              CP0_CHECK(ctx->bi);
>              gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_BadInstrX));
> +            tcg_gen_andi_i32(arg, arg, 0xffff0000);
>              rn = "BadInstrX";
>              break;
>          default:
>
>
> Regards,
> Stefan
>
>
> ________________________________
> From: Philippe Mathieu-Daudé <address@hidden> on behalf of Philippe 
> Mathieu-Daudé <address@hidden>
> Sent: Wednesday, July 4, 2018 10:31:27 PM
> To: Aleksandar Markovic; address@hidden; Richard Henderson
> Cc: address@hidden; Aleksandar Markovic; Stefan Markovic; Petar Jovanovic; 
> Paul Burton
> Subject: Re: [PATCH v3 5/8] target/mips: Add CP0 BadInstrX register
>
> Hi Aleksandar,
>
> On 07/04/2018 04:30 PM, Aleksandar Markovic wrote:
>> From: Stefan Markovic <address@hidden>
>>
>> Add CP0 BadInstrX register. This register will be used in nanoMIPS.
>>
>> Signed-off-by: Stefan Markovic <address@hidden>
>> Signed-off-by: Yongbok Kim <address@hidden>
>> Signed-off-by: Aleksandar Markovic <address@hidden>
>> Reviewed-by: Aleksandar Markovic <address@hidden>
>> ---
>>  target/mips/cpu.h       |  1 +
>>  target/mips/machine.c   |  5 +++--
>>  target/mips/translate.c | 20 +++++++++++++++++++-
>>  3 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
>> index edbb66d..8ccbc21 100644
>> --- a/target/mips/cpu.h
>> +++ b/target/mips/cpu.h
>> @@ -323,6 +323,7 @@ struct CPUMIPSState {
>>      target_ulong CP0_BadVAddr;
>>      uint32_t CP0_BadInstr;
>>      uint32_t CP0_BadInstrP;
>> +    uint32_t CP0_BadInstrX;
>>      int32_t CP0_Count;
>>      target_ulong CP0_EntryHi;
>>  #define CP0EnHi_EHINV 10
>> diff --git a/target/mips/machine.c b/target/mips/machine.c
>> index 20100d5..5ba78ac 100644
>> --- a/target/mips/machine.c
>> +++ b/target/mips/machine.c
>> @@ -212,8 +212,8 @@ const VMStateDescription vmstate_tlb = {
>>
>>  const VMStateDescription vmstate_mips_cpu = {
>>      .name = "cpu",
>> -    .version_id = 10,
>> -    .minimum_version_id = 10,
>> +    .version_id = 11,
>> +    .minimum_version_id = 11,
>>      .post_load = cpu_post_load,
>>      .fields = (VMStateField[]) {
>>          /* Active TC */
>> @@ -266,6 +266,7 @@ const VMStateDescription vmstate_mips_cpu = {
>>          VMSTATE_UINTTL(env.CP0_BadVAddr, MIPSCPU),
>>          VMSTATE_UINT32(env.CP0_BadInstr, MIPSCPU),
>>          VMSTATE_UINT32(env.CP0_BadInstrP, MIPSCPU),
>> +        VMSTATE_UINT32(env.CP0_BadInstrX, MIPSCPU),
>>          VMSTATE_INT32(env.CP0_Count, MIPSCPU),
>>          VMSTATE_UINTTL(env.CP0_EntryHi, MIPSCPU),
>>          VMSTATE_INT32(env.CP0_Compare, MIPSCPU),
>> diff --git a/target/mips/translate.c b/target/mips/translate.c
>> index 88699ae..0562851 100644
>> --- a/target/mips/translate.c
>> +++ b/target/mips/translate.c
>> @@ -5315,7 +5315,12 @@ static void gen_mfc0(DisasContext *ctx, TCGv arg, int 
>> reg, int sel)
>>              gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_BadInstrP));
>>              rn = "BadInstrP";
>>              break;
>> -        default:
>> +        case 3:
>> +            CP0_CHECK(ctx->bi);
>> +            gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_BadInstrX));
>> +            rn = "BadInstrX";
>> +            break;
>> +       default:
>>              goto cp0_unimplemented;
>>          }
>>          break;
>> @@ -6006,6 +6011,10 @@ static void gen_mtc0(DisasContext *ctx, TCGv arg, int 
>> reg, int sel)
>>              /* ignored */
>>              rn = "BadInstrP";
>>              break;
>> +        case 3:
>> +            /* ignored */
>> +            rn = "BadInstrX";
>> +            break;
>>          default:
>>              goto cp0_unimplemented;
>>          }
>> @@ -6711,6 +6720,11 @@ static void gen_dmfc0(DisasContext *ctx, TCGv arg, 
>> int reg, int sel)
>>              gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_BadInstrP));
>>              rn = "BadInstrP";
>>              break;
>> +        case 3:
>> +            CP0_CHECK(ctx->bi);
>> +            gen_mfc0_load32(arg, offsetof(CPUMIPSState, CP0_BadInstrX));
>
> I'm unsure re-using gen_mfc0_load32() is enough, shouldn't we zero the
> 16 lower bits?
>
>> +            rn = "BadInstrX";
>> +            break;
>>          default:
>>              goto cp0_unimplemented;
>>          }
>> @@ -7385,6 +7399,10 @@ static void gen_dmtc0(DisasContext *ctx, TCGv arg, 
>> int reg, int sel)
>>              /* ignored */
>>              rn = "BadInstrP";
>>              break;
>> +        case 3:
>> +            /* ignored */
>> +            rn = "BadInstrX";
>> +            break;
>>          default:
>>              goto cp0_unimplemented;
>>          }
>>