[Qemu-devel] [PATCH 0/2] mips: Allow more 'Chip specific instructions' flags

[Qemu-devel] [PATCH 0/2] mips: Allow more 'Chip specific instructions' flags

[Philippe Mathieu-Daudé]

Hi,

After noticing Fredrik patch [1] clashes with an ongoing work, I shared my
concerns after the current limitations of CPUMIPSState::insn_flags, having
1 bit left to store more 'Chip specific instructions'.

The first patch drop this restriction,
the second simply add definitions for 2 Toshiba cores.

Regards,

Phil.

[1] http://lists.nongnu.org/archive/html/qemu-devel/2018-07/msg01978.html
[2] http://lists.nongnu.org/archive/html/qemu-devel/2018-09/msg00901.html

Philippe Mathieu-Daudé (2):
  target/mips: Increase the 'supported instructions' flags holder size
  target/mips: Add entries for the Toshiba's R3900 and R5900 cores

 target/mips/cpu.h       | 2 +-
 target/mips/internal.h  | 2 +-
 target/mips/mips-defs.h | 2 ++
 target/mips/translate.c | 4 ++--
 4 files changed, 6 insertions(+), 4 deletions(-)

-- 
2.19.0.rc2

[Qemu-devel] [PATCH 1/2] target/mips: Increase the 'supported instructions' flags holder size

[Philippe Mathieu-Daudé]

Currently this holder is limited to at most 32 flags on
a 32-bit architecture, which lets an unique bit available
for another 'chip specific instructions' flag.

Relax this limit using a 64-bit integer.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/cpu.h       | 2 +-
 target/mips/internal.h  | 2 +-
 target/mips/translate.c | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 28af4d191c..f2a5031fd2 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -614,7 +614,7 @@ struct CPUMIPSState {
     int CCRes; /* Cycle count resolution/divisor */
     uint32_t CP0_Status_rw_bitmask; /* Read/write bits in CP0_Status */
     uint32_t CP0_TCStatus_rw_bitmask; /* Read/write bits in CP0_TCStatus */
-    int insn_flags; /* Supported instruction set */
+    uint64_t insn_flags; /* Supported instruction set */
 
     /* Fields up to this point are cleared by a CPU reset */
     struct {} end_reset_fields;
diff --git a/target/mips/internal.h b/target/mips/internal.h
index e41051f8e6..bfe83ee613 100644
--- a/target/mips/internal.h
+++ b/target/mips/internal.h
@@ -59,7 +59,7 @@ struct mips_def_t {
     int32_t CP0_PageGrain_rw_bitmask;
     int32_t CP0_PageGrain;
     target_ulong CP0_EBaseWG_rw_bitmask;
-    int insn_flags;
+    uint64_t insn_flags;
     enum mips_mmu_types mmu_type;
 };
 
diff --git a/target/mips/translate.c b/target/mips/translate.c
index ab16cdb911..3b4e9ebae9 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -1870,7 +1870,7 @@ static inline void check_dspr2(DisasContext *ctx)
 
 /* This code generates a "reserved instruction" exception if the
    CPU does not support the instruction set corresponding to flags. */
-static inline void check_insn(DisasContext *ctx, int flags)
+static inline void check_insn(DisasContext *ctx, uint64_t flags)
 {
     if (unlikely(!(ctx->insn_flags & flags))) {
         generate_exception_end(ctx, EXCP_RI);
@@ -1880,7 +1880,7 @@ static inline void check_insn(DisasContext *ctx, int flags)
 /* This code generates a "reserved instruction" exception if the
    CPU has corresponding flag set which indicates that the instruction
    has been removed. */
-static inline void check_insn_opc_removed(DisasContext *ctx, int flags)
+static inline void check_insn_opc_removed(DisasContext *ctx, uint64_t flags)
 {
     if (unlikely(ctx->insn_flags & flags)) {
         generate_exception_end(ctx, EXCP_RI);
-- 
2.19.0.rc2

[Qemu-devel] [PATCH 2/2] target/mips: Add entries for the Toshiba's R3900 and R5900 cores

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/mips-defs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
index c8e99791ad..9875bdac82 100644
--- a/target/mips/mips-defs.h
+++ b/target/mips/mips-defs.h
@@ -56,6 +56,8 @@
 #define		INSN_LOONGSON2E  0x20000000
 #define		INSN_LOONGSON2F  0x40000000
 #define		INSN_VR54XX	0x80000000
+#define     INSN_R3900       0x100000000ULL
+#define     INSN_R5900       0x200000000ULL
 
 /* MIPS CPU defines. */
 #define		CPU_MIPS1	(ISA_MIPS1)
-- 
2.19.0.rc2

Re: [Qemu-devel] [PATCH 1/2] target/mips: Increase the 'supported instructions' flags holder size

[Aleksandar Markovic]

> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> on behalf of Philippe Mathieu-Daudé <f4bug@amsat.org>
> Sent: Sunday, September 9, 2018 3:34 AM
>
> Subject: [PATCH 1/2] target/mips: Increase the 'supported instructions' flags holder size
>
> Currently this holder is limited to at most 32 flags on
> a 32-bit architecture, which lets an unique bit available
> for another 'chip specific instructions' flag.
> 
> Relax this limit using a 64-bit integer.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> target/mips/cpu.h       | 2 +-
> target/mips/internal.h  | 2 +-
> target/mips/translate.c | 4 ++--
> 3 files changed, 4 insertions(+), 4 deletions(-)

"int insn_flags;" in DisasContext definition in target/mips/translate.c needs to be updated as well.

Re: [Qemu-devel] [PATCH 2/2] target/mips: Add entries for the Toshiba's R3900 and R5900 cores

[Aleksandar Markovic]

> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> on behalf of Philippe > Mathieu-Daudé <f4bug@amsat.org>
> Sent: Sunday, September 9, 2018 3:34 AM
> To: Fredrik Noring; Aleksandar Markovic
> Cc: Philippe Mathieu-Daudé; qemu-devel@nongnu.org; Richard Henderson; Aurelien Jarno
> Subject: [PATCH 2/2] target/mips: Add entries for the Toshiba's R3900 and R5900 cores
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/mips/mips-defs.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
> index c8e99791ad..9875bdac82 100644
> --- a/target/mips/mips-defs.h
> +++ b/target/mips/mips-defs.h
> @@ -56,6 +56,8 @@
>  #define                INSN_LOONGSON2E  0x20000000
>  #define                INSN_LOONGSON2F  0x40000000
>  #define                INSN_VR54XX     0x80000000
> +#define     INSN_R3900       0x100000000ULL
> +#define     INSN_R5900       0x200000000ULL
> 
>  /* MIPS CPU defines. */
>  #define                CPU_MIPS1       (ISA_MIPS1)

I don't think we should add flags for not yet supported CPUs ot ASEs. (Corresponding ELF flags in other headers are OK, since they originate from external headers - however, "insn_flags" is a purely internal QEMU construct.)

Instead, I would reorganize existing flags (as you, as well, indicated in a previous message).

Let's say, something like this:


/*
 * insn_flags
 */

/*
 * bits 0-31 MIPS base instruction sets
 */ 
#define ISA_MIPS1         0x0000000000000001
#define ISA_MIPS2         0x0000000000000002
#define ISA_MIPS3         0x0000000000000004
#define ISA_MIPS4         0x0000000000000008
#define ISA_MIPS5         0x0000000000000010
#define ISA_MIPS32        0x0000000000000020
#define ISA_MIPS32R2      0x0000000000000040
#define ISA_MIPS64        0x0000000000000080
#define ISA_MIPS64R2      0x0000000000000100
#define ISA_MIPS32R3      0x0000000000000200
#define ISA_MIPS64R3      0x0000000000000400
#define ISA_MIPS32R5      0x0000000000000800
#define ISA_MIPS64R5      0x0000000000001000
#define ISA_MIPS32R6      0x0000000000002000
#define ISA_MIPS64R6      0x0000000000004000
#define ISA_NANOMIPS32    0x0000000000008000

/*
 * bits 32-47 MIPS ASEs
 */ 
#define ASE_MIPS16        0x0000000100000000
#define ASE_MIPS3D        0x0000000200000000
#define ASE_MDMX          0x0000000400000000
#define ASE_DSP           0x0000000800000000
#define ASE_DSPR2         0x0000001000000000
#define ASE_DSPR3         0x0000002000000000
#define ASE_MT            0x0000004000000000
#define ASE_SMARTMIPS     0x0000008000000000
#define ASE_MICROMIPS     0x0000010000000000
#define ASE_MSA           0x0000020000000000

/*
 * bits 48-55 vendor-specific base instruction sets
 */ 
#define	INSN_LOONGSON2E   0x0001000000000000
#define	INSN_LOONGSON2F   0x0002000000000000
#define	INSN_VR54XX	  0x0004000000000000

/*
 * bits 56-63 vendor-specific ASEs
 */ 

Notice that I inserted ASE_DSPR3, that was missing (there is an instruction from DSP R3 already implemented, and needing this flag for correct availability control.).

INSN_R5900 would belong to the third category. ASE_MXU and ASE MXU2 (not yet implemented in QEMU) from Igenic would be examples for the fourth category. The fourth category would be empty for now.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH 1/2] target/mips: Increase the 'supported instructions' flags holder size

[Philippe Mathieu-Daudé]

On 9/11/18 6:33 AM, Aleksandar Markovic wrote:
>> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> on behalf of Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Sent: Sunday, September 9, 2018 3:34 AM
>>
>> Subject: [PATCH 1/2] target/mips: Increase the 'supported instructions' flags holder size
>>
>> Currently this holder is limited to at most 32 flags on
>> a 32-bit architecture, which lets an unique bit available
>> for another 'chip specific instructions' flag.
>>
>> Relax this limit using a 64-bit integer.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> target/mips/cpu.h       | 2 +-
>> target/mips/internal.h  | 2 +-
>> target/mips/translate.c | 4 ++--
>> 3 files changed, 4 insertions(+), 4 deletions(-)
> 
> "int insn_flags;" in DisasContext definition in target/mips/translate.c needs to be updated as well.

Oops I missed this one, good catch :)

Re: [Qemu-devel] [PATCH 2/2] target/mips: Add entries for the Toshiba's R3900 and R5900 cores

[Philippe Mathieu-Daudé]

Hi Aleksandar,

On 9/11/18 7:18 AM, Aleksandar Markovic wrote:
>> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> on behalf of Philippe > Mathieu-Daudé <f4bug@amsat.org>
>> Sent: Sunday, September 9, 2018 3:34 AM
>> To: Fredrik Noring; Aleksandar Markovic
>> Cc: Philippe Mathieu-Daudé; qemu-devel@nongnu.org; Richard Henderson; Aurelien Jarno
>> Subject: [PATCH 2/2] target/mips: Add entries for the Toshiba's R3900 and R5900 cores
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  target/mips/mips-defs.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/target/mips/mips-defs.h b/target/mips/mips-defs.h
>> index c8e99791ad..9875bdac82 100644
>> --- a/target/mips/mips-defs.h
>> +++ b/target/mips/mips-defs.h
>> @@ -56,6 +56,8 @@
>>  #define                INSN_LOONGSON2E  0x20000000
>>  #define                INSN_LOONGSON2F  0x40000000
>>  #define                INSN_VR54XX     0x80000000
>> +#define     INSN_R3900       0x100000000ULL
>> +#define     INSN_R5900       0x200000000ULL
>>
>>  /* MIPS CPU defines. */
>>  #define                CPU_MIPS1       (ISA_MIPS1)
> 
> I don't think we should add flags for not yet supported CPUs ot ASEs. (Corresponding ELF flags in other headers are OK, since they originate from external headers - however, "insn_flags" is a purely internal QEMU construct.)

OK. I added those definitions to avoid Fredrik's series clashing with my
work, but I'll adapt later.

> Instead, I would reorganize existing flags (as you, as well, indicated in a previous message).

Good.

> Let's say, something like this:
> 
> 
> /*
>  * insn_flags
>  */
> 
> /*
>  * bits 0-31 MIPS base instruction sets
>  */ 
> #define ISA_MIPS1         0x0000000000000001
> #define ISA_MIPS2         0x0000000000000002
> #define ISA_MIPS3         0x0000000000000004
> #define ISA_MIPS4         0x0000000000000008
> #define ISA_MIPS5         0x0000000000000010
> #define ISA_MIPS32        0x0000000000000020
> #define ISA_MIPS32R2      0x0000000000000040
> #define ISA_MIPS64        0x0000000000000080
> #define ISA_MIPS64R2      0x0000000000000100
> #define ISA_MIPS32R3      0x0000000000000200
> #define ISA_MIPS64R3      0x0000000000000400
> #define ISA_MIPS32R5      0x0000000000000800
> #define ISA_MIPS64R5      0x0000000000001000
> #define ISA_MIPS32R6      0x0000000000002000
> #define ISA_MIPS64R6      0x0000000000004000
> #define ISA_NANOMIPS32    0x0000000000008000
> 
> /*
>  * bits 32-47 MIPS ASEs
>  */ 
> #define ASE_MIPS16        0x0000000100000000
> #define ASE_MIPS3D        0x0000000200000000
> #define ASE_MDMX          0x0000000400000000
> #define ASE_DSP           0x0000000800000000
> #define ASE_DSPR2         0x0000001000000000
> #define ASE_DSPR3         0x0000002000000000
> #define ASE_MT            0x0000004000000000
> #define ASE_SMARTMIPS     0x0000008000000000
> #define ASE_MICROMIPS     0x0000010000000000
> #define ASE_MSA           0x0000020000000000
> 
> /*
>  * bits 48-55 vendor-specific base instruction sets
>  */ 
> #define	INSN_LOONGSON2E   0x0001000000000000
> #define	INSN_LOONGSON2F   0x0002000000000000
> #define	INSN_VR54XX	  0x0004000000000000
> 
> /*
>  * bits 56-63 vendor-specific ASEs
>  */ 

Yes, this is cleaner, I like it. Beware we have to add the ULL suffix.

> Notice that I inserted ASE_DSPR3, that was missing (there is an instruction from DSP R3 already implemented, and needing this flag for correct availability control.).

OK.

> INSN_R5900 would belong to the third category. ASE_MXU and ASE MXU2 (not yet implemented in QEMU) from Igenic would be examples for the fourth category. The fourth category would be empty for now.

OK, thanks for the review!

Phil.