qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] Add RISCV Zilsd extension
@ 2025-11-10  9:05 Roan Richmond
  2025-11-12  1:41 ` Alistair Francis
  2025-11-13 10:54 ` Richard Henderson
  0 siblings, 2 replies; 5+ messages in thread
From: Roan Richmond @ 2025-11-10  9:05 UTC (permalink / raw)
  To: qemu-riscv
  Cc: palmer, alistair.francis, liwei1518, dbarboza, zhiwei_liu,
	qemu-devel, alistair23, richard.henderson, Roan Richmond

This is based on v1.0 of the Zilsd extension [1].
The specification is listed as in the Ratified state [2],
 since Jan 30, 2025.

[1]: https://github.com/riscv/riscv-zilsd
[2]: https://riscv.atlassian.net/wiki/spaces/HOME/pages/16154861/RISC-V+Specs+Under+Development

Reviewed-by: Daniel Henrique barboza <dbarboza@ventanamicro.com>
Reviewed-by: Richard Henderson  <richard.henderson@linaro.org>
Signed-off-by: Roan Richmond <roan.richmond@codethink.co.uk>
---
v2:
  - In `gen_load_zilsd` swapped `dest_gpr` with `tcg_temp_new`, this prevents
    writing to `r1/r0` in the case where `r0` is the set as `rd`.
  - Removed unneeded brackets around `a->rd` + c expressions

 target/riscv/cpu.c                      |  2 +
 target/riscv/cpu_cfg_fields.h.inc       |  1 +
 target/riscv/insn_trans/trans_rvi.c.inc | 57 ++++++++++++++++++++++++-
 3 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 73d4280d7c..6e0d37fb96 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -121,6 +121,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
     ISA_EXT_DATA_ENTRY(zihintntl, PRIV_VERSION_1_10_0, ext_zihintntl),
     ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
     ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
+    ISA_EXT_DATA_ENTRY(zilsd, PRIV_VERSION_1_13_0, ext_zilsd),
     ISA_EXT_DATA_ENTRY(zimop, PRIV_VERSION_1_13_0, ext_zimop),
     ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
     ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, has_priv_1_12),
@@ -1247,6 +1248,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
     MULTI_EXT_CFG_BOOL("zicsr", ext_zicsr, true),
     MULTI_EXT_CFG_BOOL("zihintntl", ext_zihintntl, true),
     MULTI_EXT_CFG_BOOL("zihintpause", ext_zihintpause, true),
+    MULTI_EXT_CFG_BOOL("zilsd", ext_zilsd, false),
     MULTI_EXT_CFG_BOOL("zimop", ext_zimop, false),
     MULTI_EXT_CFG_BOOL("zcmop", ext_zcmop, false),
     MULTI_EXT_CFG_BOOL("zacas", ext_zacas, false),
diff --git a/target/riscv/cpu_cfg_fields.h.inc b/target/riscv/cpu_cfg_fields.h.inc
index a154ecdc79..8d8e35e4cf 100644
--- a/target/riscv/cpu_cfg_fields.h.inc
+++ b/target/riscv/cpu_cfg_fields.h.inc
@@ -41,6 +41,7 @@ BOOL_FIELD(ext_zicond)
 BOOL_FIELD(ext_zihintntl)
 BOOL_FIELD(ext_zihintpause)
 BOOL_FIELD(ext_zihpm)
+BOOL_FIELD(ext_zilsd)
 BOOL_FIELD(ext_zimop)
 BOOL_FIELD(ext_zcmop)
 BOOL_FIELD(ext_ztso)
diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
index 54b9b4f241..20058bf483 100644
--- a/target/riscv/insn_trans/trans_rvi.c.inc
+++ b/target/riscv/insn_trans/trans_rvi.c.inc
@@ -18,6 +18,7 @@
  * this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "migration/multifd.h"
 static bool trans_illegal(DisasContext *ctx, arg_empty *a)
 {
     gen_exception_illegal(ctx);
@@ -370,6 +371,27 @@ static bool gen_load_tl(DisasContext *ctx, arg_lb *a, MemOp memop)
     return true;
 }
 
+/* Zilsd extension adds load/store double for 32bit arch */
+static bool gen_load_zilsd(DisasContext *ctx, arg_lb *a)
+{
+    TCGv dest_1 = tcg_temp_new();
+    TCGv dest_2 = tcg_temp_new();
+    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);
+    TCGv addr_2 = get_address(ctx, a->rs1, a->imm+4);
+    
+    tcg_gen_qemu_ld_tl(dest_1, addr_1, ctx->mem_idx, MO_SL);
+    tcg_gen_qemu_ld_tl(dest_2, addr_2, ctx->mem_idx, MO_SL);
+
+    /* If destination is x0 then result of the load is discarded */
+    if (a->rd == 0) {
+        return true;
+    }
+
+    gen_set_gpr(ctx, a->rd, dest_1);
+    gen_set_gpr(ctx, a->rd+1, dest_2);
+    return true;
+}
+
 /* Compute only 64-bit addresses to use the address translation mechanism */
 static bool gen_load_i128(DisasContext *ctx, arg_lb *a, MemOp memop)
 {
@@ -409,6 +431,8 @@ static bool gen_load(DisasContext *ctx, arg_lb *a, MemOp memop)
     decode_save_opc(ctx, 0);
     if (get_xl(ctx) == MXL_RV128) {
         out = gen_load_i128(ctx, a, memop);
+    } else if (memop == MO_SQ && get_xl(ctx) == MXL_RV32) {
+        out = gen_load_zilsd(ctx, a);
     } else {
         out = gen_load_tl(ctx, a, memop);
     }
@@ -437,7 +461,10 @@ static bool trans_lw(DisasContext *ctx, arg_lw *a)
 
 static bool trans_ld(DisasContext *ctx, arg_ld *a)
 {
-    REQUIRE_64_OR_128BIT(ctx);
+    /* Check for Zilsd extension if 32bit */
+    if (get_xl(ctx) == MXL_RV32 && !ctx->cfg_ptr->ext_zilsd) {
+        return false;
+    }
     return gen_load(ctx, a, MO_SQ);
 }
 
@@ -482,6 +509,27 @@ static bool gen_store_tl(DisasContext *ctx, arg_sb *a, MemOp memop)
     return true;
 }
 
+/* Zilsd extension adds load/store double for 32bit arch */
+static bool gen_store_zilsd(DisasContext *ctx, arg_sb *a)
+{
+    TCGv data_1 = tcg_temp_new();
+    TCGv data_2 = tcg_temp_new();
+    if (a->rs2 != 0) {
+        data_1 = get_gpr(ctx, a->rs2, EXT_NONE);
+        data_2 = get_gpr(ctx, a->rs2+1, EXT_NONE);
+    }
+    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);
+    TCGv addr_2 = get_address(ctx, a->rs1, a->imm+4);
+
+    if (ctx->ztso) {
+        tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
+    }
+
+    tcg_gen_qemu_st_tl(data_1, addr_1, ctx->mem_idx, MO_SL);
+    tcg_gen_qemu_st_tl(data_2, addr_2, ctx->mem_idx, MO_SL);
+    return true;
+}
+
 static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop)
 {
     TCGv src1l = get_gpr(ctx, a->rs1, EXT_NONE);
@@ -511,6 +559,8 @@ static bool gen_store(DisasContext *ctx, arg_sb *a, MemOp memop)
     decode_save_opc(ctx, 0);
     if (get_xl(ctx) == MXL_RV128) {
         return gen_store_i128(ctx, a, memop);
+    } else if (memop == MO_UQ && get_xl(ctx) == MXL_RV32) {
+        return gen_store_zilsd(ctx, a);
     } else {
         return gen_store_tl(ctx, a, memop);
     }
@@ -533,7 +583,10 @@ static bool trans_sw(DisasContext *ctx, arg_sw *a)
 
 static bool trans_sd(DisasContext *ctx, arg_sd *a)
 {
-    REQUIRE_64_OR_128BIT(ctx);
+    /* Check for Zilsd extension if 32bit */
+    if (get_xl(ctx) == MXL_RV32 && !ctx->cfg_ptr->ext_zilsd) {
+        return false;
+    }
     return gen_store(ctx, a, MO_UQ);
 }
 
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Add RISCV Zilsd extension
  2025-11-10  9:05 [PATCH v2] Add RISCV Zilsd extension Roan Richmond
@ 2025-11-12  1:41 ` Alistair Francis
  2025-11-13  8:53   ` Roan Richmond
  2025-11-13 10:54 ` Richard Henderson
  1 sibling, 1 reply; 5+ messages in thread
From: Alistair Francis @ 2025-11-12  1:41 UTC (permalink / raw)
  To: Roan Richmond
  Cc: qemu-riscv, palmer, alistair.francis, liwei1518, dbarboza,
	zhiwei_liu, qemu-devel, richard.henderson

On Mon, Nov 10, 2025 at 7:05 PM Roan Richmond
<roan.richmond@codethink.co.uk> wrote:
>
> This is based on v1.0 of the Zilsd extension [1].
> The specification is listed as in the Ratified state [2],
>  since Jan 30, 2025.
>
> [1]: https://github.com/riscv/riscv-zilsd
> [2]: https://riscv.atlassian.net/wiki/spaces/HOME/pages/16154861/RISC-V+Specs+Under+Development
>
> Reviewed-by: Daniel Henrique barboza <dbarboza@ventanamicro.com>
> Reviewed-by: Richard Henderson  <richard.henderson@linaro.org>
> Signed-off-by: Roan Richmond <roan.richmond@codethink.co.uk>
> ---
> v2:
>   - In `gen_load_zilsd` swapped `dest_gpr` with `tcg_temp_new`, this prevents
>     writing to `r1/r0` in the case where `r0` is the set as `rd`.
>   - Removed unneeded brackets around `a->rd` + c expressions
>
>  target/riscv/cpu.c                      |  2 +
>  target/riscv/cpu_cfg_fields.h.inc       |  1 +
>  target/riscv/insn_trans/trans_rvi.c.inc | 57 ++++++++++++++++++++++++-
>  3 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 73d4280d7c..6e0d37fb96 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -121,6 +121,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>      ISA_EXT_DATA_ENTRY(zihintntl, PRIV_VERSION_1_10_0, ext_zihintntl),
>      ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
>      ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
> +    ISA_EXT_DATA_ENTRY(zilsd, PRIV_VERSION_1_13_0, ext_zilsd),
>      ISA_EXT_DATA_ENTRY(zimop, PRIV_VERSION_1_13_0, ext_zimop),
>      ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
>      ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, has_priv_1_12),
> @@ -1247,6 +1248,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>      MULTI_EXT_CFG_BOOL("zicsr", ext_zicsr, true),
>      MULTI_EXT_CFG_BOOL("zihintntl", ext_zihintntl, true),
>      MULTI_EXT_CFG_BOOL("zihintpause", ext_zihintpause, true),
> +    MULTI_EXT_CFG_BOOL("zilsd", ext_zilsd, false),
>      MULTI_EXT_CFG_BOOL("zimop", ext_zimop, false),
>      MULTI_EXT_CFG_BOOL("zcmop", ext_zcmop, false),
>      MULTI_EXT_CFG_BOOL("zacas", ext_zacas, false),
> diff --git a/target/riscv/cpu_cfg_fields.h.inc b/target/riscv/cpu_cfg_fields.h.inc
> index a154ecdc79..8d8e35e4cf 100644
> --- a/target/riscv/cpu_cfg_fields.h.inc
> +++ b/target/riscv/cpu_cfg_fields.h.inc
> @@ -41,6 +41,7 @@ BOOL_FIELD(ext_zicond)
>  BOOL_FIELD(ext_zihintntl)
>  BOOL_FIELD(ext_zihintpause)
>  BOOL_FIELD(ext_zihpm)
> +BOOL_FIELD(ext_zilsd)
>  BOOL_FIELD(ext_zimop)
>  BOOL_FIELD(ext_zcmop)
>  BOOL_FIELD(ext_ztso)
> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
> index 54b9b4f241..20058bf483 100644
> --- a/target/riscv/insn_trans/trans_rvi.c.inc
> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
> @@ -18,6 +18,7 @@
>   * this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>
> +#include "migration/multifd.h"
>  static bool trans_illegal(DisasContext *ctx, arg_empty *a)
>  {
>      gen_exception_illegal(ctx);
> @@ -370,6 +371,27 @@ static bool gen_load_tl(DisasContext *ctx, arg_lb *a, MemOp memop)
>      return true;
>  }
>
> +/* Zilsd extension adds load/store double for 32bit arch */
> +static bool gen_load_zilsd(DisasContext *ctx, arg_lb *a)
> +{
> +    TCGv dest_1 = tcg_temp_new();
> +    TCGv dest_2 = tcg_temp_new();
> +    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);
> +    TCGv addr_2 = get_address(ctx, a->rs1, a->imm+4);
> +
> +    tcg_gen_qemu_ld_tl(dest_1, addr_1, ctx->mem_idx, MO_SL);
> +    tcg_gen_qemu_ld_tl(dest_2, addr_2, ctx->mem_idx, MO_SL);
> +
> +    /* If destination is x0 then result of the load is discarded */
> +    if (a->rd == 0) {
> +        return true;
> +    }
> +
> +    gen_set_gpr(ctx, a->rd, dest_1);
> +    gen_set_gpr(ctx, a->rd+1, dest_2);
> +    return true;
> +}
> +
>  /* Compute only 64-bit addresses to use the address translation mechanism */
>  static bool gen_load_i128(DisasContext *ctx, arg_lb *a, MemOp memop)
>  {
> @@ -409,6 +431,8 @@ static bool gen_load(DisasContext *ctx, arg_lb *a, MemOp memop)
>      decode_save_opc(ctx, 0);
>      if (get_xl(ctx) == MXL_RV128) {
>          out = gen_load_i128(ctx, a, memop);
> +    } else if (memop == MO_SQ && get_xl(ctx) == MXL_RV32) {
> +        out = gen_load_zilsd(ctx, a);
>      } else {
>          out = gen_load_tl(ctx, a, memop);
>      }
> @@ -437,7 +461,10 @@ static bool trans_lw(DisasContext *ctx, arg_lw *a)
>
>  static bool trans_ld(DisasContext *ctx, arg_ld *a)
>  {
> -    REQUIRE_64_OR_128BIT(ctx);
> +    /* Check for Zilsd extension if 32bit */
> +    if (get_xl(ctx) == MXL_RV32 && !ctx->cfg_ptr->ext_zilsd) {
> +        return false;
> +    }
>      return gen_load(ctx, a, MO_SQ);
>  }
>
> @@ -482,6 +509,27 @@ static bool gen_store_tl(DisasContext *ctx, arg_sb *a, MemOp memop)
>      return true;
>  }
>
> +/* Zilsd extension adds load/store double for 32bit arch */
> +static bool gen_store_zilsd(DisasContext *ctx, arg_sb *a)
> +{
> +    TCGv data_1 = tcg_temp_new();
> +    TCGv data_2 = tcg_temp_new();
> +    if (a->rs2 != 0) {
> +        data_1 = get_gpr(ctx, a->rs2, EXT_NONE);
> +        data_2 = get_gpr(ctx, a->rs2+1, EXT_NONE);
> +    }

Don't mix code and declarations, otherwise

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> +    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);
> +    TCGv addr_2 = get_address(ctx, a->rs1, a->imm+4);
> +
> +    if (ctx->ztso) {
> +        tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
> +    }
> +
> +    tcg_gen_qemu_st_tl(data_1, addr_1, ctx->mem_idx, MO_SL);
> +    tcg_gen_qemu_st_tl(data_2, addr_2, ctx->mem_idx, MO_SL);
> +    return true;
> +}
> +
>  static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop)
>  {
>      TCGv src1l = get_gpr(ctx, a->rs1, EXT_NONE);
> @@ -511,6 +559,8 @@ static bool gen_store(DisasContext *ctx, arg_sb *a, MemOp memop)
>      decode_save_opc(ctx, 0);
>      if (get_xl(ctx) == MXL_RV128) {
>          return gen_store_i128(ctx, a, memop);
> +    } else if (memop == MO_UQ && get_xl(ctx) == MXL_RV32) {
> +        return gen_store_zilsd(ctx, a);
>      } else {
>          return gen_store_tl(ctx, a, memop);
>      }
> @@ -533,7 +583,10 @@ static bool trans_sw(DisasContext *ctx, arg_sw *a)
>
>  static bool trans_sd(DisasContext *ctx, arg_sd *a)
>  {
> -    REQUIRE_64_OR_128BIT(ctx);
> +    /* Check for Zilsd extension if 32bit */
> +    if (get_xl(ctx) == MXL_RV32 && !ctx->cfg_ptr->ext_zilsd) {
> +        return false;
> +    }
>      return gen_store(ctx, a, MO_UQ);
>  }
>
> --
> 2.43.0
>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Add RISCV Zilsd extension
  2025-11-12  1:41 ` Alistair Francis
@ 2025-11-13  8:53   ` Roan Richmond
  2025-11-21  7:41     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: Roan Richmond @ 2025-11-13  8:53 UTC (permalink / raw)
  To: Alistair Francis
  Cc: qemu-riscv, palmer, alistair.francis, liwei1518, dbarboza,
	zhiwei_liu, qemu-devel, richard.henderson


On 12/11/2025 01:41, Alistair Francis wrote:
> On Mon, Nov 10, 2025 at 7:05 PM Roan Richmond
> <roan.richmond@codethink.co.uk> wrote:
>> This is based on v1.0 of the Zilsd extension [1].
>> The specification is listed as in the Ratified state [2],
>>   since Jan 30, 2025.
>>
>> [1]: https://github.com/riscv/riscv-zilsd
>> [2]: https://riscv.atlassian.net/wiki/spaces/HOME/pages/16154861/RISC-V+Specs+Under+Development
>>
>> Reviewed-by: Daniel Henrique barboza <dbarboza@ventanamicro.com>
>> Reviewed-by: Richard Henderson  <richard.henderson@linaro.org>
>> Signed-off-by: Roan Richmond <roan.richmond@codethink.co.uk>
>> ---
>> v2:
>>    - In `gen_load_zilsd` swapped `dest_gpr` with `tcg_temp_new`, this prevents
>>      writing to `r1/r0` in the case where `r0` is the set as `rd`.
>>    - Removed unneeded brackets around `a->rd` + c expressions
>>
>>   target/riscv/cpu.c                      |  2 +
>>   target/riscv/cpu_cfg_fields.h.inc       |  1 +
>>   target/riscv/insn_trans/trans_rvi.c.inc | 57 ++++++++++++++++++++++++-
>>   3 files changed, 58 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index 73d4280d7c..6e0d37fb96 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -121,6 +121,7 @@ const RISCVIsaExtData isa_edata_arr[] = {
>>       ISA_EXT_DATA_ENTRY(zihintntl, PRIV_VERSION_1_10_0, ext_zihintntl),
>>       ISA_EXT_DATA_ENTRY(zihintpause, PRIV_VERSION_1_10_0, ext_zihintpause),
>>       ISA_EXT_DATA_ENTRY(zihpm, PRIV_VERSION_1_12_0, ext_zihpm),
>> +    ISA_EXT_DATA_ENTRY(zilsd, PRIV_VERSION_1_13_0, ext_zilsd),
>>       ISA_EXT_DATA_ENTRY(zimop, PRIV_VERSION_1_13_0, ext_zimop),
>>       ISA_EXT_DATA_ENTRY(zmmul, PRIV_VERSION_1_12_0, ext_zmmul),
>>       ISA_EXT_DATA_ENTRY(za64rs, PRIV_VERSION_1_12_0, has_priv_1_12),
>> @@ -1247,6 +1248,7 @@ const RISCVCPUMultiExtConfig riscv_cpu_extensions[] = {
>>       MULTI_EXT_CFG_BOOL("zicsr", ext_zicsr, true),
>>       MULTI_EXT_CFG_BOOL("zihintntl", ext_zihintntl, true),
>>       MULTI_EXT_CFG_BOOL("zihintpause", ext_zihintpause, true),
>> +    MULTI_EXT_CFG_BOOL("zilsd", ext_zilsd, false),
>>       MULTI_EXT_CFG_BOOL("zimop", ext_zimop, false),
>>       MULTI_EXT_CFG_BOOL("zcmop", ext_zcmop, false),
>>       MULTI_EXT_CFG_BOOL("zacas", ext_zacas, false),
>> diff --git a/target/riscv/cpu_cfg_fields.h.inc b/target/riscv/cpu_cfg_fields.h.inc
>> index a154ecdc79..8d8e35e4cf 100644
>> --- a/target/riscv/cpu_cfg_fields.h.inc
>> +++ b/target/riscv/cpu_cfg_fields.h.inc
>> @@ -41,6 +41,7 @@ BOOL_FIELD(ext_zicond)
>>   BOOL_FIELD(ext_zihintntl)
>>   BOOL_FIELD(ext_zihintpause)
>>   BOOL_FIELD(ext_zihpm)
>> +BOOL_FIELD(ext_zilsd)
>>   BOOL_FIELD(ext_zimop)
>>   BOOL_FIELD(ext_zcmop)
>>   BOOL_FIELD(ext_ztso)
>> diff --git a/target/riscv/insn_trans/trans_rvi.c.inc b/target/riscv/insn_trans/trans_rvi.c.inc
>> index 54b9b4f241..20058bf483 100644
>> --- a/target/riscv/insn_trans/trans_rvi.c.inc
>> +++ b/target/riscv/insn_trans/trans_rvi.c.inc
>> @@ -18,6 +18,7 @@
>>    * this program.  If not, see <http://www.gnu.org/licenses/>.
>>    */
>>
>> +#include "migration/multifd.h"
>>   static bool trans_illegal(DisasContext *ctx, arg_empty *a)
>>   {
>>       gen_exception_illegal(ctx);
>> @@ -370,6 +371,27 @@ static bool gen_load_tl(DisasContext *ctx, arg_lb *a, MemOp memop)
>>       return true;
>>   }
>>
>> +/* Zilsd extension adds load/store double for 32bit arch */
>> +static bool gen_load_zilsd(DisasContext *ctx, arg_lb *a)
>> +{
>> +    TCGv dest_1 = tcg_temp_new();
>> +    TCGv dest_2 = tcg_temp_new();
>> +    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);
>> +    TCGv addr_2 = get_address(ctx, a->rs1, a->imm+4);
>> +
>> +    tcg_gen_qemu_ld_tl(dest_1, addr_1, ctx->mem_idx, MO_SL);
>> +    tcg_gen_qemu_ld_tl(dest_2, addr_2, ctx->mem_idx, MO_SL);
>> +
>> +    /* If destination is x0 then result of the load is discarded */
>> +    if (a->rd == 0) {
>> +        return true;
>> +    }
>> +
>> +    gen_set_gpr(ctx, a->rd, dest_1);
>> +    gen_set_gpr(ctx, a->rd+1, dest_2);
>> +    return true;
>> +}
>> +
>>   /* Compute only 64-bit addresses to use the address translation mechanism */
>>   static bool gen_load_i128(DisasContext *ctx, arg_lb *a, MemOp memop)
>>   {
>> @@ -409,6 +431,8 @@ static bool gen_load(DisasContext *ctx, arg_lb *a, MemOp memop)
>>       decode_save_opc(ctx, 0);
>>       if (get_xl(ctx) == MXL_RV128) {
>>           out = gen_load_i128(ctx, a, memop);
>> +    } else if (memop == MO_SQ && get_xl(ctx) == MXL_RV32) {
>> +        out = gen_load_zilsd(ctx, a);
>>       } else {
>>           out = gen_load_tl(ctx, a, memop);
>>       }
>> @@ -437,7 +461,10 @@ static bool trans_lw(DisasContext *ctx, arg_lw *a)
>>
>>   static bool trans_ld(DisasContext *ctx, arg_ld *a)
>>   {
>> -    REQUIRE_64_OR_128BIT(ctx);
>> +    /* Check for Zilsd extension if 32bit */
>> +    if (get_xl(ctx) == MXL_RV32 && !ctx->cfg_ptr->ext_zilsd) {
>> +        return false;
>> +    }
>>       return gen_load(ctx, a, MO_SQ);
>>   }
>>
>> @@ -482,6 +509,27 @@ static bool gen_store_tl(DisasContext *ctx, arg_sb *a, MemOp memop)
>>       return true;
>>   }
>>
>> +/* Zilsd extension adds load/store double for 32bit arch */
>> +static bool gen_store_zilsd(DisasContext *ctx, arg_sb *a)
>> +{
>> +    TCGv data_1 = tcg_temp_new();
>> +    TCGv data_2 = tcg_temp_new();
>> +    if (a->rs2 != 0) {
>> +        data_1 = get_gpr(ctx, a->rs2, EXT_NONE);
>> +        data_2 = get_gpr(ctx, a->rs2+1, EXT_NONE);
>> +    }
> Don't mix code and declarations, otherwise
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Alistair
Struggling to see what you mean by this.

Could you provide a bit more clarification on the changes you would like 
me to make and then I'll send out a v3.

Thanks,
Roan

>
>> +    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);
>> +    TCGv addr_2 = get_address(ctx, a->rs1, a->imm+4);
>> +
>> +    if (ctx->ztso) {
>> +        tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
>> +    }
>> +
>> +    tcg_gen_qemu_st_tl(data_1, addr_1, ctx->mem_idx, MO_SL);
>> +    tcg_gen_qemu_st_tl(data_2, addr_2, ctx->mem_idx, MO_SL);
>> +    return true;
>> +}
>> +
>>   static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop)
>>   {
>>       TCGv src1l = get_gpr(ctx, a->rs1, EXT_NONE);
>> @@ -511,6 +559,8 @@ static bool gen_store(DisasContext *ctx, arg_sb *a, MemOp memop)
>>       decode_save_opc(ctx, 0);
>>       if (get_xl(ctx) == MXL_RV128) {
>>           return gen_store_i128(ctx, a, memop);
>> +    } else if (memop == MO_UQ && get_xl(ctx) == MXL_RV32) {
>> +        return gen_store_zilsd(ctx, a);
>>       } else {
>>           return gen_store_tl(ctx, a, memop);
>>       }
>> @@ -533,7 +583,10 @@ static bool trans_sw(DisasContext *ctx, arg_sw *a)
>>
>>   static bool trans_sd(DisasContext *ctx, arg_sd *a)
>>   {
>> -    REQUIRE_64_OR_128BIT(ctx);
>> +    /* Check for Zilsd extension if 32bit */
>> +    if (get_xl(ctx) == MXL_RV32 && !ctx->cfg_ptr->ext_zilsd) {
>> +        return false;
>> +    }
>>       return gen_store(ctx, a, MO_UQ);
>>   }
>>
>> --
>> 2.43.0
>>
-- 
Roan Richmond (he/him)
Software Engineer
Codethink Ltd



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Add RISCV Zilsd extension
  2025-11-10  9:05 [PATCH v2] Add RISCV Zilsd extension Roan Richmond
  2025-11-12  1:41 ` Alistair Francis
@ 2025-11-13 10:54 ` Richard Henderson
  1 sibling, 0 replies; 5+ messages in thread
From: Richard Henderson @ 2025-11-13 10:54 UTC (permalink / raw)
  To: Roan Richmond, qemu-riscv
  Cc: palmer, alistair.francis, liwei1518, dbarboza, zhiwei_liu,
	qemu-devel, alistair23

On 11/10/25 10:05, Roan Richmond wrote:
> +/* Zilsd extension adds load/store double for 32bit arch */
> +static bool gen_store_zilsd(DisasContext *ctx, arg_sb *a)
> +{
> +    TCGv data_1 = tcg_temp_new();
> +    TCGv data_2 = tcg_temp_new();
> +    if (a->rs2 != 0) {
> +        data_1 = get_gpr(ctx, a->rs2, EXT_NONE);
> +        data_2 = get_gpr(ctx, a->rs2+1, EXT_NONE);
> +    }
> +    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);
> +    TCGv addr_2 = get_address(ctx, a->rs1, a->imm+4);
> +
> +    if (ctx->ztso) {
> +        tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
> +    }
> +
> +    tcg_gen_qemu_st_tl(data_1, addr_1, ctx->mem_idx, MO_SL);
> +    tcg_gen_qemu_st_tl(data_2, addr_2, ctx->mem_idx, MO_SL);

This is wrong, because if rs2 == 0, then data_1 and data_2 are uninitialized.  If you're 
testing properly with --enable-debug-tcg, this should trigger an assertion failure.

You wanted

     if (a->rs2 == 0) {
         data_1 = tcg_constant_tl(0);
         data_2 = data_1;
     } else {
         data_1 = get_gpr(ctx, a->rs2, EXT_NONE);
         data_2 = get_gpr(ctx, a->rs2 + 1, EXT_NONE);
     }

You're also missing the endianness indicator: mo_endian().

You really should consider combining the two halves so that you can perform one 64-bit 
store.  While I see that the spec allows the store to be non-atomic, you still probably do 
not want to to allow the first half store to succeed when the second half store faults.  I 
don't see clear language about that.  Anyway, that's as simple as

     TCGv_i64 data;
     MemOp end = mo_endian(ctx);

     if (a->rs2 == 0) {
         data = tcg_constant_i64(0);
     } else {
         TCGv data_1 = get_gpr(ctx, a->rs2, EXT_NONE);
         TCGv data_2 = get_gpr(ctx, a->rs2 + 1, EXT_NONE);
         data = tcg_temp_new_i64();
         if (end == MO_LE) {
             tcg_gen_concat_tl_i64(data, data_1, data_2);
         } else {
             tcg_gen_concat_tl_i64(data, data_2, data_1);
         }
     }
     tcg_gen_qemu_st_i64(data, addr, ctx->mem_idx, MO_UQ | end);


r~


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] Add RISCV Zilsd extension
  2025-11-13  8:53   ` Roan Richmond
@ 2025-11-21  7:41     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-11-21  7:41 UTC (permalink / raw)
  To: Roan Richmond, Alistair Francis
  Cc: qemu-riscv, palmer, alistair.francis, liwei1518, dbarboza,
	zhiwei_liu, qemu-devel, richard.henderson

Hi Roan,

On 13/11/25 09:53, Roan Richmond wrote:
> On 12/11/2025 01:41, Alistair Francis wrote:
>> On Mon, Nov 10, 2025 at 7:05 PM Roan Richmond
>> <roan.richmond@codethink.co.uk> wrote:
>>> This is based on v1.0 of the Zilsd extension [1].
>>> The specification is listed as in the Ratified state [2],
>>>   since Jan 30, 2025.
>>>
>>> [1]: https://github.com/riscv/riscv-zilsd
>>> [2]: https://riscv.atlassian.net/wiki/spaces/HOME/pages/16154861/ 
>>> RISC-V+Specs+Under+Development
>>>
>>> Reviewed-by: Daniel Henrique barboza <dbarboza@ventanamicro.com>
>>> Reviewed-by: Richard Henderson  <richard.henderson@linaro.org>
>>> Signed-off-by: Roan Richmond <roan.richmond@codethink.co.uk>
>>> ---
>>> v2:
>>>    - In `gen_load_zilsd` swapped `dest_gpr` with `tcg_temp_new`, this 
>>> prevents
>>>      writing to `r1/r0` in the case where `r0` is the set as `rd`.
>>>    - Removed unneeded brackets around `a->rd` + c expressions
>>>
>>>   target/riscv/cpu.c                      |  2 +
>>>   target/riscv/cpu_cfg_fields.h.inc       |  1 +
>>>   target/riscv/insn_trans/trans_rvi.c.inc | 57 ++++++++++++++++++++++++-
>>>   3 files changed, 58 insertions(+), 2 deletions(-)


>>> @@ -482,6 +509,27 @@ static bool gen_store_tl(DisasContext *ctx, 
>>> arg_sb *a, MemOp memop)
>>>       return true;
>>>   }
>>>
>>> +/* Zilsd extension adds load/store double for 32bit arch */
>>> +static bool gen_store_zilsd(DisasContext *ctx, arg_sb *a)
>>> +{
>>> +    TCGv data_1 = tcg_temp_new();
>>> +    TCGv data_2 = tcg_temp_new();

[*]

>>> +    if (a->rs2 != 0) {
>>> +        data_1 = get_gpr(ctx, a->rs2, EXT_NONE);
>>> +        data_2 = get_gpr(ctx, a->rs2+1, EXT_NONE);
>>> +    }
>> Don't mix code and declarations, otherwise
>>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>>
>> Alistair
> Struggling to see what you mean by this.
> 
> Could you provide a bit more clarification on the changes you would like 
> me to make and then I'll send out a v3.
> 
> Thanks,
> Roan
> 
>>
>>> +    TCGv addr_1 = get_address(ctx, a->rs1, a->imm);

The declaration part is:

    TCGv addr_1;

and the code part is:

    addr_1 = get_address(ctx, a->rs1, a->imm);

Per our coding style (docs/devel/style.rst) chapter "Declarations":

    Mixed declarations (interleaving statements and declarations
    within blocks) are generally not allowed; declarations should
    be at the beginning of blocks.

Declarations should go in [*].

For v3 you should address Richard's comments.

Regards,

Phil.

>>> +    TCGv addr_2 = get_address(ctx, a->rs1, a->imm+4);
>>> +
>>> +    if (ctx->ztso) {
>>> +        tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
>>> +    }
>>> +
>>> +    tcg_gen_qemu_st_tl(data_1, addr_1, ctx->mem_idx, MO_SL);
>>> +    tcg_gen_qemu_st_tl(data_2, addr_2, ctx->mem_idx, MO_SL);
>>> +    return true;
>>> +}
>>> +
>>>   static bool gen_store_i128(DisasContext *ctx, arg_sb *a, MemOp memop)
>>>   {
>>>       TCGv src1l = get_gpr(ctx, a->rs1, EXT_NONE);
>>> @@ -511,6 +559,8 @@ static bool gen_store(DisasContext *ctx, arg_sb 
>>> *a, MemOp memop)
>>>       decode_save_opc(ctx, 0);
>>>       if (get_xl(ctx) == MXL_RV128) {
>>>           return gen_store_i128(ctx, a, memop);
>>> +    } else if (memop == MO_UQ && get_xl(ctx) == MXL_RV32) {
>>> +        return gen_store_zilsd(ctx, a);
>>>       } else {
>>>           return gen_store_tl(ctx, a, memop);
>>>       }
>>> @@ -533,7 +583,10 @@ static bool trans_sw(DisasContext *ctx, arg_sw *a)
>>>
>>>   static bool trans_sd(DisasContext *ctx, arg_sd *a)
>>>   {
>>> -    REQUIRE_64_OR_128BIT(ctx);
>>> +    /* Check for Zilsd extension if 32bit */
>>> +    if (get_xl(ctx) == MXL_RV32 && !ctx->cfg_ptr->ext_zilsd) {
>>> +        return false;
>>> +    }
>>>       return gen_store(ctx, a, MO_UQ);
>>>   }
>>>
>>> -- 
>>> 2.43.0
>>>



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-11-21  7:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-10  9:05 [PATCH v2] Add RISCV Zilsd extension Roan Richmond
2025-11-12  1:41 ` Alistair Francis
2025-11-13  8:53   ` Roan Richmond
2025-11-21  7:41     ` Philippe Mathieu-Daudé
2025-11-13 10:54 ` Richard Henderson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).