1. Patchew
  2. QEMU
  3. [PATCH 00/11] Add support for the T-Head vendor extensions
  4. View patch

[PATCH 01/11] riscv: Add privilege level to DisasContext

Christoph Muellner posted 11 patches 3 years, 5 months ago
There is a newer version of this series
[PATCH 01/11] riscv: Add privilege level to DisasContext
Posted by Christoph Muellner 3 years, 5 months ago 
From: Christoph Müllner <christoph.muellner@vrull.eu>

This allows privileged instructions to check the required
privilege level in the translation without calling a helper.

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
---
 target/riscv/translate.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 63b04e8a94..fd241ff667 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -59,6 +59,9 @@ typedef struct DisasContext {
     /* pc_succ_insn points to the instruction following base.pc_next */
     target_ulong pc_succ_insn;
     target_ulong priv_ver;
+#ifndef CONFIG_USER_ONLY
+    target_ulong priv;
+#endif
     RISCVMXL misa_mxl_max;
     RISCVMXL xl;
     uint32_t misa_ext;
@@ -1079,6 +1082,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->mstatus_vs = tb_flags & TB_FLAGS_MSTATUS_VS;
     ctx->priv_ver = env->priv_ver;
 #if !defined(CONFIG_USER_ONLY)
+    ctx->priv = env->priv;
     if (riscv_has_ext(env, RVH)) {
         ctx->virt_enabled = riscv_cpu_virt_enabled(env);
     } else {
-- 
2.37.2
Re: [PATCH 01/11] riscv: Add privilege level to DisasContext
Posted by Richard Henderson 3 years, 4 months ago 
On 9/6/22 14:22, Christoph Muellner wrote:
> From: Christoph Müllner <christoph.muellner@vrull.eu>
> 
> This allows privileged instructions to check the required
> privilege level in the translation without calling a helper.
> 
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
>   target/riscv/translate.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 63b04e8a94..fd241ff667 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -59,6 +59,9 @@ typedef struct DisasContext {
>       /* pc_succ_insn points to the instruction following base.pc_next */
>       target_ulong pc_succ_insn;
>       target_ulong priv_ver;
> +#ifndef CONFIG_USER_ONLY
> +    target_ulong priv;
> +#endif
>       RISCVMXL misa_mxl_max;
>       RISCVMXL xl;
>       uint32_t misa_ext;
> @@ -1079,6 +1082,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>       ctx->mstatus_vs = tb_flags & TB_FLAGS_MSTATUS_VS;
>       ctx->priv_ver = env->priv_ver;
>   #if !defined(CONFIG_USER_ONLY)
> +    ctx->priv = env->priv;

Reading directly from env here is, in general, wrong.  Items that are constant, like 
priv_ver, are ok.  But otherwise these values must be recorded by cpu_get_tb_cpu_state().

This instance will happen to work, because the execution context is already constrained. 
In this case because env->priv == ctx->mem_idx (see cpu_mmu_index) so, really, you don't 
need this new field at all.  Or, keep the field, because it's usage will be more 
self-documentary, but copy the value from ctx->mmu_idx and add a comment.


>       if (riscv_has_ext(env, RVH)) {
>           ctx->virt_enabled = riscv_cpu_virt_enabled(env);
>       } else {

Incidentally, this (existing) usage appears to be a bug.  I can see nothing in 
cpu_get_tb_cpu_state that corresponds directly to this value.


r~
Re: [PATCH 01/11] riscv: Add privilege level to DisasContext
Posted by LIU Zhiwei 3 years, 4 months ago 
On 2022/9/16 14:00, Richard Henderson wrote:
> On 9/6/22 14:22, Christoph Muellner wrote:
>> From: Christoph Müllner <christoph.muellner@vrull.eu>
>>
>> This allows privileged instructions to check the required
>> privilege level in the translation without calling a helper.
>>
>> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
>> ---
>>   target/riscv/translate.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>> index 63b04e8a94..fd241ff667 100644
>> --- a/target/riscv/translate.c
>> +++ b/target/riscv/translate.c
>> @@ -59,6 +59,9 @@ typedef struct DisasContext {
>>       /* pc_succ_insn points to the instruction following 
>> base.pc_next */
>>       target_ulong pc_succ_insn;
>>       target_ulong priv_ver;
>> +#ifndef CONFIG_USER_ONLY
>> +    target_ulong priv;
>> +#endif
>>       RISCVMXL misa_mxl_max;
>>       RISCVMXL xl;
>>       uint32_t misa_ext;
>> @@ -1079,6 +1082,7 @@ static void 
>> riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>>       ctx->mstatus_vs = tb_flags & TB_FLAGS_MSTATUS_VS;
>>       ctx->priv_ver = env->priv_ver;
>>   #if !defined(CONFIG_USER_ONLY)
>> +    ctx->priv = env->priv;
>
> Reading directly from env here is, in general, wrong.  Items that are 
> constant, like priv_ver, are ok.  But otherwise these values must be 
> recorded by cpu_get_tb_cpu_state().
>
> This instance will happen to work, because the execution context is 
> already constrained. 

Exactly. Thanks for pointing it out.

> In this case because env->priv == ctx->mem_idx (see cpu_mmu_index) so, 
> really, you don't need this new field at all.  Or, keep the field, 
> because it's usage will be more self-documentary, but copy the value 
> from ctx->mmu_idx and add a comment.
>
>
>>       if (riscv_has_ext(env, RVH)) {
>>           ctx->virt_enabled = riscv_cpu_virt_enabled(env);
>>       } else {
>
> Incidentally, this (existing) usage appears to be a bug.  I can see 
> nothing in cpu_get_tb_cpu_state that corresponds directly to this value.

Agree.

Zhiwei

>
>
> r~
Re: [PATCH 01/11] riscv: Add privilege level to DisasContext
Posted by Richard Henderson 3 years, 4 months ago 
On 9/16/22 08:00, Richard Henderson wrote:
> Or, keep the field, because it's usage will be more self-documentary, but copy the value 
> from ctx->mmu_idx and add a comment.

Or, add an inline function like

static inline int priv_level(DisasContext *ctx)
{
#ifdef CONFIG_USER_ONLY
     return PRV_U;
#else
     /* Priv level equals mmu index -- see cpu_mmu_index. */
     return ctx->mmu_idx;
#endif
}

so that usages within a user-only build are compile-time constant and folded away.


r~
Re: [PATCH 01/11] riscv: Add privilege level to DisasContext
Posted by LIU Zhiwei 3 years, 4 months ago 
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

On 2022/9/6 20:22, Christoph Muellner wrote:
> From: Christoph Müllner <christoph.muellner@vrull.eu>
>
> This allows privileged instructions to check the required
> privilege level in the translation without calling a helper.
>
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
>   target/riscv/translate.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 63b04e8a94..fd241ff667 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -59,6 +59,9 @@ typedef struct DisasContext {
>       /* pc_succ_insn points to the instruction following base.pc_next */
>       target_ulong pc_succ_insn;
>       target_ulong priv_ver;
> +#ifndef CONFIG_USER_ONLY
> +    target_ulong priv;
> +#endif
>       RISCVMXL misa_mxl_max;
>       RISCVMXL xl;
>       uint32_t misa_ext;
> @@ -1079,6 +1082,7 @@ static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>       ctx->mstatus_vs = tb_flags & TB_FLAGS_MSTATUS_VS;
>       ctx->priv_ver = env->priv_ver;
>   #if !defined(CONFIG_USER_ONLY)
> +    ctx->priv = env->priv;
>       if (riscv_has_ext(env, RVH)) {
>           ctx->virt_enabled = riscv_cpu_virt_enabled(env);
>       } else {

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.