* [PATCH 0/4] target/riscv: Fix mstatus related problems
@ 2023-05-29 12:17 Weiwei Li
2023-05-29 12:17 ` [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M Weiwei Li
` (3 more replies)
0 siblings, 4 replies; 32+ messages in thread
From: Weiwei Li @ 2023-05-29 12:17 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
wangjunqiang, lazyparser, Weiwei Li
This patchset tries to fix some problems in the fields of mstatus, such as make MPV only work when MPP != PRM.
The port is available here:
https://github.com/plctlab/plct-qemu/tree/plct-mpv-upstream
Weiwei Li (4):
target/riscv: Make MPV only work when MPP != PRV_M
target/riscv: Remove check on mode for MPRV
target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
target/riscv: Remove redundant assignment to SXL
target/riscv/cpu_helper.c | 5 +++--
target/riscv/csr.c | 14 ++++----------
target/riscv/op_helper.c | 3 ++-
3 files changed, 9 insertions(+), 13 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M
2023-05-29 12:17 [PATCH 0/4] target/riscv: Fix mstatus related problems Weiwei Li
@ 2023-05-29 12:17 ` Weiwei Li
2023-05-30 20:23 ` Daniel Henrique Barboza
` (3 more replies)
2023-05-29 12:17 ` [PATCH 2/4] target/riscv: Remove check on mode for MPRV Weiwei Li
` (2 subsequent siblings)
3 siblings, 4 replies; 32+ messages in thread
From: Weiwei Li @ 2023-05-29 12:17 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
wangjunqiang, lazyparser, Weiwei Li
Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
when MPP=PRV_M.
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/cpu_helper.c | 3 ++-
target/riscv/op_helper.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 09ea227ceb..bd892c05d4 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
mode = get_field(env->mstatus, MSTATUS_MPP);
- virt = get_field(env->mstatus, MSTATUS_MPV);
+ virt = get_field(env->mstatus, MSTATUS_MPV) &&
+ (mode != PRV_M);
if (virt) {
status = env->vsstatus;
}
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index f563dc3981..9cdb9cdd06 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
}
- target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
+ target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
+ (prev_priv != PRV_M);
mstatus = set_field(mstatus, MSTATUS_MIE,
get_field(mstatus, MSTATUS_MPIE));
mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
--
2.25.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/4] target/riscv: Remove check on mode for MPRV
2023-05-29 12:17 [PATCH 0/4] target/riscv: Fix mstatus related problems Weiwei Li
2023-05-29 12:17 ` [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M Weiwei Li
@ 2023-05-29 12:17 ` Weiwei Li
2023-05-30 20:25 ` Daniel Henrique Barboza
` (2 more replies)
2023-05-29 12:17 ` [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled Weiwei Li
2023-05-29 12:17 ` [PATCH 4/4] target/riscv: Remove redundant assignment to SXL Weiwei Li
3 siblings, 3 replies; 32+ messages in thread
From: Weiwei Li @ 2023-05-29 12:17 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
wangjunqiang, lazyparser, Weiwei Li
Normally, MPRV can be set to 1 only in M mode (It will be cleared
when returning to lower-privilege mode by MRET/SRET).
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/cpu_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index bd892c05d4..45baf95c77 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
if (!ifetch) {
uint64_t status = env->mstatus;
- if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
+ if (get_field(status, MSTATUS_MPRV)) {
mode = get_field(env->mstatus, MSTATUS_MPP);
virt = get_field(env->mstatus, MSTATUS_MPV) &&
(mode != PRV_M);
--
2.25.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
2023-05-29 12:17 [PATCH 0/4] target/riscv: Fix mstatus related problems Weiwei Li
2023-05-29 12:17 ` [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M Weiwei Li
2023-05-29 12:17 ` [PATCH 2/4] target/riscv: Remove check on mode for MPRV Weiwei Li
@ 2023-05-29 12:17 ` Weiwei Li
2023-05-30 20:26 ` Daniel Henrique Barboza
` (2 more replies)
2023-05-29 12:17 ` [PATCH 4/4] target/riscv: Remove redundant assignment to SXL Weiwei Li
3 siblings, 3 replies; 32+ messages in thread
From: Weiwei Li @ 2023-05-29 12:17 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
wangjunqiang, lazyparser, Weiwei Li
MPV and GVA bits are added by hypervisor extension to mstatus
and mstatush (if MXLEN=32).
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/csr.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 58499b5afc..6ac11d1f11 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1311,11 +1311,9 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
}
if (xl != MXL_RV32 || env->debugger) {
- /*
- * RV32: MPV and GVA are not in mstatus. The current plan is to
- * add them to mstatush. For now, we just don't support it.
- */
- mask |= MSTATUS_MPV | MSTATUS_GVA;
+ if (riscv_has_ext(env, RVH)) {
+ mask |= MSTATUS_MPV | MSTATUS_GVA;
+ }
if ((val & MSTATUS64_UXL) != 0) {
mask |= MSTATUS64_UXL;
}
@@ -1351,7 +1349,7 @@ static RISCVException write_mstatush(CPURISCVState *env, int csrno,
target_ulong val)
{
uint64_t valh = (uint64_t)val << 32;
- uint64_t mask = MSTATUS_MPV | MSTATUS_GVA;
+ uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV | MSTATUS_GVA : 0;
env->mstatus = (env->mstatus & ~mask) | (valh & mask);
--
2.25.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 4/4] target/riscv: Remove redundant assignment to SXL
2023-05-29 12:17 [PATCH 0/4] target/riscv: Fix mstatus related problems Weiwei Li
` (2 preceding siblings ...)
2023-05-29 12:17 ` [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled Weiwei Li
@ 2023-05-29 12:17 ` Weiwei Li
2023-05-30 20:28 ` Daniel Henrique Barboza
` (2 more replies)
3 siblings, 3 replies; 32+ messages in thread
From: Weiwei Li @ 2023-05-29 12:17 UTC (permalink / raw)
To: qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, zhiwei_liu,
wangjunqiang, lazyparser, Weiwei Li
SXL is initialized as env->misa_mxl which is also the mxl value.
So we can just remain it unchanged to keep it read-only.
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
target/riscv/csr.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 6ac11d1f11..25345f3153 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -1321,10 +1321,6 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
mstatus = (mstatus & ~mask) | (val & mask);
- if (xl > MXL_RV32) {
- /* SXL field is for now read only */
- mstatus = set_field(mstatus, MSTATUS64_SXL, xl);
- }
env->mstatus = mstatus;
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M
2023-05-29 12:17 ` [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M Weiwei Li
@ 2023-05-30 20:23 ` Daniel Henrique Barboza
2023-05-31 0:27 ` Weiwei Li
2023-05-31 9:28 ` Daniel Henrique Barboza
` (2 subsequent siblings)
3 siblings, 1 reply; 32+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-30 20:23 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang, lazyparser
On 5/29/23 09:17, Weiwei Li wrote:
> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
> when MPP=PRV_M.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/cpu_helper.c | 3 ++-
> target/riscv/op_helper.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 09ea227ceb..bd892c05d4 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>
> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> mode = get_field(env->mstatus, MSTATUS_MPP);
> - virt = get_field(env->mstatus, MSTATUS_MPV);
> + virt = get_field(env->mstatus, MSTATUS_MPV) &&
> + (mode != PRV_M);
This change makes more sense in patch 2 where you also removed the 'mode'
check for MPRV. As it is now I read the code above and thought "but mode
is guaranteed to be == PRV_M, so (mode != PRV_M) is guaranteed to be
false every time".
The change in helper_mret() below is fine.
Thanks,
Daniel
> if (virt) {
> status = env->vsstatus;
> }
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index f563dc3981..9cdb9cdd06 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
> }
>
> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
> + (prev_priv != PRV_M);
> mstatus = set_field(mstatus, MSTATUS_MIE,
> get_field(mstatus, MSTATUS_MPIE));
> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
2023-05-29 12:17 ` [PATCH 2/4] target/riscv: Remove check on mode for MPRV Weiwei Li
@ 2023-05-30 20:25 ` Daniel Henrique Barboza
2023-05-31 9:28 ` Daniel Henrique Barboza
2023-06-01 5:27 ` Alistair Francis
2 siblings, 0 replies; 32+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-30 20:25 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang, lazyparser
On 5/29/23 09:17, Weiwei Li wrote:
> Normally, MPRV can be set to 1 only in M mode (It will be cleared
> when returning to lower-privilege mode by MRET/SRET).
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/cpu_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index bd892c05d4..45baf95c77 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> if (!ifetch) {
> uint64_t status = env->mstatus;
>
> - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> + if (get_field(status, MSTATUS_MPRV)) {
As I mentioned in patch 1 this is a good place to put this change:
- virt = get_field(env->mstatus, MSTATUS_MPV);
+ virt = get_field(env->mstatus, MSTATUS_MPV) &&
+ (mode != PRV_M);
if (virt) {
status = env->vsstatus;
Thanks,
Daniel
> mode = get_field(env->mstatus, MSTATUS_MPP);
> virt = get_field(env->mstatus, MSTATUS_MPV) &&
> (mode != PRV_M);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
2023-05-29 12:17 ` [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled Weiwei Li
@ 2023-05-30 20:26 ` Daniel Henrique Barboza
2023-06-01 5:28 ` Alistair Francis
2023-06-12 3:08 ` LIU Zhiwei
2 siblings, 0 replies; 32+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-30 20:26 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang, lazyparser
On 5/29/23 09:17, Weiwei Li wrote:
> MPV and GVA bits are added by hypervisor extension to mstatus
> and mstatush (if MXLEN=32).
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/csr.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 58499b5afc..6ac11d1f11 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1311,11 +1311,9 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
> }
>
> if (xl != MXL_RV32 || env->debugger) {
> - /*
> - * RV32: MPV and GVA are not in mstatus. The current plan is to
> - * add them to mstatush. For now, we just don't support it.
> - */
> - mask |= MSTATUS_MPV | MSTATUS_GVA;
> + if (riscv_has_ext(env, RVH)) {
> + mask |= MSTATUS_MPV | MSTATUS_GVA;
> + }
> if ((val & MSTATUS64_UXL) != 0) {
> mask |= MSTATUS64_UXL;
> }
> @@ -1351,7 +1349,7 @@ static RISCVException write_mstatush(CPURISCVState *env, int csrno,
> target_ulong val)
> {
> uint64_t valh = (uint64_t)val << 32;
> - uint64_t mask = MSTATUS_MPV | MSTATUS_GVA;
> + uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV | MSTATUS_GVA : 0;
>
> env->mstatus = (env->mstatus & ~mask) | (valh & mask);
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] target/riscv: Remove redundant assignment to SXL
2023-05-29 12:17 ` [PATCH 4/4] target/riscv: Remove redundant assignment to SXL Weiwei Li
@ 2023-05-30 20:28 ` Daniel Henrique Barboza
2023-06-01 5:31 ` Alistair Francis
2023-06-12 5:55 ` LIU Zhiwei
2 siblings, 0 replies; 32+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-30 20:28 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang, lazyparser
On 5/29/23 09:17, Weiwei Li wrote:
> SXL is initialized as env->misa_mxl which is also the mxl value.
> So we can just remain it unchanged to keep it read-only.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/csr.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 6ac11d1f11..25345f3153 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1321,10 +1321,6 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>
> mstatus = (mstatus & ~mask) | (val & mask);
>
> - if (xl > MXL_RV32) {
> - /* SXL field is for now read only */
> - mstatus = set_field(mstatus, MSTATUS64_SXL, xl);
> - }
> env->mstatus = mstatus;
>
> /*
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M
2023-05-30 20:23 ` Daniel Henrique Barboza
@ 2023-05-31 0:27 ` Weiwei Li
2023-05-31 9:28 ` Daniel Henrique Barboza
0 siblings, 1 reply; 32+ messages in thread
From: Weiwei Li @ 2023-05-31 0:27 UTC (permalink / raw)
To: Daniel Henrique Barboza, Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang, lazyparser
On 2023/5/31 04:23, Daniel Henrique Barboza wrote:
>
>
> On 5/29/23 09:17, Weiwei Li wrote:
>> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
>> when MPP=PRV_M.
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>> target/riscv/cpu_helper.c | 3 ++-
>> target/riscv/op_helper.c | 3 ++-
>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 09ea227ceb..bd892c05d4 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool
>> ifetch)
>> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
>> mode = get_field(env->mstatus, MSTATUS_MPP);
>> - virt = get_field(env->mstatus, MSTATUS_MPV);
>> + virt = get_field(env->mstatus, MSTATUS_MPV) &&
>> + (mode != PRV_M);
>
> This change makes more sense in patch 2 where you also removed the 'mode'
> check for MPRV. As it is now I read the code above and thought "but mode
> is guaranteed to be == PRV_M, so (mode != PRV_M) is guaranteed to be
> false every time".
No, this 'mode' (get from MPP) is not the previous 'mode'(the current
privilege mode).
Regards,
Weiwei Li
>
> The change in helper_mret() below is fine.
>
> Thanks,
>
> Daniel
>
>> if (virt) {
>> status = env->vsstatus;
>> }
>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>> index f563dc3981..9cdb9cdd06 100644
>> --- a/target/riscv/op_helper.c
>> +++ b/target/riscv/op_helper.c
>> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
>> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT,
>> GETPC());
>> }
>> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
>> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
>> + (prev_priv != PRV_M);
>> mstatus = set_field(mstatus, MSTATUS_MIE,
>> get_field(mstatus, MSTATUS_MPIE));
>> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M
2023-05-31 0:27 ` Weiwei Li
@ 2023-05-31 9:28 ` Daniel Henrique Barboza
0 siblings, 0 replies; 32+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-31 9:28 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang, lazyparser
On 5/30/23 21:27, Weiwei Li wrote:
>
> On 2023/5/31 04:23, Daniel Henrique Barboza wrote:
>>
>>
>> On 5/29/23 09:17, Weiwei Li wrote:
>>> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
>>> when MPP=PRV_M.
>>>
>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>> ---
>>> target/riscv/cpu_helper.c | 3 ++-
>>> target/riscv/op_helper.c | 3 ++-
>>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>> index 09ea227ceb..bd892c05d4 100644
>>> --- a/target/riscv/cpu_helper.c
>>> +++ b/target/riscv/cpu_helper.c
>>> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>>> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
>>> mode = get_field(env->mstatus, MSTATUS_MPP);
>>> - virt = get_field(env->mstatus, MSTATUS_MPV);
>>> + virt = get_field(env->mstatus, MSTATUS_MPV) &&
>>> + (mode != PRV_M);
>>
>> This change makes more sense in patch 2 where you also removed the 'mode'
>> check for MPRV. As it is now I read the code above and thought "but mode
>> is guaranteed to be == PRV_M, so (mode != PRV_M) is guaranteed to be
>> false every time".
>
> No, this 'mode' (get from MPP) is not the previous 'mode'(the current privilege mode).
Oh, right:
>>> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
>>> mode = get_field(env->mstatus, MSTATUS_MPP);
'mode' is being reassigned here.
>
> Regards,
>
> Weiwei Li
>
>>
>> The change in helper_mret() below is fine.
>>
>> Thanks,
>>
>> Daniel
>>
>>> if (virt) {
>>> status = env->vsstatus;
>>> }
>>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>>> index f563dc3981..9cdb9cdd06 100644
>>> --- a/target/riscv/op_helper.c
>>> +++ b/target/riscv/op_helper.c
>>> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
>>> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
>>> }
>>> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
>>> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
>>> + (prev_priv != PRV_M);
>>> mstatus = set_field(mstatus, MSTATUS_MIE,
>>> get_field(mstatus, MSTATUS_MPIE));
>>> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M
2023-05-29 12:17 ` [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M Weiwei Li
2023-05-30 20:23 ` Daniel Henrique Barboza
@ 2023-05-31 9:28 ` Daniel Henrique Barboza
2023-06-01 5:24 ` Alistair Francis
2023-06-12 2:45 ` LIU Zhiwei
3 siblings, 0 replies; 32+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-31 9:28 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang, lazyparser
On 5/29/23 09:17, Weiwei Li wrote:
> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
> when MPP=PRV_M.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/cpu_helper.c | 3 ++-
> target/riscv/op_helper.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 09ea227ceb..bd892c05d4 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>
> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> mode = get_field(env->mstatus, MSTATUS_MPP);
> - virt = get_field(env->mstatus, MSTATUS_MPV);
> + virt = get_field(env->mstatus, MSTATUS_MPV) &&
> + (mode != PRV_M);
> if (virt) {
> status = env->vsstatus;
> }
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index f563dc3981..9cdb9cdd06 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
> }
>
> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
> + (prev_priv != PRV_M);
> mstatus = set_field(mstatus, MSTATUS_MIE,
> get_field(mstatus, MSTATUS_MPIE));
> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
2023-05-29 12:17 ` [PATCH 2/4] target/riscv: Remove check on mode for MPRV Weiwei Li
2023-05-30 20:25 ` Daniel Henrique Barboza
@ 2023-05-31 9:28 ` Daniel Henrique Barboza
2023-06-01 5:27 ` Alistair Francis
2 siblings, 0 replies; 32+ messages in thread
From: Daniel Henrique Barboza @ 2023-05-31 9:28 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, zhiwei_liu, wangjunqiang, lazyparser
On 5/29/23 09:17, Weiwei Li wrote:
> Normally, MPRV can be set to 1 only in M mode (It will be cleared
> when returning to lower-privilege mode by MRET/SRET).
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/riscv/cpu_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index bd892c05d4..45baf95c77 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> if (!ifetch) {
> uint64_t status = env->mstatus;
>
> - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> + if (get_field(status, MSTATUS_MPRV)) {
> mode = get_field(env->mstatus, MSTATUS_MPP);
> virt = get_field(env->mstatus, MSTATUS_MPV) &&
> (mode != PRV_M);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M
2023-05-29 12:17 ` [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M Weiwei Li
2023-05-30 20:23 ` Daniel Henrique Barboza
2023-05-31 9:28 ` Daniel Henrique Barboza
@ 2023-06-01 5:24 ` Alistair Francis
2023-06-12 2:45 ` LIU Zhiwei
3 siblings, 0 replies; 32+ messages in thread
From: Alistair Francis @ 2023-06-01 5:24 UTC (permalink / raw)
To: Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, wangjunqiang, lazyparser
On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
> when MPP=PRV_M.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/cpu_helper.c | 3 ++-
> target/riscv/op_helper.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 09ea227ceb..bd892c05d4 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>
> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> mode = get_field(env->mstatus, MSTATUS_MPP);
> - virt = get_field(env->mstatus, MSTATUS_MPV);
> + virt = get_field(env->mstatus, MSTATUS_MPV) &&
> + (mode != PRV_M);
> if (virt) {
> status = env->vsstatus;
> }
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index f563dc3981..9cdb9cdd06 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
> }
>
> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
> + (prev_priv != PRV_M);
> mstatus = set_field(mstatus, MSTATUS_MIE,
> get_field(mstatus, MSTATUS_MPIE));
> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
2023-05-29 12:17 ` [PATCH 2/4] target/riscv: Remove check on mode for MPRV Weiwei Li
2023-05-30 20:25 ` Daniel Henrique Barboza
2023-05-31 9:28 ` Daniel Henrique Barboza
@ 2023-06-01 5:27 ` Alistair Francis
2023-06-01 6:43 ` Weiwei Li
2 siblings, 1 reply; 32+ messages in thread
From: Alistair Francis @ 2023-06-01 5:27 UTC (permalink / raw)
To: Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, wangjunqiang, lazyparser
On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> Normally, MPRV can be set to 1 only in M mode (It will be cleared
> when returning to lower-privilege mode by MRET/SRET).
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/cpu_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index bd892c05d4..45baf95c77 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> if (!ifetch) {
> uint64_t status = env->mstatus;
>
> - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> + if (get_field(status, MSTATUS_MPRV)) {
The original check is correct though, why remove it?
Alistair
> mode = get_field(env->mstatus, MSTATUS_MPP);
> virt = get_field(env->mstatus, MSTATUS_MPV) &&
> (mode != PRV_M);
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
2023-05-29 12:17 ` [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled Weiwei Li
2023-05-30 20:26 ` Daniel Henrique Barboza
@ 2023-06-01 5:28 ` Alistair Francis
2023-06-12 3:08 ` LIU Zhiwei
2 siblings, 0 replies; 32+ messages in thread
From: Alistair Francis @ 2023-06-01 5:28 UTC (permalink / raw)
To: Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, wangjunqiang, lazyparser
On Mon, May 29, 2023 at 10:18 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> MPV and GVA bits are added by hypervisor extension to mstatus
> and mstatush (if MXLEN=32).
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/csr.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 58499b5afc..6ac11d1f11 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1311,11 +1311,9 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
> }
>
> if (xl != MXL_RV32 || env->debugger) {
> - /*
> - * RV32: MPV and GVA are not in mstatus. The current plan is to
> - * add them to mstatush. For now, we just don't support it.
> - */
> - mask |= MSTATUS_MPV | MSTATUS_GVA;
> + if (riscv_has_ext(env, RVH)) {
> + mask |= MSTATUS_MPV | MSTATUS_GVA;
> + }
> if ((val & MSTATUS64_UXL) != 0) {
> mask |= MSTATUS64_UXL;
> }
> @@ -1351,7 +1349,7 @@ static RISCVException write_mstatush(CPURISCVState *env, int csrno,
> target_ulong val)
> {
> uint64_t valh = (uint64_t)val << 32;
> - uint64_t mask = MSTATUS_MPV | MSTATUS_GVA;
> + uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV | MSTATUS_GVA : 0;
>
> env->mstatus = (env->mstatus & ~mask) | (valh & mask);
>
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] target/riscv: Remove redundant assignment to SXL
2023-05-29 12:17 ` [PATCH 4/4] target/riscv: Remove redundant assignment to SXL Weiwei Li
2023-05-30 20:28 ` Daniel Henrique Barboza
@ 2023-06-01 5:31 ` Alistair Francis
2023-06-12 5:55 ` LIU Zhiwei
2 siblings, 0 replies; 32+ messages in thread
From: Alistair Francis @ 2023-06-01 5:31 UTC (permalink / raw)
To: Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, wangjunqiang, lazyparser
On Mon, May 29, 2023 at 10:18 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
> SXL is initialized as env->misa_mxl which is also the mxl value.
> So we can just remain it unchanged to keep it read-only.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
> target/riscv/csr.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 6ac11d1f11..25345f3153 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1321,10 +1321,6 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>
> mstatus = (mstatus & ~mask) | (val & mask);
>
> - if (xl > MXL_RV32) {
> - /* SXL field is for now read only */
> - mstatus = set_field(mstatus, MSTATUS64_SXL, xl);
> - }
> env->mstatus = mstatus;
>
> /*
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
2023-06-01 5:27 ` Alistair Francis
@ 2023-06-01 6:43 ` Weiwei Li
2023-06-01 23:03 ` Alistair Francis
0 siblings, 1 reply; 32+ messages in thread
From: Weiwei Li @ 2023-06-01 6:43 UTC (permalink / raw)
To: Alistair Francis, Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, wangjunqiang, lazyparser
On 2023/6/1 13:27, Alistair Francis wrote:
> On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>> Normally, MPRV can be set to 1 only in M mode (It will be cleared
>> when returning to lower-privilege mode by MRET/SRET).
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>> target/riscv/cpu_helper.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index bd892c05d4..45baf95c77 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>> if (!ifetch) {
>> uint64_t status = env->mstatus;
>>
>> - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
>> + if (get_field(status, MSTATUS_MPRV)) {
> The original check is correct though, why remove it?
Yeah. As described in the commit message, I think MPRV can only be set
to 1 in M mode normally
which means check on MPRV is enough in this case. So I remove the check
on mode here.
Regards,
Weiwei Li
>
> Alistair
>
>> mode = get_field(env->mstatus, MSTATUS_MPP);
>> virt = get_field(env->mstatus, MSTATUS_MPV) &&
>> (mode != PRV_M);
>> --
>> 2.25.1
>>
>>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
2023-06-01 6:43 ` Weiwei Li
@ 2023-06-01 23:03 ` Alistair Francis
2023-06-02 1:31 ` Weiwei Li
0 siblings, 1 reply; 32+ messages in thread
From: Alistair Francis @ 2023-06-01 23:03 UTC (permalink / raw)
To: Weiwei Li
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, wangjunqiang, lazyparser
On Thu, Jun 1, 2023 at 4:43 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>
>
> On 2023/6/1 13:27, Alistair Francis wrote:
> > On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
> >> Normally, MPRV can be set to 1 only in M mode (It will be cleared
> >> when returning to lower-privilege mode by MRET/SRET).
> >>
> >> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> >> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> >> ---
> >> target/riscv/cpu_helper.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index bd892c05d4..45baf95c77 100644
> >> --- a/target/riscv/cpu_helper.c
> >> +++ b/target/riscv/cpu_helper.c
> >> @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> >> if (!ifetch) {
> >> uint64_t status = env->mstatus;
> >>
> >> - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> >> + if (get_field(status, MSTATUS_MPRV)) {
> > The original check is correct though, why remove it?
>
> Yeah. As described in the commit message, I think MPRV can only be set
> to 1 in M mode normally
That's true. I do feel that keeping the check makes the code easier to
follow. Otherwise future developers need to check to see how MPRV can
be set. The current code is explicit and obviously follows the spec.
For a performance gain I think it's worth making the trade off, but it
doesn't sound like we really get any gain here.
Alistair
>
> which means check on MPRV is enough in this case. So I remove the check
> on mode here.
>
> Regards,
>
> Weiwei Li
>
> >
> > Alistair
> >
> >> mode = get_field(env->mstatus, MSTATUS_MPP);
> >> virt = get_field(env->mstatus, MSTATUS_MPV) &&
> >> (mode != PRV_M);
> >> --
> >> 2.25.1
> >>
> >>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
2023-06-01 23:03 ` Alistair Francis
@ 2023-06-02 1:31 ` Weiwei Li
2023-06-02 21:01 ` Richard Henderson
0 siblings, 1 reply; 32+ messages in thread
From: Weiwei Li @ 2023-06-02 1:31 UTC (permalink / raw)
To: Alistair Francis
Cc: liweiwei, qemu-riscv, qemu-devel, palmer, alistair.francis,
bin.meng, dbarboza, zhiwei_liu, wangjunqiang, lazyparser
On 2023/6/2 07:03, Alistair Francis wrote:
> On Thu, Jun 1, 2023 at 4:43 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>>
>> On 2023/6/1 13:27, Alistair Francis wrote:
>>> On Mon, May 29, 2023 at 10:19 PM Weiwei Li <liweiwei@iscas.ac.cn> wrote:
>>>> Normally, MPRV can be set to 1 only in M mode (It will be cleared
>>>> when returning to lower-privilege mode by MRET/SRET).
>>>>
>>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>>> ---
>>>> target/riscv/cpu_helper.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>> index bd892c05d4..45baf95c77 100644
>>>> --- a/target/riscv/cpu_helper.c
>>>> +++ b/target/riscv/cpu_helper.c
>>>> @@ -44,7 +44,7 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>>>> if (!ifetch) {
>>>> uint64_t status = env->mstatus;
>>>>
>>>> - if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
>>>> + if (get_field(status, MSTATUS_MPRV)) {
>>> The original check is correct though, why remove it?
>> Yeah. As described in the commit message, I think MPRV can only be set
>> to 1 in M mode normally
> That's true. I do feel that keeping the check makes the code easier to
> follow. Otherwise future developers need to check to see how MPRV can
> be set. The current code is explicit and obviously follows the spec.
>
> For a performance gain I think it's worth making the trade off, but it
> doesn't sound like we really get any gain here.
Yeah. It's acceptable to me.
Just another question: whether MPRV is truly limited to work on M mode?
I can only find following description in the note:
"The MPRV and MXR mechanisms *were* conceived to improve the efficiency
of M-mode routines
that emulate missing hardware features, e.g., misaligned loads and stores."
To some degree, It seems not limit them to work on other mode.
Even though MPRV normally can be set to 1 in M mode, it seems possible
to set it to 1 in other mode by gdbstub.
Regards,
Weiwei Li
> Alistair
>
>> which means check on MPRV is enough in this case. So I remove the check
>> on mode here.
>>
>> Regards,
>>
>> Weiwei Li
>>
>>> Alistair
>>>
>>>> mode = get_field(env->mstatus, MSTATUS_MPP);
>>>> virt = get_field(env->mstatus, MSTATUS_MPV) &&
>>>> (mode != PRV_M);
>>>> --
>>>> 2.25.1
>>>>
>>>>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
2023-06-02 1:31 ` Weiwei Li
@ 2023-06-02 21:01 ` Richard Henderson
2023-06-03 13:46 ` Weiwei Li
0 siblings, 1 reply; 32+ messages in thread
From: Richard Henderson @ 2023-06-02 21:01 UTC (permalink / raw)
To: Weiwei Li, Alistair Francis
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, wangjunqiang, lazyparser
On 6/1/23 18:31, Weiwei Li wrote:
> Even though MPRV normally can be set to 1 in M mode, it seems possible to set it to 1 in
> other mode by gdbstub.
That would seem to be a gdbstub bug, since it is cleared on exit from M-mode, and cannot
be set again until we re-enter M-mode.
r~
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/4] target/riscv: Remove check on mode for MPRV
2023-06-02 21:01 ` Richard Henderson
@ 2023-06-03 13:46 ` Weiwei Li
0 siblings, 0 replies; 32+ messages in thread
From: Weiwei Li @ 2023-06-03 13:46 UTC (permalink / raw)
To: Richard Henderson, Weiwei Li, Alistair Francis
Cc: qemu-riscv, qemu-devel, palmer, alistair.francis, bin.meng,
dbarboza, zhiwei_liu, wangjunqiang, lazyparser
On 2023/6/3 05:01, Richard Henderson wrote:
> On 6/1/23 18:31, Weiwei Li wrote:
>> Even though MPRV normally can be set to 1 in M mode, it seems
>> possible to set it to 1 in other mode by gdbstub.
>
> That would seem to be a gdbstub bug, since it is cleared on exit from
> M-mode, and cannot be set again until we re-enter M-mode.
Yeah, MPRV normally can be set only in M mode. However, gdbstub can
bypass the check on privilege mode for CSRs access inriscv_csrrw_check.
Regards,
Weiwei Li
>
>
> r~
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M
2023-05-29 12:17 ` [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M Weiwei Li
` (2 preceding siblings ...)
2023-06-01 5:24 ` Alistair Francis
@ 2023-06-12 2:45 ` LIU Zhiwei
2023-06-12 3:10 ` Weiwei Li
3 siblings, 1 reply; 32+ messages in thread
From: LIU Zhiwei @ 2023-06-12 2:45 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, wangjunqiang, lazyparser
On 2023/5/29 20:17, Weiwei Li wrote:
> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
> when MPP=PRV_M.
Does MPP==PRV_M always indicate the MPV==0?
Zhiwei
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/cpu_helper.c | 3 ++-
> target/riscv/op_helper.c | 3 ++-
> 2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 09ea227ceb..bd892c05d4 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>
> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
> mode = get_field(env->mstatus, MSTATUS_MPP);
> - virt = get_field(env->mstatus, MSTATUS_MPV);
> + virt = get_field(env->mstatus, MSTATUS_MPV) &&
> + (mode != PRV_M);
> if (virt) {
> status = env->vsstatus;
> }
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index f563dc3981..9cdb9cdd06 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT, GETPC());
> }
>
> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
> + (prev_priv != PRV_M);
> mstatus = set_field(mstatus, MSTATUS_MIE,
> get_field(mstatus, MSTATUS_MPIE));
> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
2023-05-29 12:17 ` [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled Weiwei Li
2023-05-30 20:26 ` Daniel Henrique Barboza
2023-06-01 5:28 ` Alistair Francis
@ 2023-06-12 3:08 ` LIU Zhiwei
2023-06-12 3:16 ` Weiwei Li
2 siblings, 1 reply; 32+ messages in thread
From: LIU Zhiwei @ 2023-06-12 3:08 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, wangjunqiang, lazyparser
On 2023/5/29 20:17, Weiwei Li wrote:
> MPV and GVA bits are added by hypervisor extension to mstatus
> and mstatush (if MXLEN=32).
Have you found the CSR field specifications for them, especially for GVA.
Zhiwei
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/csr.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 58499b5afc..6ac11d1f11 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1311,11 +1311,9 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
> }
>
> if (xl != MXL_RV32 || env->debugger) {
> - /*
> - * RV32: MPV and GVA are not in mstatus. The current plan is to
> - * add them to mstatush. For now, we just don't support it.
> - */
> - mask |= MSTATUS_MPV | MSTATUS_GVA;
> + if (riscv_has_ext(env, RVH)) {
> + mask |= MSTATUS_MPV | MSTATUS_GVA;
> + }
> if ((val & MSTATUS64_UXL) != 0) {
> mask |= MSTATUS64_UXL;
> }
> @@ -1351,7 +1349,7 @@ static RISCVException write_mstatush(CPURISCVState *env, int csrno,
> target_ulong val)
> {
> uint64_t valh = (uint64_t)val << 32;
> - uint64_t mask = MSTATUS_MPV | MSTATUS_GVA;
> + uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV | MSTATUS_GVA : 0;
>
> env->mstatus = (env->mstatus & ~mask) | (valh & mask);
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M
2023-06-12 2:45 ` LIU Zhiwei
@ 2023-06-12 3:10 ` Weiwei Li
2023-06-12 3:30 ` LIU Zhiwei
0 siblings, 1 reply; 32+ messages in thread
From: Weiwei Li @ 2023-06-12 3:10 UTC (permalink / raw)
To: LIU Zhiwei, qemu-riscv, qemu-devel
Cc: liweiwei, palmer, alistair.francis, bin.meng, dbarboza,
wangjunqiang, lazyparser
On 2023/6/12 10:45, LIU Zhiwei wrote:
>
> On 2023/5/29 20:17, Weiwei Li wrote:
>> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
>> when MPP=PRV_M.
> Does MPP==PRV_M always indicate the MPV==0?
No, I think . The spec doesn't restrict this. When MPP=PRV_M, MPV wll be
0 in normal case.
But users can set MPV=1 by write to mstatus CSR directly.
As described in spec, "When an MRET instruction is executed, the
virtualization mode V is set to MPV, unless
MPP=3, in which case V remains 0."
MPV is just ignored if MPP = 3. This also can be seen in "table 9.5
Effect of MPRV on the translation and protection of explicit memory
accesses".
Regards,
Weiwei Li
>
> Zhiwei
>
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>> target/riscv/cpu_helper.c | 3 ++-
>> target/riscv/op_helper.c | 3 ++-
>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 09ea227ceb..bd892c05d4 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool
>> ifetch)
>> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
>> mode = get_field(env->mstatus, MSTATUS_MPP);
>> - virt = get_field(env->mstatus, MSTATUS_MPV);
>> + virt = get_field(env->mstatus, MSTATUS_MPV) &&
>> + (mode != PRV_M);
>> if (virt) {
>> status = env->vsstatus;
>> }
>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>> index f563dc3981..9cdb9cdd06 100644
>> --- a/target/riscv/op_helper.c
>> +++ b/target/riscv/op_helper.c
>> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
>> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT,
>> GETPC());
>> }
>> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
>> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
>> + (prev_priv != PRV_M);
>> mstatus = set_field(mstatus, MSTATUS_MIE,
>> get_field(mstatus, MSTATUS_MPIE));
>> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
2023-06-12 3:08 ` LIU Zhiwei
@ 2023-06-12 3:16 ` Weiwei Li
2023-06-12 3:18 ` LIU Zhiwei
0 siblings, 1 reply; 32+ messages in thread
From: Weiwei Li @ 2023-06-12 3:16 UTC (permalink / raw)
To: LIU Zhiwei, Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, wangjunqiang, lazyparser
On 2023/6/12 11:08, LIU Zhiwei wrote:
>
> On 2023/5/29 20:17, Weiwei Li wrote:
>> MPV and GVA bits are added by hypervisor extension to mstatus
>> and mstatush (if MXLEN=32).
>
> Have you found the CSR field specifications for them, especially for GVA.
Yeah. in the section 9.4.1 of the privilege spec:
"/The hypervisor extension adds two fields, MPV and GVA, to the
machine-level mstatus or mstatush CSR/".
Regards,
Weiwei Li
>
> Zhiwei
>
>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>> target/riscv/csr.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>> index 58499b5afc..6ac11d1f11 100644
>> --- a/target/riscv/csr.c
>> +++ b/target/riscv/csr.c
>> @@ -1311,11 +1311,9 @@ static RISCVException
>> write_mstatus(CPURISCVState *env, int csrno,
>> }
>> if (xl != MXL_RV32 || env->debugger) {
>> - /*
>> - * RV32: MPV and GVA are not in mstatus. The current plan is to
>> - * add them to mstatush. For now, we just don't support it.
>> - */
>> - mask |= MSTATUS_MPV | MSTATUS_GVA;
>> + if (riscv_has_ext(env, RVH)) {
>> + mask |= MSTATUS_MPV | MSTATUS_GVA;
>> + }
>> if ((val & MSTATUS64_UXL) != 0) {
>> mask |= MSTATUS64_UXL;
>> }
>> @@ -1351,7 +1349,7 @@ static RISCVException
>> write_mstatush(CPURISCVState *env, int csrno,
>> target_ulong val)
>> {
>> uint64_t valh = (uint64_t)val << 32;
>> - uint64_t mask = MSTATUS_MPV | MSTATUS_GVA;
>> + uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV |
>> MSTATUS_GVA : 0;
>> env->mstatus = (env->mstatus & ~mask) | (valh & mask);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
2023-06-12 3:16 ` Weiwei Li
@ 2023-06-12 3:18 ` LIU Zhiwei
2023-06-12 4:35 ` Weiwei Li
0 siblings, 1 reply; 32+ messages in thread
From: LIU Zhiwei @ 2023-06-12 3:18 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, wangjunqiang, lazyparser
On 2023/6/12 11:16, Weiwei Li wrote:
>
> On 2023/6/12 11:08, LIU Zhiwei wrote:
>>
>> On 2023/5/29 20:17, Weiwei Li wrote:
>>> MPV and GVA bits are added by hypervisor extension to mstatus
>>> and mstatush (if MXLEN=32).
>>
>> Have you found the CSR field specifications for them, especially for
>> GVA.
>
> Yeah. in the section 9.4.1 of the privilege spec:
>
> "/The hypervisor extension adds two fields, MPV and GVA, to the
> machine-level mstatus or mstatush CSR/".
I mean the WARL or other CSR field specifications here.
Zhiwei
>
> Regards,
>
> Weiwei Li
>
>>
>> Zhiwei
>>
>>>
>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>> ---
>>> target/riscv/csr.c | 10 ++++------
>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>> index 58499b5afc..6ac11d1f11 100644
>>> --- a/target/riscv/csr.c
>>> +++ b/target/riscv/csr.c
>>> @@ -1311,11 +1311,9 @@ static RISCVException
>>> write_mstatus(CPURISCVState *env, int csrno,
>>> }
>>> if (xl != MXL_RV32 || env->debugger) {
>>> - /*
>>> - * RV32: MPV and GVA are not in mstatus. The current plan
>>> is to
>>> - * add them to mstatush. For now, we just don't support it.
>>> - */
>>> - mask |= MSTATUS_MPV | MSTATUS_GVA;
>>> + if (riscv_has_ext(env, RVH)) {
>>> + mask |= MSTATUS_MPV | MSTATUS_GVA;
>>> + }
>>> if ((val & MSTATUS64_UXL) != 0) {
>>> mask |= MSTATUS64_UXL;
>>> }
>>> @@ -1351,7 +1349,7 @@ static RISCVException
>>> write_mstatush(CPURISCVState *env, int csrno,
>>> target_ulong val)
>>> {
>>> uint64_t valh = (uint64_t)val << 32;
>>> - uint64_t mask = MSTATUS_MPV | MSTATUS_GVA;
>>> + uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV |
>>> MSTATUS_GVA : 0;
>>> env->mstatus = (env->mstatus & ~mask) | (valh & mask);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M
2023-06-12 3:10 ` Weiwei Li
@ 2023-06-12 3:30 ` LIU Zhiwei
0 siblings, 0 replies; 32+ messages in thread
From: LIU Zhiwei @ 2023-06-12 3:30 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, wangjunqiang, lazyparser
On 2023/6/12 11:10, Weiwei Li wrote:
>
> On 2023/6/12 10:45, LIU Zhiwei wrote:
>>
>> On 2023/5/29 20:17, Weiwei Li wrote:
>>> Upon MRET or explicit memory access with MPRV=1, MPV should be ignored
>>> when MPP=PRV_M.
>> Does MPP==PRV_M always indicate the MPV==0?
>
> No, I think . The spec doesn't restrict this. When MPP=PRV_M, MPV wll
> be 0 in normal case.
>
> But users can set MPV=1 by write to mstatus CSR directly.
Make sense. The fields specification(WARL and others) of mstatus and
other CSR always not clear on the specification. Maybe I missed
something. But I think this field could be written by the mode software.
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Zhiwei
>
> As described in spec, "When an MRET instruction is executed, the
> virtualization mode V is set to MPV, unless
>
> MPP=3, in which case V remains 0."
>
> MPV is just ignored if MPP = 3. This also can be seen in "table 9.5
> Effect of MPRV on the translation and protection of explicit memory
> accesses".
>
> Regards,
>
> Weiwei Li
>
>>
>> Zhiwei
>>
>>>
>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>> ---
>>> target/riscv/cpu_helper.c | 3 ++-
>>> target/riscv/op_helper.c | 3 ++-
>>> 2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>> index 09ea227ceb..bd892c05d4 100644
>>> --- a/target/riscv/cpu_helper.c
>>> +++ b/target/riscv/cpu_helper.c
>>> @@ -46,7 +46,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool
>>> ifetch)
>>> if (mode == PRV_M && get_field(status, MSTATUS_MPRV)) {
>>> mode = get_field(env->mstatus, MSTATUS_MPP);
>>> - virt = get_field(env->mstatus, MSTATUS_MPV);
>>> + virt = get_field(env->mstatus, MSTATUS_MPV) &&
>>> + (mode != PRV_M);
>>> if (virt) {
>>> status = env->vsstatus;
>>> }
>>> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
>>> index f563dc3981..9cdb9cdd06 100644
>>> --- a/target/riscv/op_helper.c
>>> +++ b/target/riscv/op_helper.c
>>> @@ -335,7 +335,8 @@ target_ulong helper_mret(CPURISCVState *env)
>>> riscv_raise_exception(env, RISCV_EXCP_INST_ACCESS_FAULT,
>>> GETPC());
>>> }
>>> - target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV);
>>> + target_ulong prev_virt = get_field(env->mstatus, MSTATUS_MPV) &&
>>> + (prev_priv != PRV_M);
>>> mstatus = set_field(mstatus, MSTATUS_MIE,
>>> get_field(mstatus, MSTATUS_MPIE));
>>> mstatus = set_field(mstatus, MSTATUS_MPIE, 1);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
2023-06-12 3:18 ` LIU Zhiwei
@ 2023-06-12 4:35 ` Weiwei Li
2023-06-12 5:40 ` LIU Zhiwei
0 siblings, 1 reply; 32+ messages in thread
From: Weiwei Li @ 2023-06-12 4:35 UTC (permalink / raw)
To: LIU Zhiwei, qemu-riscv, qemu-devel
Cc: liweiwei, palmer, alistair.francis, bin.meng, dbarboza,
wangjunqiang, lazyparser
On 2023/6/12 11:18, LIU Zhiwei wrote:
>
> On 2023/6/12 11:16, Weiwei Li wrote:
>>
>> On 2023/6/12 11:08, LIU Zhiwei wrote:
>>>
>>> On 2023/5/29 20:17, Weiwei Li wrote:
>>>> MPV and GVA bits are added by hypervisor extension to mstatus
>>>> and mstatush (if MXLEN=32).
>>>
>>> Have you found the CSR field specifications for them, especially for
>>> GVA.
>>
>> Yeah. in the section 9.4.1 of the privilege spec:
>>
>> "/The hypervisor extension adds two fields, MPV and GVA, to the
>> machine-level mstatus or mstatush CSR/".
>
> I mean the WARL or other CSR field specifications here.
I don't quite get your idea. The only specification for MPV and GVA I
found is in section 9.4.1. The spec for most of mstatus fields can be
found in Section 3.1.6
"Machine Status Registers (mstatus and mstatush)".
Regards,
Weiwei Li
>
> Zhiwei
>
>>
>> Regards,
>>
>> Weiwei Li
>>
>>>
>>> Zhiwei
>>>
>>>>
>>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>>> ---
>>>> target/riscv/csr.c | 10 ++++------
>>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>> index 58499b5afc..6ac11d1f11 100644
>>>> --- a/target/riscv/csr.c
>>>> +++ b/target/riscv/csr.c
>>>> @@ -1311,11 +1311,9 @@ static RISCVException
>>>> write_mstatus(CPURISCVState *env, int csrno,
>>>> }
>>>> if (xl != MXL_RV32 || env->debugger) {
>>>> - /*
>>>> - * RV32: MPV and GVA are not in mstatus. The current plan
>>>> is to
>>>> - * add them to mstatush. For now, we just don't support it.
>>>> - */
>>>> - mask |= MSTATUS_MPV | MSTATUS_GVA;
>>>> + if (riscv_has_ext(env, RVH)) {
>>>> + mask |= MSTATUS_MPV | MSTATUS_GVA;
>>>> + }
>>>> if ((val & MSTATUS64_UXL) != 0) {
>>>> mask |= MSTATUS64_UXL;
>>>> }
>>>> @@ -1351,7 +1349,7 @@ static RISCVException
>>>> write_mstatush(CPURISCVState *env, int csrno,
>>>> target_ulong val)
>>>> {
>>>> uint64_t valh = (uint64_t)val << 32;
>>>> - uint64_t mask = MSTATUS_MPV | MSTATUS_GVA;
>>>> + uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV |
>>>> MSTATUS_GVA : 0;
>>>> env->mstatus = (env->mstatus & ~mask) | (valh & mask);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
2023-06-12 4:35 ` Weiwei Li
@ 2023-06-12 5:40 ` LIU Zhiwei
2023-06-12 7:27 ` Weiwei Li
0 siblings, 1 reply; 32+ messages in thread
From: LIU Zhiwei @ 2023-06-12 5:40 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, wangjunqiang, lazyparser
On 2023/6/12 12:35, Weiwei Li wrote:
>
> On 2023/6/12 11:18, LIU Zhiwei wrote:
>>
>> On 2023/6/12 11:16, Weiwei Li wrote:
>>>
>>> On 2023/6/12 11:08, LIU Zhiwei wrote:
>>>>
>>>> On 2023/5/29 20:17, Weiwei Li wrote:
>>>>> MPV and GVA bits are added by hypervisor extension to mstatus
>>>>> and mstatush (if MXLEN=32).
>>>>
>>>> Have you found the CSR field specifications for them, especially
>>>> for GVA.
>>>
>>> Yeah. in the section 9.4.1 of the privilege spec:
>>>
>>> "/The hypervisor extension adds two fields, MPV and GVA, to the
>>> machine-level mstatus or mstatush CSR/".
>>
>> I mean the WARL or other CSR field specifications here.
>
> I don't quite get your idea. The only specification for MPV and GVA I
> found is in section 9.4.1. The spec for most of mstatus fields can be
> found in Section 3.1.6
> "Machine Status Registers (mstatus and mstatush)".
I mean is the GVA field read only or WARL(WPRI or WLRL) for the
software? It could be written by the implementation. But I am not sure
whether it could be written by the software.
Zhiwei
>
> Regards,
>
> Weiwei Li
>
>>
>> Zhiwei
>>
>>>
>>> Regards,
>>>
>>> Weiwei Li
>>>
>>>>
>>>> Zhiwei
>>>>
>>>>>
>>>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>>>> ---
>>>>> target/riscv/csr.c | 10 ++++------
>>>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>>> index 58499b5afc..6ac11d1f11 100644
>>>>> --- a/target/riscv/csr.c
>>>>> +++ b/target/riscv/csr.c
>>>>> @@ -1311,11 +1311,9 @@ static RISCVException
>>>>> write_mstatus(CPURISCVState *env, int csrno,
>>>>> }
>>>>> if (xl != MXL_RV32 || env->debugger) {
>>>>> - /*
>>>>> - * RV32: MPV and GVA are not in mstatus. The current plan
>>>>> is to
>>>>> - * add them to mstatush. For now, we just don't support it.
>>>>> - */
>>>>> - mask |= MSTATUS_MPV | MSTATUS_GVA;
>>>>> + if (riscv_has_ext(env, RVH)) {
>>>>> + mask |= MSTATUS_MPV | MSTATUS_GVA;
>>>>> + }
>>>>> if ((val & MSTATUS64_UXL) != 0) {
>>>>> mask |= MSTATUS64_UXL;
>>>>> }
>>>>> @@ -1351,7 +1349,7 @@ static RISCVException
>>>>> write_mstatush(CPURISCVState *env, int csrno,
>>>>> target_ulong val)
>>>>> {
>>>>> uint64_t valh = (uint64_t)val << 32;
>>>>> - uint64_t mask = MSTATUS_MPV | MSTATUS_GVA;
>>>>> + uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV |
>>>>> MSTATUS_GVA : 0;
>>>>> env->mstatus = (env->mstatus & ~mask) | (valh & mask);
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] target/riscv: Remove redundant assignment to SXL
2023-05-29 12:17 ` [PATCH 4/4] target/riscv: Remove redundant assignment to SXL Weiwei Li
2023-05-30 20:28 ` Daniel Henrique Barboza
2023-06-01 5:31 ` Alistair Francis
@ 2023-06-12 5:55 ` LIU Zhiwei
2 siblings, 0 replies; 32+ messages in thread
From: LIU Zhiwei @ 2023-06-12 5:55 UTC (permalink / raw)
To: Weiwei Li, qemu-riscv, qemu-devel
Cc: palmer, alistair.francis, bin.meng, dbarboza, wangjunqiang, lazyparser
On 2023/5/29 20:17, Weiwei Li wrote:
> SXL is initialized as env->misa_mxl which is also the mxl value.
> So we can just remain it unchanged to keep it read-only.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
> target/riscv/csr.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 6ac11d1f11..25345f3153 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -1321,10 +1321,6 @@ static RISCVException write_mstatus(CPURISCVState *env, int csrno,
>
> mstatus = (mstatus & ~mask) | (val & mask);
>
> - if (xl > MXL_RV32) {
> - /* SXL field is for now read only */
> - mstatus = set_field(mstatus, MSTATUS64_SXL, xl);
> - }
Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>
Zhiwei
> env->mstatus = mstatus;
>
> /*
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled
2023-06-12 5:40 ` LIU Zhiwei
@ 2023-06-12 7:27 ` Weiwei Li
0 siblings, 0 replies; 32+ messages in thread
From: Weiwei Li @ 2023-06-12 7:27 UTC (permalink / raw)
To: LIU Zhiwei, qemu-riscv, qemu-devel
Cc: liweiwei, palmer, alistair.francis, bin.meng, dbarboza,
wangjunqiang, lazyparser
On 2023/6/12 13:40, LIU Zhiwei wrote:
>
> On 2023/6/12 12:35, Weiwei Li wrote:
>>
>> On 2023/6/12 11:18, LIU Zhiwei wrote:
>>>
>>> On 2023/6/12 11:16, Weiwei Li wrote:
>>>>
>>>> On 2023/6/12 11:08, LIU Zhiwei wrote:
>>>>>
>>>>> On 2023/5/29 20:17, Weiwei Li wrote:
>>>>>> MPV and GVA bits are added by hypervisor extension to mstatus
>>>>>> and mstatush (if MXLEN=32).
>>>>>
>>>>> Have you found the CSR field specifications for them, especially
>>>>> for GVA.
>>>>
>>>> Yeah. in the section 9.4.1 of the privilege spec:
>>>>
>>>> "/The hypervisor extension adds two fields, MPV and GVA, to the
>>>> machine-level mstatus or mstatush CSR/".
>>>
>>> I mean the WARL or other CSR field specifications here.
>>
>> I don't quite get your idea. The only specification for MPV and GVA
>> I found is in section 9.4.1. The spec for most of mstatus fields can
>> be found in Section 3.1.6
>> "Machine Status Registers (mstatus and mstatush)".
>
> I mean is the GVA field read only or WARL(WPRI or WLRL) for the
> software? It could be written by the implementation. But I am not sure
> whether it could be written by the software.
No, I didn't find any description about this.
Regards,
Weiwei Li
>
> Zhiwei
>
>>
>> Regards,
>>
>> Weiwei Li
>>
>>>
>>> Zhiwei
>>>
>>>>
>>>> Regards,
>>>>
>>>> Weiwei Li
>>>>
>>>>>
>>>>> Zhiwei
>>>>>
>>>>>>
>>>>>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>>>>>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>>>>>> ---
>>>>>> target/riscv/csr.c | 10 ++++------
>>>>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
>>>>>> index 58499b5afc..6ac11d1f11 100644
>>>>>> --- a/target/riscv/csr.c
>>>>>> +++ b/target/riscv/csr.c
>>>>>> @@ -1311,11 +1311,9 @@ static RISCVException
>>>>>> write_mstatus(CPURISCVState *env, int csrno,
>>>>>> }
>>>>>> if (xl != MXL_RV32 || env->debugger) {
>>>>>> - /*
>>>>>> - * RV32: MPV and GVA are not in mstatus. The current
>>>>>> plan is to
>>>>>> - * add them to mstatush. For now, we just don't support it.
>>>>>> - */
>>>>>> - mask |= MSTATUS_MPV | MSTATUS_GVA;
>>>>>> + if (riscv_has_ext(env, RVH)) {
>>>>>> + mask |= MSTATUS_MPV | MSTATUS_GVA;
>>>>>> + }
>>>>>> if ((val & MSTATUS64_UXL) != 0) {
>>>>>> mask |= MSTATUS64_UXL;
>>>>>> }
>>>>>> @@ -1351,7 +1349,7 @@ static RISCVException
>>>>>> write_mstatush(CPURISCVState *env, int csrno,
>>>>>> target_ulong val)
>>>>>> {
>>>>>> uint64_t valh = (uint64_t)val << 32;
>>>>>> - uint64_t mask = MSTATUS_MPV | MSTATUS_GVA;
>>>>>> + uint64_t mask = riscv_has_ext(env, RVH) ? MSTATUS_MPV |
>>>>>> MSTATUS_GVA : 0;
>>>>>> env->mstatus = (env->mstatus & ~mask) | (valh & mask);
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2023-06-12 7:27 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-29 12:17 [PATCH 0/4] target/riscv: Fix mstatus related problems Weiwei Li
2023-05-29 12:17 ` [PATCH 1/4] target/riscv: Make MPV only work when MPP != PRV_M Weiwei Li
2023-05-30 20:23 ` Daniel Henrique Barboza
2023-05-31 0:27 ` Weiwei Li
2023-05-31 9:28 ` Daniel Henrique Barboza
2023-05-31 9:28 ` Daniel Henrique Barboza
2023-06-01 5:24 ` Alistair Francis
2023-06-12 2:45 ` LIU Zhiwei
2023-06-12 3:10 ` Weiwei Li
2023-06-12 3:30 ` LIU Zhiwei
2023-05-29 12:17 ` [PATCH 2/4] target/riscv: Remove check on mode for MPRV Weiwei Li
2023-05-30 20:25 ` Daniel Henrique Barboza
2023-05-31 9:28 ` Daniel Henrique Barboza
2023-06-01 5:27 ` Alistair Francis
2023-06-01 6:43 ` Weiwei Li
2023-06-01 23:03 ` Alistair Francis
2023-06-02 1:31 ` Weiwei Li
2023-06-02 21:01 ` Richard Henderson
2023-06-03 13:46 ` Weiwei Li
2023-05-29 12:17 ` [PATCH 3/4] target/riscv: Support MSTATUS.MPV/GVA only when RVH is enabled Weiwei Li
2023-05-30 20:26 ` Daniel Henrique Barboza
2023-06-01 5:28 ` Alistair Francis
2023-06-12 3:08 ` LIU Zhiwei
2023-06-12 3:16 ` Weiwei Li
2023-06-12 3:18 ` LIU Zhiwei
2023-06-12 4:35 ` Weiwei Li
2023-06-12 5:40 ` LIU Zhiwei
2023-06-12 7:27 ` Weiwei Li
2023-05-29 12:17 ` [PATCH 4/4] target/riscv: Remove redundant assignment to SXL Weiwei Li
2023-05-30 20:28 ` Daniel Henrique Barboza
2023-06-01 5:31 ` Alistair Francis
2023-06-12 5:55 ` LIU Zhiwei
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).