* Re: [PATCH v3] target/riscv: Raise exceptions on wrs.nto
2024-04-24 14:28 [PATCH v3] target/riscv: Raise exceptions on wrs.nto Andrew Jones
@ 2024-04-24 14:46 ` Andrew Jones
2024-04-26 12:58 ` Daniel Henrique Barboza
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Andrew Jones @ 2024-04-24 14:46 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: christoph.muellner, palmer, alistair.francis, bin.meng, liwei1518,
dbarboza, zhiwei_liu
On Wed, Apr 24, 2024 at 04:28:09PM +0200, Andrew Jones wrote:
> Implementing wrs.nto to always just return is consistent with the
> specification, as the instruction is permitted to terminate the
> stall for any reason, but it's not useful for virtualization, where
> we'd like the guest to trap to the hypervisor in order to allow
> scheduling of the lock holding VCPU. Change to always immediately
> raise exceptions when the appropriate conditions are present,
> otherwise continue to just return. Note, immediately raising
> exceptions is also consistent with the specification since the
> time limit that should expire prior to the exception is
> implementation-specific.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Christoph M??llner <christoph.muellner@vrull.eu>
> ---
> v3:
> - Sending again, hoping the ?? remains in M??llner
This looks like my mail reader (neomutt)'s problem. The patch was correct
(also for v2) and other readers render it correctly. Sorry for the noise.
Thanks,
drew
> v2:
> - Added #ifndef CONFIG_USER_ONLY around helper call
>
> target/riscv/helper.h | 1 +
> target/riscv/insn_trans/trans_rvzawrs.c.inc | 29 ++++++++++++++-------
> target/riscv/op_helper.c | 11 ++++++++
> 3 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index 8a635238514d..451261ce5a4f 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -132,6 +132,7 @@ DEF_HELPER_6(csrrw_i128, tl, env, int, tl, tl, tl, tl)
> DEF_HELPER_1(sret, tl, env)
> DEF_HELPER_1(mret, tl, env)
> DEF_HELPER_1(wfi, void, env)
> +DEF_HELPER_1(wrs_nto, void, env)
> DEF_HELPER_1(tlb_flush, void, env)
> DEF_HELPER_1(tlb_flush_all, void, env)
> /* Native Debug */
> diff --git a/target/riscv/insn_trans/trans_rvzawrs.c.inc b/target/riscv/insn_trans/trans_rvzawrs.c.inc
> index 32efbff4d5a5..0eef03383889 100644
> --- a/target/riscv/insn_trans/trans_rvzawrs.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzawrs.c.inc
> @@ -16,7 +16,7 @@
> * this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> -static bool trans_wrs(DisasContext *ctx)
> +static bool trans_wrs_sto(DisasContext *ctx, arg_wrs_sto *a)
> {
> if (!ctx->cfg_ptr->ext_zawrs) {
> return false;
> @@ -40,12 +40,23 @@ static bool trans_wrs(DisasContext *ctx)
> return true;
> }
>
> -#define GEN_TRANS_WRS(insn) \
> -static bool trans_ ## insn(DisasContext *ctx, arg_ ## insn *a) \
> -{ \
> - (void)a; \
> - return trans_wrs(ctx); \
> -}
> +static bool trans_wrs_nto(DisasContext *ctx, arg_wrs_nto *a)
> +{
> + if (!ctx->cfg_ptr->ext_zawrs) {
> + return false;
> + }
>
> -GEN_TRANS_WRS(wrs_nto)
> -GEN_TRANS_WRS(wrs_sto)
> + /*
> + * Depending on the mode of execution, mstatus.TW and hstatus.VTW, wrs.nto
> + * should raise an exception when the implementation-specific bounded time
> + * limit has expired. Our time limit is zero, so we either return
> + * immediately, as does our implementation of wrs.sto, or raise an
> + * exception, as handled by the wrs.nto helper.
> + */
> +#ifndef CONFIG_USER_ONLY
> + gen_helper_wrs_nto(tcg_env);
> +#endif
> +
> + /* We only get here when helper_wrs_nto() doesn't raise an exception. */
> + return trans_wrs_sto(ctx, NULL);
> +}
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index f414aaebdbab..2baf5bc3ca19 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -380,6 +380,17 @@ void helper_wfi(CPURISCVState *env)
> }
> }
>
> +void helper_wrs_nto(CPURISCVState *env)
> +{
> + if (env->virt_enabled && (env->priv == PRV_S || env->priv == PRV_U) &&
> + get_field(env->hstatus, HSTATUS_VTW) &&
> + !get_field(env->mstatus, MSTATUS_TW)) {
> + riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
> + } else if (env->priv != PRV_M && get_field(env->mstatus, MSTATUS_TW)) {
> + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> + }
> +}
> +
> void helper_tlb_flush(CPURISCVState *env)
> {
> CPUState *cs = env_cpu(env);
> --
> 2.44.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] target/riscv: Raise exceptions on wrs.nto
2024-04-24 14:28 [PATCH v3] target/riscv: Raise exceptions on wrs.nto Andrew Jones
2024-04-24 14:46 ` Andrew Jones
@ 2024-04-26 12:58 ` Daniel Henrique Barboza
2024-04-29 2:10 ` Alistair Francis
2024-04-29 2:12 ` Alistair Francis
3 siblings, 0 replies; 5+ messages in thread
From: Daniel Henrique Barboza @ 2024-04-26 12:58 UTC (permalink / raw)
To: Andrew Jones, qemu-devel, qemu-riscv
Cc: christoph.muellner, palmer, alistair.francis, bin.meng, liwei1518,
zhiwei_liu
On 4/24/24 11:28, Andrew Jones wrote:
> Implementing wrs.nto to always just return is consistent with the
> specification, as the instruction is permitted to terminate the
> stall for any reason, but it's not useful for virtualization, where
> we'd like the guest to trap to the hypervisor in order to allow
> scheduling of the lock holding VCPU. Change to always immediately
> raise exceptions when the appropriate conditions are present,
> otherwise continue to just return. Note, immediately raising
> exceptions is also consistent with the specification since the
> time limit that should expire prior to the exception is
> implementation-specific.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> v3:
> - Sending again, hoping the ü remains in Müllner
> v2:
> - Added #ifndef CONFIG_USER_ONLY around helper call
>
> target/riscv/helper.h | 1 +
> target/riscv/insn_trans/trans_rvzawrs.c.inc | 29 ++++++++++++++-------
> target/riscv/op_helper.c | 11 ++++++++
> 3 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index 8a635238514d..451261ce5a4f 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -132,6 +132,7 @@ DEF_HELPER_6(csrrw_i128, tl, env, int, tl, tl, tl, tl)
> DEF_HELPER_1(sret, tl, env)
> DEF_HELPER_1(mret, tl, env)
> DEF_HELPER_1(wfi, void, env)
> +DEF_HELPER_1(wrs_nto, void, env)
> DEF_HELPER_1(tlb_flush, void, env)
> DEF_HELPER_1(tlb_flush_all, void, env)
> /* Native Debug */
> diff --git a/target/riscv/insn_trans/trans_rvzawrs.c.inc b/target/riscv/insn_trans/trans_rvzawrs.c.inc
> index 32efbff4d5a5..0eef03383889 100644
> --- a/target/riscv/insn_trans/trans_rvzawrs.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzawrs.c.inc
> @@ -16,7 +16,7 @@
> * this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> -static bool trans_wrs(DisasContext *ctx)
> +static bool trans_wrs_sto(DisasContext *ctx, arg_wrs_sto *a)
> {
> if (!ctx->cfg_ptr->ext_zawrs) {
> return false;
> @@ -40,12 +40,23 @@ static bool trans_wrs(DisasContext *ctx)
> return true;
> }
>
> -#define GEN_TRANS_WRS(insn) \
> -static bool trans_ ## insn(DisasContext *ctx, arg_ ## insn *a) \
> -{ \
> - (void)a; \
> - return trans_wrs(ctx); \
> -}
> +static bool trans_wrs_nto(DisasContext *ctx, arg_wrs_nto *a)
> +{
> + if (!ctx->cfg_ptr->ext_zawrs) {
> + return false;
> + }
>
> -GEN_TRANS_WRS(wrs_nto)
> -GEN_TRANS_WRS(wrs_sto)
> + /*
> + * Depending on the mode of execution, mstatus.TW and hstatus.VTW, wrs.nto
> + * should raise an exception when the implementation-specific bounded time
> + * limit has expired. Our time limit is zero, so we either return
> + * immediately, as does our implementation of wrs.sto, or raise an
> + * exception, as handled by the wrs.nto helper.
> + */
> +#ifndef CONFIG_USER_ONLY
> + gen_helper_wrs_nto(tcg_env);
> +#endif
> +
> + /* We only get here when helper_wrs_nto() doesn't raise an exception. */
> + return trans_wrs_sto(ctx, NULL);
> +}
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index f414aaebdbab..2baf5bc3ca19 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -380,6 +380,17 @@ void helper_wfi(CPURISCVState *env)
> }
> }
>
> +void helper_wrs_nto(CPURISCVState *env)
> +{
> + if (env->virt_enabled && (env->priv == PRV_S || env->priv == PRV_U) &&
> + get_field(env->hstatus, HSTATUS_VTW) &&
> + !get_field(env->mstatus, MSTATUS_TW)) {
> + riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
> + } else if (env->priv != PRV_M && get_field(env->mstatus, MSTATUS_TW)) {
> + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> + }
> +}
> +
> void helper_tlb_flush(CPURISCVState *env)
> {
> CPUState *cs = env_cpu(env);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] target/riscv: Raise exceptions on wrs.nto
2024-04-24 14:28 [PATCH v3] target/riscv: Raise exceptions on wrs.nto Andrew Jones
2024-04-24 14:46 ` Andrew Jones
2024-04-26 12:58 ` Daniel Henrique Barboza
@ 2024-04-29 2:10 ` Alistair Francis
2024-04-29 2:12 ` Alistair Francis
3 siblings, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2024-04-29 2:10 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-devel, qemu-riscv, christoph.muellner, palmer,
alistair.francis, bin.meng, liwei1518, dbarboza, zhiwei_liu
On Thu, Apr 25, 2024 at 12:29 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> Implementing wrs.nto to always just return is consistent with the
> specification, as the instruction is permitted to terminate the
> stall for any reason, but it's not useful for virtualization, where
> we'd like the guest to trap to the hypervisor in order to allow
> scheduling of the lock holding VCPU. Change to always immediately
> raise exceptions when the appropriate conditions are present,
> otherwise continue to just return. Note, immediately raising
> exceptions is also consistent with the specification since the
> time limit that should expire prior to the exception is
> implementation-specific.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Christoph Müllner <christoph.muellner@vrull.eu>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> v3:
> - Sending again, hoping the ü remains in Müllner
> v2:
> - Added #ifndef CONFIG_USER_ONLY around helper call
>
> target/riscv/helper.h | 1 +
> target/riscv/insn_trans/trans_rvzawrs.c.inc | 29 ++++++++++++++-------
> target/riscv/op_helper.c | 11 ++++++++
> 3 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index 8a635238514d..451261ce5a4f 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -132,6 +132,7 @@ DEF_HELPER_6(csrrw_i128, tl, env, int, tl, tl, tl, tl)
> DEF_HELPER_1(sret, tl, env)
> DEF_HELPER_1(mret, tl, env)
> DEF_HELPER_1(wfi, void, env)
> +DEF_HELPER_1(wrs_nto, void, env)
> DEF_HELPER_1(tlb_flush, void, env)
> DEF_HELPER_1(tlb_flush_all, void, env)
> /* Native Debug */
> diff --git a/target/riscv/insn_trans/trans_rvzawrs.c.inc b/target/riscv/insn_trans/trans_rvzawrs.c.inc
> index 32efbff4d5a5..0eef03383889 100644
> --- a/target/riscv/insn_trans/trans_rvzawrs.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzawrs.c.inc
> @@ -16,7 +16,7 @@
> * this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> -static bool trans_wrs(DisasContext *ctx)
> +static bool trans_wrs_sto(DisasContext *ctx, arg_wrs_sto *a)
> {
> if (!ctx->cfg_ptr->ext_zawrs) {
> return false;
> @@ -40,12 +40,23 @@ static bool trans_wrs(DisasContext *ctx)
> return true;
> }
>
> -#define GEN_TRANS_WRS(insn) \
> -static bool trans_ ## insn(DisasContext *ctx, arg_ ## insn *a) \
> -{ \
> - (void)a; \
> - return trans_wrs(ctx); \
> -}
> +static bool trans_wrs_nto(DisasContext *ctx, arg_wrs_nto *a)
> +{
> + if (!ctx->cfg_ptr->ext_zawrs) {
> + return false;
> + }
>
> -GEN_TRANS_WRS(wrs_nto)
> -GEN_TRANS_WRS(wrs_sto)
> + /*
> + * Depending on the mode of execution, mstatus.TW and hstatus.VTW, wrs.nto
> + * should raise an exception when the implementation-specific bounded time
> + * limit has expired. Our time limit is zero, so we either return
> + * immediately, as does our implementation of wrs.sto, or raise an
> + * exception, as handled by the wrs.nto helper.
> + */
> +#ifndef CONFIG_USER_ONLY
> + gen_helper_wrs_nto(tcg_env);
> +#endif
> +
> + /* We only get here when helper_wrs_nto() doesn't raise an exception. */
> + return trans_wrs_sto(ctx, NULL);
> +}
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index f414aaebdbab..2baf5bc3ca19 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -380,6 +380,17 @@ void helper_wfi(CPURISCVState *env)
> }
> }
>
> +void helper_wrs_nto(CPURISCVState *env)
> +{
> + if (env->virt_enabled && (env->priv == PRV_S || env->priv == PRV_U) &&
> + get_field(env->hstatus, HSTATUS_VTW) &&
> + !get_field(env->mstatus, MSTATUS_TW)) {
> + riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
> + } else if (env->priv != PRV_M && get_field(env->mstatus, MSTATUS_TW)) {
> + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> + }
> +}
> +
> void helper_tlb_flush(CPURISCVState *env)
> {
> CPUState *cs = env_cpu(env);
> --
> 2.44.0
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] target/riscv: Raise exceptions on wrs.nto
2024-04-24 14:28 [PATCH v3] target/riscv: Raise exceptions on wrs.nto Andrew Jones
` (2 preceding siblings ...)
2024-04-29 2:10 ` Alistair Francis
@ 2024-04-29 2:12 ` Alistair Francis
3 siblings, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2024-04-29 2:12 UTC (permalink / raw)
To: Andrew Jones
Cc: qemu-devel, qemu-riscv, christoph.muellner, palmer,
alistair.francis, bin.meng, liwei1518, dbarboza, zhiwei_liu
On Thu, Apr 25, 2024 at 12:29 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> Implementing wrs.nto to always just return is consistent with the
> specification, as the instruction is permitted to terminate the
> stall for any reason, but it's not useful for virtualization, where
> we'd like the guest to trap to the hypervisor in order to allow
> scheduling of the lock holding VCPU. Change to always immediately
> raise exceptions when the appropriate conditions are present,
> otherwise continue to just return. Note, immediately raising
> exceptions is also consistent with the specification since the
> time limit that should expire prior to the exception is
> implementation-specific.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Christoph Müllner <christoph.muellner@vrull.eu>
Thanks!
Applied to riscv-to-apply.next
Alistair
> ---
> v3:
> - Sending again, hoping the ü remains in Müllner
> v2:
> - Added #ifndef CONFIG_USER_ONLY around helper call
>
> target/riscv/helper.h | 1 +
> target/riscv/insn_trans/trans_rvzawrs.c.inc | 29 ++++++++++++++-------
> target/riscv/op_helper.c | 11 ++++++++
> 3 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/target/riscv/helper.h b/target/riscv/helper.h
> index 8a635238514d..451261ce5a4f 100644
> --- a/target/riscv/helper.h
> +++ b/target/riscv/helper.h
> @@ -132,6 +132,7 @@ DEF_HELPER_6(csrrw_i128, tl, env, int, tl, tl, tl, tl)
> DEF_HELPER_1(sret, tl, env)
> DEF_HELPER_1(mret, tl, env)
> DEF_HELPER_1(wfi, void, env)
> +DEF_HELPER_1(wrs_nto, void, env)
> DEF_HELPER_1(tlb_flush, void, env)
> DEF_HELPER_1(tlb_flush_all, void, env)
> /* Native Debug */
> diff --git a/target/riscv/insn_trans/trans_rvzawrs.c.inc b/target/riscv/insn_trans/trans_rvzawrs.c.inc
> index 32efbff4d5a5..0eef03383889 100644
> --- a/target/riscv/insn_trans/trans_rvzawrs.c.inc
> +++ b/target/riscv/insn_trans/trans_rvzawrs.c.inc
> @@ -16,7 +16,7 @@
> * this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> -static bool trans_wrs(DisasContext *ctx)
> +static bool trans_wrs_sto(DisasContext *ctx, arg_wrs_sto *a)
> {
> if (!ctx->cfg_ptr->ext_zawrs) {
> return false;
> @@ -40,12 +40,23 @@ static bool trans_wrs(DisasContext *ctx)
> return true;
> }
>
> -#define GEN_TRANS_WRS(insn) \
> -static bool trans_ ## insn(DisasContext *ctx, arg_ ## insn *a) \
> -{ \
> - (void)a; \
> - return trans_wrs(ctx); \
> -}
> +static bool trans_wrs_nto(DisasContext *ctx, arg_wrs_nto *a)
> +{
> + if (!ctx->cfg_ptr->ext_zawrs) {
> + return false;
> + }
>
> -GEN_TRANS_WRS(wrs_nto)
> -GEN_TRANS_WRS(wrs_sto)
> + /*
> + * Depending on the mode of execution, mstatus.TW and hstatus.VTW, wrs.nto
> + * should raise an exception when the implementation-specific bounded time
> + * limit has expired. Our time limit is zero, so we either return
> + * immediately, as does our implementation of wrs.sto, or raise an
> + * exception, as handled by the wrs.nto helper.
> + */
> +#ifndef CONFIG_USER_ONLY
> + gen_helper_wrs_nto(tcg_env);
> +#endif
> +
> + /* We only get here when helper_wrs_nto() doesn't raise an exception. */
> + return trans_wrs_sto(ctx, NULL);
> +}
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index f414aaebdbab..2baf5bc3ca19 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -380,6 +380,17 @@ void helper_wfi(CPURISCVState *env)
> }
> }
>
> +void helper_wrs_nto(CPURISCVState *env)
> +{
> + if (env->virt_enabled && (env->priv == PRV_S || env->priv == PRV_U) &&
> + get_field(env->hstatus, HSTATUS_VTW) &&
> + !get_field(env->mstatus, MSTATUS_TW)) {
> + riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
> + } else if (env->priv != PRV_M && get_field(env->mstatus, MSTATUS_TW)) {
> + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> + }
> +}
> +
> void helper_tlb_flush(CPURISCVState *env)
> {
> CPUState *cs = env_cpu(env);
> --
> 2.44.0
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread