[PATCH 0/5] target/riscv: Fix pointer mask related support

[PATCH 0/5] target/riscv: Fix pointer mask related support

[Weiwei Li]

This patchset tries to fix some problems in current implementation for pointer
mask extension, and add support for pointer mask of instruction fetch.

The port is available here:
https://github.com/plctlab/plct-qemu/tree/plct-pm-fix

Weiwei Li (5):
  target/riscv: Fix effective address for pointer mask
  target/riscv: Use sign-extended data address when xl = 32
  target/riscv: Fix pointer mask transformation for vector address
  target/riscv: take xl into consideration for vector address
  target/riscv: Add pointer mask support for instruction fetch

 target/riscv/cpu.h           |  1 +
 target/riscv/cpu_helper.c    | 25 +++++++++++++++++++++++--
 target/riscv/csr.c           |  2 --
 target/riscv/translate.c     | 16 ++++++++++++----
 target/riscv/vector_helper.c |  5 ++++-
 5 files changed, 40 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH 1/5] target/riscv: Fix effective address for pointer mask

[Weiwei Li]

Since pointer mask works on effective address, and the xl works on the
generation of effective address, so xl related calculation should be done
before pointer mask.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/translate.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index 0ee8ee147d..bf0e2d318e 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -568,11 +568,15 @@ static TCGv get_address(DisasContext *ctx, int rs1, int imm)
     TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
 
     tcg_gen_addi_tl(addr, src1, imm);
+
+    if (get_xl(ctx) == MXL_RV32) {
+        tcg_gen_ext32u_tl(addr, addr);
+    }
+
     if (ctx->pm_mask_enabled) {
         tcg_gen_andc_tl(addr, addr, pm_mask);
-    } else if (get_xl(ctx) == MXL_RV32) {
-        tcg_gen_ext32u_tl(addr, addr);
     }
+
     if (ctx->pm_base_enabled) {
         tcg_gen_or_tl(addr, addr, pm_base);
     }
@@ -586,11 +590,15 @@ static TCGv get_address_indexed(DisasContext *ctx, int rs1, TCGv offs)
     TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
 
     tcg_gen_add_tl(addr, src1, offs);
+
+    if (get_xl(ctx) == MXL_RV32) {
+        tcg_gen_ext32u_tl(addr, addr);
+    }
+
     if (ctx->pm_mask_enabled) {
         tcg_gen_andc_tl(addr, addr, pm_mask);
-    } else if (get_xl(ctx) == MXL_RV32) {
-        tcg_gen_ext32u_tl(addr, addr);
     }
+
     if (ctx->pm_base_enabled) {
         tcg_gen_or_tl(addr, addr, pm_base);
     }
-- 
2.25.1

Re: [PATCH 1/5] target/riscv: Fix effective address for pointer mask

[Daniel Henrique Barboza]

On 3/27/23 07:00, Weiwei Li wrote:
> Since pointer mask works on effective address, and the xl works on the
> generation of effective address, so xl related calculation should be done

nit: I believe you can remove the 'so'

> before pointer mask.
> 
> 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/translate.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 0ee8ee147d..bf0e2d318e 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -568,11 +568,15 @@ static TCGv get_address(DisasContext *ctx, int rs1, int imm)
>       TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
>   
>       tcg_gen_addi_tl(addr, src1, imm);
> +
> +    if (get_xl(ctx) == MXL_RV32) {
> +        tcg_gen_ext32u_tl(addr, addr);
> +    }
> +
>       if (ctx->pm_mask_enabled) {
>           tcg_gen_andc_tl(addr, addr, pm_mask);
> -    } else if (get_xl(ctx) == MXL_RV32) {
> -        tcg_gen_ext32u_tl(addr, addr);
>       }
> +
>       if (ctx->pm_base_enabled) {
>           tcg_gen_or_tl(addr, addr, pm_base);
>       }
> @@ -586,11 +590,15 @@ static TCGv get_address_indexed(DisasContext *ctx, int rs1, TCGv offs)
>       TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
>   
>       tcg_gen_add_tl(addr, src1, offs);
> +
> +    if (get_xl(ctx) == MXL_RV32) {
> +        tcg_gen_ext32u_tl(addr, addr);
> +    }
> +
>       if (ctx->pm_mask_enabled) {
>           tcg_gen_andc_tl(addr, addr, pm_mask);
> -    } else if (get_xl(ctx) == MXL_RV32) {
> -        tcg_gen_ext32u_tl(addr, addr);
>       }
> +
>       if (ctx->pm_base_enabled) {
>           tcg_gen_or_tl(addr, addr, pm_base);
>       }

Re: [PATCH 1/5] target/riscv: Fix effective address for pointer mask

[LIU Zhiwei]

On 2023/3/27 18:00, Weiwei Li wrote:
> Since pointer mask works on effective address, and the xl works on the
> generation of effective address, so xl related calculation should be done
> before pointer mask.

Incorrect. It has been done.

When updating the pm_mask,  we have already considered the env->xl.

You can see it in riscv_cpu_update_mask

     if (env->xl == MXL_RV32) {
         env->cur_pmmask = mask & UINT32_MAX;
         env->cur_pmbase = base & UINT32_MAX;
     } else {
         env->cur_pmmask = mask;
         env->cur_pmbase = base;
     }

>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/translate.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index 0ee8ee147d..bf0e2d318e 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -568,11 +568,15 @@ static TCGv get_address(DisasContext *ctx, int rs1, int imm)
>       TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
>   
>       tcg_gen_addi_tl(addr, src1, imm);
> +
> +    if (get_xl(ctx) == MXL_RV32) {
> +        tcg_gen_ext32u_tl(addr, addr);
> +    }
> +
>       if (ctx->pm_mask_enabled) {
>           tcg_gen_andc_tl(addr, addr, pm_mask);
> -    } else if (get_xl(ctx) == MXL_RV32) {
> -        tcg_gen_ext32u_tl(addr, addr);
>       }

The else is processing when only xl works, and the pm_mask doesn't work.

Zhiwei

> +
>       if (ctx->pm_base_enabled) {
>           tcg_gen_or_tl(addr, addr, pm_base);
>       }
> @@ -586,11 +590,15 @@ static TCGv get_address_indexed(DisasContext *ctx, int rs1, TCGv offs)
>       TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
>   
>       tcg_gen_add_tl(addr, src1, offs);
> +
> +    if (get_xl(ctx) == MXL_RV32) {
> +        tcg_gen_ext32u_tl(addr, addr);
> +    }
> +
>       if (ctx->pm_mask_enabled) {
>           tcg_gen_andc_tl(addr, addr, pm_mask);
> -    } else if (get_xl(ctx) == MXL_RV32) {
> -        tcg_gen_ext32u_tl(addr, addr);
>       }
> +
>       if (ctx->pm_base_enabled) {
>           tcg_gen_or_tl(addr, addr, pm_base);
>       }

Re: [PATCH 1/5] target/riscv: Fix effective address for pointer mask

[liweiwei]

On 2023/3/28 10:20, LIU Zhiwei wrote:
>
> On 2023/3/27 18:00, Weiwei Li wrote:
>> Since pointer mask works on effective address, and the xl works on the
>> generation of effective address, so xl related calculation should be 
>> done
>> before pointer mask.
>
> Incorrect. It has been done.
>
> When updating the pm_mask,  we have already considered the env->xl.
>
> You can see it in riscv_cpu_update_mask
>
>     if (env->xl == MXL_RV32) {
>         env->cur_pmmask = mask & UINT32_MAX;
>         env->cur_pmbase = base & UINT32_MAX;
>     } else {
>         env->cur_pmmask = mask;
>         env->cur_pmbase = base;
>     }
>
Yeah, I missed this part. Then we should ensure cur_pmmask/base is 
updated when xl changes.

If so, I'll drop this patch.

Regards,

Weiwei Li

>>
>> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
>> ---
>>   target/riscv/translate.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>> index 0ee8ee147d..bf0e2d318e 100644
>> --- a/target/riscv/translate.c
>> +++ b/target/riscv/translate.c
>> @@ -568,11 +568,15 @@ static TCGv get_address(DisasContext *ctx, int 
>> rs1, int imm)
>>       TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
>>         tcg_gen_addi_tl(addr, src1, imm);
>> +
>> +    if (get_xl(ctx) == MXL_RV32) {
>> +        tcg_gen_ext32u_tl(addr, addr);
>> +    }
>> +
>>       if (ctx->pm_mask_enabled) {
>>           tcg_gen_andc_tl(addr, addr, pm_mask);
>> -    } else if (get_xl(ctx) == MXL_RV32) {
>> -        tcg_gen_ext32u_tl(addr, addr);
>>       }
>
> The else is processing when only xl works, and the pm_mask doesn't work.
>
> Zhiwei
>
>> +
>>       if (ctx->pm_base_enabled) {
>>           tcg_gen_or_tl(addr, addr, pm_base);
>>       }
>> @@ -586,11 +590,15 @@ static TCGv get_address_indexed(DisasContext 
>> *ctx, int rs1, TCGv offs)
>>       TCGv src1 = get_gpr(ctx, rs1, EXT_NONE);
>>         tcg_gen_add_tl(addr, src1, offs);
>> +
>> +    if (get_xl(ctx) == MXL_RV32) {
>> +        tcg_gen_ext32u_tl(addr, addr);
>> +    }
>> +
>>       if (ctx->pm_mask_enabled) {
>>           tcg_gen_andc_tl(addr, addr, pm_mask);
>> -    } else if (get_xl(ctx) == MXL_RV32) {
>> -        tcg_gen_ext32u_tl(addr, addr);
>>       }
>> +
>>       if (ctx->pm_base_enabled) {
>>           tcg_gen_or_tl(addr, addr, pm_base);
>>       }

Re: [PATCH 1/5] target/riscv: Fix effective address for pointer mask

[Richard Henderson]

On 3/27/23 19:48, liweiwei wrote:
> 
> On 2023/3/28 10:20, LIU Zhiwei wrote:
>>
>> On 2023/3/27 18:00, Weiwei Li wrote:
>>> Since pointer mask works on effective address, and the xl works on the
>>> generation of effective address, so xl related calculation should be done
>>> before pointer mask.
>>
>> Incorrect. It has been done.
>>
>> When updating the pm_mask,  we have already considered the env->xl.
>>
>> You can see it in riscv_cpu_update_mask
>>
>>     if (env->xl == MXL_RV32) {
>>         env->cur_pmmask = mask & UINT32_MAX;
>>         env->cur_pmbase = base & UINT32_MAX;
>>     } else {
>>         env->cur_pmmask = mask;
>>         env->cur_pmbase = base;
>>     }
>>
> Yeah, I missed this part. Then we should ensure cur_pmmask/base is updated when xl changes.

Is that even possible?  XL can change on priv level changes (SXL, UXL).


r~

Re: [PATCH 1/5] target/riscv: Fix effective address for pointer mask

[LIU Zhiwei]

On 2023/3/28 11:18, Richard Henderson wrote:
> On 3/27/23 19:48, liweiwei wrote:
>>
>> On 2023/3/28 10:20, LIU Zhiwei wrote:
>>>
>>> On 2023/3/27 18:00, Weiwei Li wrote:
>>>> Since pointer mask works on effective address, and the xl works on the
>>>> generation of effective address, so xl related calculation should 
>>>> be done
>>>> before pointer mask.
>>>
>>> Incorrect. It has been done.
>>>
>>> When updating the pm_mask,  we have already considered the env->xl.
>>>
>>> You can see it in riscv_cpu_update_mask
>>>
>>>     if (env->xl == MXL_RV32) {
>>>         env->cur_pmmask = mask & UINT32_MAX;
>>>         env->cur_pmbase = base & UINT32_MAX;
>>>     } else {
>>>         env->cur_pmmask = mask;
>>>         env->cur_pmbase = base;
>>>     }
>>>
>> Yeah, I missed this part. Then we should ensure cur_pmmask/base is 
>> updated when xl changes.
>
> Is that even possible?  XL can change on priv level changes (SXL, UXL).

I think I have considered this.

https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg04366.html

Zhiwei

>
>
> r~

Re: [PATCH 1/5] target/riscv: Fix effective address for pointer mask

[liweiwei]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]


On 2023/3/28 11:18, Richard Henderson wrote:
> On 3/27/23 19:48, liweiwei wrote:
>>
>> On 2023/3/28 10:20, LIU Zhiwei wrote:
>>>
>>> On 2023/3/27 18:00, Weiwei Li wrote:
>>>> Since pointer mask works on effective address, and the xl works on the
>>>> generation of effective address, so xl related calculation should 
>>>> be done
>>>> before pointer mask.
>>>
>>> Incorrect. It has been done.
>>>
>>> When updating the pm_mask,  we have already considered the env->xl.
>>>
>>> You can see it in riscv_cpu_update_mask
>>>
>>>     if (env->xl == MXL_RV32) {
>>>         env->cur_pmmask = mask & UINT32_MAX;
>>>         env->cur_pmbase = base & UINT32_MAX;
>>>     } else {
>>>         env->cur_pmmask = mask;
>>>         env->cur_pmbase = base;
>>>     }
>>>
>> Yeah, I missed this part. Then we should ensure cur_pmmask/base is 
>> updated when xl changes.
>
> Is that even possible?  XL can change on priv level changes (SXL, UXL).

Yeah. Not possible, since only UXL is changable currently, and SXL/UXL 
can only be changed in higher priv level.

So the recompute for xl in write_mstatus() seems redundant.

Maybe there is a way to change current xl in future if misa.mxl is 
changable.

Regards,

Weiwei Li

>
>
> r~

[-- Attachment #2: Type: text/html, Size: 2613 bytes --]

Re: [PATCH 1/5] target/riscv: Fix effective address for pointer mask

[LIU Zhiwei]

[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]


On 2023/3/28 11:33, liweiwei wrote:
>
>
> On 2023/3/28 11:18, Richard Henderson wrote:
>> On 3/27/23 19:48, liweiwei wrote:
>>>
>>> On 2023/3/28 10:20, LIU Zhiwei wrote:
>>>>
>>>> On 2023/3/27 18:00, Weiwei Li wrote:
>>>>> Since pointer mask works on effective address, and the xl works on 
>>>>> the
>>>>> generation of effective address, so xl related calculation should 
>>>>> be done
>>>>> before pointer mask.
>>>>
>>>> Incorrect. It has been done.
>>>>
>>>> When updating the pm_mask,  we have already considered the env->xl.
>>>>
>>>> You can see it in riscv_cpu_update_mask
>>>>
>>>>     if (env->xl == MXL_RV32) {
>>>>         env->cur_pmmask = mask & UINT32_MAX;
>>>>         env->cur_pmbase = base & UINT32_MAX;
>>>>     } else {
>>>>         env->cur_pmmask = mask;
>>>>         env->cur_pmbase = base;
>>>>     }
>>>>
>>> Yeah, I missed this part. Then we should ensure cur_pmmask/base is 
>>> updated when xl changes.
>>
>> Is that even possible?  XL can change on priv level changes (SXL, UXL).
>
> Yeah. Not possible, since only UXL is changable currently, and SXL/UXL 
> can only be changed in higher priv level.
>
> So the recompute for xl in write_mstatus() seems redundant.
>
I think you are almost right. But as we allow write XL field when in 
debug mode, we seemly also need call this function for it.

Zhiwei

> Maybe there is a way to change current xl in future if misa.mxl is 
> changable.
>
> Regards,
>
> Weiwei Li
>
>>
>>
>> r~

[-- Attachment #2: Type: text/html, Size: 3098 bytes --]

Re: [PATCH 1/5] target/riscv: Fix effective address for pointer mask

[liweiwei]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]


On 2023/3/28 15:25, LIU Zhiwei wrote:
>
>
> On 2023/3/28 11:33, liweiwei wrote:
>>
>>
>> On 2023/3/28 11:18, Richard Henderson wrote:
>>> On 3/27/23 19:48, liweiwei wrote:
>>>>
>>>> On 2023/3/28 10:20, LIU Zhiwei wrote:
>>>>>
>>>>> On 2023/3/27 18:00, Weiwei Li wrote:
>>>>>> Since pointer mask works on effective address, and the xl works 
>>>>>> on the
>>>>>> generation of effective address, so xl related calculation should 
>>>>>> be done
>>>>>> before pointer mask.
>>>>>
>>>>> Incorrect. It has been done.
>>>>>
>>>>> When updating the pm_mask,  we have already considered the env->xl.
>>>>>
>>>>> You can see it in riscv_cpu_update_mask
>>>>>
>>>>>     if (env->xl == MXL_RV32) {
>>>>>         env->cur_pmmask = mask & UINT32_MAX;
>>>>>         env->cur_pmbase = base & UINT32_MAX;
>>>>>     } else {
>>>>>         env->cur_pmmask = mask;
>>>>>         env->cur_pmbase = base;
>>>>>     }
>>>>>
>>>> Yeah, I missed this part. Then we should ensure cur_pmmask/base is 
>>>> updated when xl changes.
>>>
>>> Is that even possible?  XL can change on priv level changes (SXL, UXL).
>>
>> Yeah. Not possible, since only UXL is changable currently, and 
>> SXL/UXL can only be changed in higher priv level.
>>
>> So the recompute for xl in write_mstatus() seems redundant.
>>
> I think you are almost right. But as we allow write XL field when in 
> debug mode, we seemly also need call this function for it.
>
Then,  cur_pmbase/mask also need update in this case.

Weiwei Li

> Zhiwei
>
>> Maybe there is a way to change current xl in future if misa.mxl is 
>> changable.
>>
>> Regards,
>>
>> Weiwei Li
>>
>>>
>>>
>>> r~

[-- Attachment #2: Type: text/html, Size: 3773 bytes --]

[PATCH 2/5] target/riscv: Use sign-extended data address when xl = 32

[Weiwei Li]

Currently, the pc use signed-extend(in gen_set_pc*) when xl = 32. And
data address should use the same memory address space with it when
xl = 32. So we should change their address calculation to use sign-extended
address when xl = 32.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index bf0e2d318e..c48cb19389 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -570,7 +570,7 @@ static TCGv get_address(DisasContext *ctx, int rs1, int imm)
     tcg_gen_addi_tl(addr, src1, imm);
 
     if (get_xl(ctx) == MXL_RV32) {
-        tcg_gen_ext32u_tl(addr, addr);
+        tcg_gen_ext32s_tl(addr, addr);
     }
 
     if (ctx->pm_mask_enabled) {
@@ -592,7 +592,7 @@ static TCGv get_address_indexed(DisasContext *ctx, int rs1, TCGv offs)
     tcg_gen_add_tl(addr, src1, offs);
 
     if (get_xl(ctx) == MXL_RV32) {
-        tcg_gen_ext32u_tl(addr, addr);
+        tcg_gen_ext32s_tl(addr, addr);
     }
 
     if (ctx->pm_mask_enabled) {
-- 
2.25.1

Re: [PATCH 2/5] target/riscv: Use sign-extended data address when xl = 32

[Daniel Henrique Barboza]

On 3/27/23 07:00, Weiwei Li wrote:
> Currently, the pc use signed-extend(in gen_set_pc*) when xl = 32. And
> data address should use the same memory address space with it when
> xl = 32. So we should change their address calculation to use sign-extended
> address when xl = 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/translate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index bf0e2d318e..c48cb19389 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -570,7 +570,7 @@ static TCGv get_address(DisasContext *ctx, int rs1, int imm)
>       tcg_gen_addi_tl(addr, src1, imm);
>   
>       if (get_xl(ctx) == MXL_RV32) {
> -        tcg_gen_ext32u_tl(addr, addr);
> +        tcg_gen_ext32s_tl(addr, addr);
>       }
>   
>       if (ctx->pm_mask_enabled) {
> @@ -592,7 +592,7 @@ static TCGv get_address_indexed(DisasContext *ctx, int rs1, TCGv offs)
>       tcg_gen_add_tl(addr, src1, offs);
>   
>       if (get_xl(ctx) == MXL_RV32) {
> -        tcg_gen_ext32u_tl(addr, addr);
> +        tcg_gen_ext32s_tl(addr, addr);
>       }
>   
>       if (ctx->pm_mask_enabled) {

Re: [PATCH 2/5] target/riscv: Use sign-extended data address when xl = 32

[LIU Zhiwei]

[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]


On 2023/3/27 18:00, Weiwei Li wrote:
> Currently, the pc use signed-extend(in gen_set_pc*) when xl = 32. And
> data address should use the same memory address space with it when
> xl = 32. So we should change their address calculation to use sign-extended
> address when xl = 32.

Incorrect. PC sign-extend is mandated by the spec. It can be seen for 
gdb or the OS. But for the memory address for xl = 32, it's the qemu 
internal implementation.

We should not to make it too complex.

Even for the PC, when fectch instruction, we only use the low 32-bits, 
as you can see  from the cpu_get_tb_cpu_state.

*pc = cpu_get_xl(env) == MXL_RV32 ? env->pc & UINT32_MAX : env->pc;

Zhiwei

>
> Signed-off-by: Weiwei Li<liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang<wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/translate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> index bf0e2d318e..c48cb19389 100644
> --- a/target/riscv/translate.c
> +++ b/target/riscv/translate.c
> @@ -570,7 +570,7 @@ static TCGv get_address(DisasContext *ctx, int rs1, int imm)
>       tcg_gen_addi_tl(addr, src1, imm);
>   
>       if (get_xl(ctx) == MXL_RV32) {
> -        tcg_gen_ext32u_tl(addr, addr);
> +        tcg_gen_ext32s_tl(addr, addr);
>       }
>   
>       if (ctx->pm_mask_enabled) {
> @@ -592,7 +592,7 @@ static TCGv get_address_indexed(DisasContext *ctx, int rs1, TCGv offs)
>       tcg_gen_add_tl(addr, src1, offs);
>   
>       if (get_xl(ctx) == MXL_RV32) {
> -        tcg_gen_ext32u_tl(addr, addr);
> +        tcg_gen_ext32s_tl(addr, addr);
>       }
>   
>       if (ctx->pm_mask_enabled) {

[-- Attachment #2: Type: text/html, Size: 2364 bytes --]

Re: [PATCH 2/5] target/riscv: Use sign-extended data address when xl = 32

[liweiwei]

[-- Attachment #1: Type: text/plain, Size: 2048 bytes --]


On 2023/3/28 10:14, LIU Zhiwei wrote:
>
>
> On 2023/3/27 18:00, Weiwei Li wrote:
>> Currently, the pc use signed-extend(in gen_set_pc*) when xl = 32. And
>> data address should use the same memory address space with it when
>> xl = 32. So we should change their address calculation to use sign-extended
>> address when xl = 32.
>
> Incorrect. PC sign-extend is mandated by the spec. It can be seen for 
> gdb or the OS. But for the memory address for xl = 32, it's the qemu 
> internal implementation.
>
Yeah, there is no spec description for the memory address for xlen = 32. 
But it seems  easier to use the original (sign-extended) pc in this case.

We needn't cut the pc in cpu_get_tb_cpu_state and sign-extend it in 
riscv_cpu_synchronize_from_tb.

Regards,

Weiwei Li

> We should not to make it too complex.
>
> Even for the PC, when fectch instruction, we only use the low 32-bits, 
> as you can see  from the cpu_get_tb_cpu_state.
>
> *pc = cpu_get_xl(env) == MXL_RV32 ? env->pc & UINT32_MAX : env->pc;
>
> Zhiwei
>
>> Signed-off-by: Weiwei Li<liweiwei@iscas.ac.cn>
>> Signed-off-by: Junqiang Wang<wangjunqiang@iscas.ac.cn>
>> ---
>>   target/riscv/translate.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/riscv/translate.c b/target/riscv/translate.c
>> index bf0e2d318e..c48cb19389 100644
>> --- a/target/riscv/translate.c
>> +++ b/target/riscv/translate.c
>> @@ -570,7 +570,7 @@ static TCGv get_address(DisasContext *ctx, int rs1, int imm)
>>       tcg_gen_addi_tl(addr, src1, imm);
>>   
>>       if (get_xl(ctx) == MXL_RV32) {
>> -        tcg_gen_ext32u_tl(addr, addr);
>> +        tcg_gen_ext32s_tl(addr, addr);
>>       }
>>   
>>       if (ctx->pm_mask_enabled) {
>> @@ -592,7 +592,7 @@ static TCGv get_address_indexed(DisasContext *ctx, int rs1, TCGv offs)
>>       tcg_gen_add_tl(addr, src1, offs);
>>   
>>       if (get_xl(ctx) == MXL_RV32) {
>> -        tcg_gen_ext32u_tl(addr, addr);
>> +        tcg_gen_ext32s_tl(addr, addr);
>>       }
>>   
>>       if (ctx->pm_mask_enabled) {

[-- Attachment #2: Type: text/html, Size: 3319 bytes --]

[PATCH 3/5] target/riscv: Fix pointer mask transformation for vector address

[Weiwei Li]

actual_address = (requested_address & ~mpmmask) | mpmbase.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/vector_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index 2423affe37..a58d82af8c 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -172,7 +172,7 @@ static inline uint32_t vext_get_total_elems(CPURISCVState *env, uint32_t desc,
 
 static inline target_ulong adjust_addr(CPURISCVState *env, target_ulong addr)
 {
-    return (addr & env->cur_pmmask) | env->cur_pmbase;
+    return (addr & ~env->cur_pmmask) | env->cur_pmbase;
 }
 
 /*
-- 
2.25.1

Re: [PATCH 3/5] target/riscv: Fix pointer mask transformation for vector address

[Daniel Henrique Barboza]

On 3/27/23 07:00, Weiwei Li wrote:
> actual_address = (requested_address & ~mpmmask) | mpmbase.
> 
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/vector_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
> index 2423affe37..a58d82af8c 100644
> --- a/target/riscv/vector_helper.c
> +++ b/target/riscv/vector_helper.c
> @@ -172,7 +172,7 @@ static inline uint32_t vext_get_total_elems(CPURISCVState *env, uint32_t desc,
>   
>   static inline target_ulong adjust_addr(CPURISCVState *env, target_ulong addr)
>   {
> -    return (addr & env->cur_pmmask) | env->cur_pmbase;
> +    return (addr & ~env->cur_pmmask) | env->cur_pmbase;

I'm impressed that this flew under the radar for so long.


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


>   }
>   
>   /*

Re: [PATCH 3/5] target/riscv: Fix pointer mask transformation for vector address

[LIU Zhiwei]

On 2023/3/27 18:00, Weiwei Li wrote:
> actual_address = (requested_address & ~mpmmask) | mpmbase.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/vector_helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
> index 2423affe37..a58d82af8c 100644
> --- a/target/riscv/vector_helper.c
> +++ b/target/riscv/vector_helper.c
> @@ -172,7 +172,7 @@ static inline uint32_t vext_get_total_elems(CPURISCVState *env, uint32_t desc,
>   
>   static inline target_ulong adjust_addr(CPURISCVState *env, target_ulong addr)
>   {
> -    return (addr & env->cur_pmmask) | env->cur_pmbase;
> +    return (addr & ~env->cur_pmmask) | env->cur_pmbase;

It's my typo. Thanks.

Reviewed-by: LIU Zhiwei <zhiwei_liu@linux.alibaba.com>

Zhiwei

>   }
>   
>   /*

[PATCH 4/5] target/riscv: take xl into consideration for vector address

[Weiwei Li]

Sign-extend the vector address when xl = 32.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/vector_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
index a58d82af8c..07477663eb 100644
--- a/target/riscv/vector_helper.c
+++ b/target/riscv/vector_helper.c
@@ -172,6 +172,9 @@ static inline uint32_t vext_get_total_elems(CPURISCVState *env, uint32_t desc,
 
 static inline target_ulong adjust_addr(CPURISCVState *env, target_ulong addr)
 {
+    if (env->xl == MXL_RV32) {
+        addr = (int32_t)addr;
+    }
     return (addr & ~env->cur_pmmask) | env->cur_pmbase;
 }
 
-- 
2.25.1

Re: [PATCH 4/5] target/riscv: take xl into consideration for vector address

[Daniel Henrique Barboza]

On 3/27/23 07:00, Weiwei Li wrote:
> Sign-extend the vector address when xl = 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/vector_helper.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
> index a58d82af8c..07477663eb 100644
> --- a/target/riscv/vector_helper.c
> +++ b/target/riscv/vector_helper.c
> @@ -172,6 +172,9 @@ static inline uint32_t vext_get_total_elems(CPURISCVState *env, uint32_t desc,
>   
>   static inline target_ulong adjust_addr(CPURISCVState *env, target_ulong addr)
>   {
> +    if (env->xl == MXL_RV32) {
> +        addr = (int32_t)addr;
> +    }
>       return (addr & ~env->cur_pmmask) | env->cur_pmbase;
>   }
>   

Re: [PATCH 4/5] target/riscv: take xl into consideration for vector address

[LIU Zhiwei]

On 2023/3/27 18:00, Weiwei Li wrote:
> Sign-extend the vector address when xl = 32.
>
> Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> ---
>   target/riscv/vector_helper.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/target/riscv/vector_helper.c b/target/riscv/vector_helper.c
> index a58d82af8c..07477663eb 100644
> --- a/target/riscv/vector_helper.c
> +++ b/target/riscv/vector_helper.c
> @@ -172,6 +172,9 @@ static inline uint32_t vext_get_total_elems(CPURISCVState *env, uint32_t desc,
>   
>   static inline target_ulong adjust_addr(CPURISCVState *env, target_ulong addr)
>   {
> +    if (env->xl == MXL_RV32) {
> +        addr = (int32_t)addr;
> +    }

Incorrect. Same reason as patch 1.

Zhiwei

>       return (addr & ~env->cur_pmmask) | env->cur_pmbase;
>   }
>   

[PATCH 5/5] target/riscv: Add pointer mask support for instruction fetch

[Weiwei Li]

Transform the fetch address before page walk when pointer mask is
enabled for instruction fetch.

Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
---
 target/riscv/cpu.h        |  1 +
 target/riscv/cpu_helper.c | 25 +++++++++++++++++++++++--
 target/riscv/csr.c        |  2 --
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 638e47c75a..57bd9c3279 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -368,6 +368,7 @@ struct CPUArchState {
 #endif
     target_ulong cur_pmmask;
     target_ulong cur_pmbase;
+    bool cur_pminsn;
 
     /* Fields from here on are preserved across CPU reset. */
     QEMUTimer *stimer; /* Internal timer for S-mode interrupt */
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index f88c503cf4..77132a3e0c 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -124,6 +124,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
 void riscv_cpu_update_mask(CPURISCVState *env)
 {
     target_ulong mask = -1, base = 0;
+    bool insn = false;
     /*
      * TODO: Current RVJ spec does not specify
      * how the extension interacts with XLEN.
@@ -135,18 +136,21 @@ void riscv_cpu_update_mask(CPURISCVState *env)
             if (env->mmte & M_PM_ENABLE) {
                 mask = env->mpmmask;
                 base = env->mpmbase;
+                insn = env->mmte & MMTE_M_PM_INSN;
             }
             break;
         case PRV_S:
             if (env->mmte & S_PM_ENABLE) {
                 mask = env->spmmask;
                 base = env->spmbase;
+                insn = env->mmte & MMTE_S_PM_INSN;
             }
             break;
         case PRV_U:
             if (env->mmte & U_PM_ENABLE) {
                 mask = env->upmmask;
                 base = env->upmbase;
+                insn = env->mmte & MMTE_U_PM_INSN;
             }
             break;
         default:
@@ -161,6 +165,7 @@ void riscv_cpu_update_mask(CPURISCVState *env)
         env->cur_pmmask = mask;
         env->cur_pmbase = base;
     }
+    env->cur_pminsn = insn;
 }
 
 #ifndef CONFIG_USER_ONLY
@@ -1225,6 +1230,17 @@ static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
     riscv_pmu_incr_ctr(cpu, pmu_event_type);
 }
 
+static target_ulong adjust_pc_address(CPURISCVState *env, target_ulong pc)
+{
+    target_ulong adjust_pc = pc;
+
+    if (env->cur_pminsn) {
+        adjust_pc = (adjust_pc & ~env->cur_pmmask) | env->cur_pmbase;
+    }
+
+    return adjust_pc;
+}
+
 bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
                         MMUAccessType access_type, int mmu_idx,
                         bool probe, uintptr_t retaddr)
@@ -1232,6 +1248,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     RISCVCPU *cpu = RISCV_CPU(cs);
     CPURISCVState *env = &cpu->env;
     vaddr im_address;
+    vaddr orig_address = address;
     hwaddr pa = 0;
     int prot, prot2, prot_pmp;
     bool pmp_violation = false;
@@ -1248,6 +1265,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
                   __func__, address, access_type, mmu_idx);
 
+    if (access_type == MMU_INST_FETCH) {
+        address = adjust_pc_address(env, address);
+    }
+
     /* MPRV does not affect the virtual-machine load/store
        instructions, HLV, HLVX, and HSV. */
     if (riscv_cpu_two_stage_lookup(mmu_idx)) {
@@ -1351,13 +1372,13 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
     }
 
     if (ret == TRANSLATE_SUCCESS) {
-        tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
+        tlb_set_page(cs, orig_address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
                      prot, mmu_idx, tlb_size);
         return true;
     } else if (probe) {
         return false;
     } else {
-        raise_mmu_exception(env, address, access_type, pmp_violation,
+        raise_mmu_exception(env, orig_address, access_type, pmp_violation,
                             first_stage_error,
                             riscv_cpu_virt_enabled(env) ||
                                 riscv_cpu_two_stage_lookup(mmu_idx),
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index d522efc0b6..4544c9d934 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -3511,8 +3511,6 @@ static RISCVException write_mmte(CPURISCVState *env, int csrno,
     /* for machine mode pm.current is hardwired to 1 */
     wpri_val |= MMTE_M_PM_CURRENT;
 
-    /* hardwiring pm.instruction bit to 0, since it's not supported yet */
-    wpri_val &= ~(MMTE_M_PM_INSN | MMTE_S_PM_INSN | MMTE_U_PM_INSN);
     env->mmte = wpri_val | PM_EXT_DIRTY;
     riscv_cpu_update_mask(env);
 
-- 
2.25.1

Re: [PATCH 5/5] target/riscv: Add pointer mask support for instruction fetch

[Daniel Henrique Barboza]

On 3/27/23 07:00, Weiwei Li wrote:
> Transform the fetch address before page walk when pointer mask is
> enabled for instruction fetch.
> 
> 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.h        |  1 +
>   target/riscv/cpu_helper.c | 25 +++++++++++++++++++++++--
>   target/riscv/csr.c        |  2 --
>   3 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 638e47c75a..57bd9c3279 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -368,6 +368,7 @@ struct CPUArchState {
>   #endif
>       target_ulong cur_pmmask;
>       target_ulong cur_pmbase;
> +    bool cur_pminsn;
>   
>       /* Fields from here on are preserved across CPU reset. */
>       QEMUTimer *stimer; /* Internal timer for S-mode interrupt */
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f88c503cf4..77132a3e0c 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -124,6 +124,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>   void riscv_cpu_update_mask(CPURISCVState *env)
>   {
>       target_ulong mask = -1, base = 0;
> +    bool insn = false;
>       /*
>        * TODO: Current RVJ spec does not specify
>        * how the extension interacts with XLEN.
> @@ -135,18 +136,21 @@ void riscv_cpu_update_mask(CPURISCVState *env)
>               if (env->mmte & M_PM_ENABLE) {
>                   mask = env->mpmmask;
>                   base = env->mpmbase;
> +                insn = env->mmte & MMTE_M_PM_INSN;
>               }
>               break;
>           case PRV_S:
>               if (env->mmte & S_PM_ENABLE) {
>                   mask = env->spmmask;
>                   base = env->spmbase;
> +                insn = env->mmte & MMTE_S_PM_INSN;
>               }
>               break;
>           case PRV_U:
>               if (env->mmte & U_PM_ENABLE) {
>                   mask = env->upmmask;
>                   base = env->upmbase;
> +                insn = env->mmte & MMTE_U_PM_INSN;
>               }
>               break;
>           default:
> @@ -161,6 +165,7 @@ void riscv_cpu_update_mask(CPURISCVState *env)
>           env->cur_pmmask = mask;
>           env->cur_pmbase = base;
>       }
> +    env->cur_pminsn = insn;
>   }
>   
>   #ifndef CONFIG_USER_ONLY
> @@ -1225,6 +1230,17 @@ static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
>       riscv_pmu_incr_ctr(cpu, pmu_event_type);
>   }
>   
> +static target_ulong adjust_pc_address(CPURISCVState *env, target_ulong pc)
> +{
> +    target_ulong adjust_pc = pc;
> +
> +    if (env->cur_pminsn) {
> +        adjust_pc = (adjust_pc & ~env->cur_pmmask) | env->cur_pmbase;
> +    }
> +
> +    return adjust_pc;
> +}
> +
>   bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                           MMUAccessType access_type, int mmu_idx,
>                           bool probe, uintptr_t retaddr)
> @@ -1232,6 +1248,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       RISCVCPU *cpu = RISCV_CPU(cs);
>       CPURISCVState *env = &cpu->env;
>       vaddr im_address;
> +    vaddr orig_address = address;
>       hwaddr pa = 0;
>       int prot, prot2, prot_pmp;
>       bool pmp_violation = false;
> @@ -1248,6 +1265,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>                     __func__, address, access_type, mmu_idx);
>   
> +    if (access_type == MMU_INST_FETCH) {
> +        address = adjust_pc_address(env, address);
> +    }
> +
>       /* MPRV does not affect the virtual-machine load/store
>          instructions, HLV, HLVX, and HSV. */
>       if (riscv_cpu_two_stage_lookup(mmu_idx)) {
> @@ -1351,13 +1372,13 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       }
>   
>       if (ret == TRANSLATE_SUCCESS) {
> -        tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
> +        tlb_set_page(cs, orig_address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
>                        prot, mmu_idx, tlb_size);
>           return true;
>       } else if (probe) {
>           return false;
>       } else {
> -        raise_mmu_exception(env, address, access_type, pmp_violation,
> +        raise_mmu_exception(env, orig_address, access_type, pmp_violation,
>                               first_stage_error,
>                               riscv_cpu_virt_enabled(env) ||
>                                   riscv_cpu_two_stage_lookup(mmu_idx),
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d522efc0b6..4544c9d934 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3511,8 +3511,6 @@ static RISCVException write_mmte(CPURISCVState *env, int csrno,
>       /* for machine mode pm.current is hardwired to 1 */
>       wpri_val |= MMTE_M_PM_CURRENT;
>   
> -    /* hardwiring pm.instruction bit to 0, since it's not supported yet */
> -    wpri_val &= ~(MMTE_M_PM_INSN | MMTE_S_PM_INSN | MMTE_U_PM_INSN);
>       env->mmte = wpri_val | PM_EXT_DIRTY;
>       riscv_cpu_update_mask(env);
>   

Re: [PATCH 5/5] target/riscv: Add pointer mask support for instruction fetch

[Daniel Henrique Barboza]

On 3/27/23 07:00, Weiwei Li wrote:
> Transform the fetch address before page walk when pointer mask is
> enabled for instruction fetch.
> 
> 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.h        |  1 +
>   target/riscv/cpu_helper.c | 25 +++++++++++++++++++++++--
>   target/riscv/csr.c        |  2 --
>   3 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 638e47c75a..57bd9c3279 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -368,6 +368,7 @@ struct CPUArchState {
>   #endif
>       target_ulong cur_pmmask;
>       target_ulong cur_pmbase;
> +    bool cur_pminsn;
>   
>       /* Fields from here on are preserved across CPU reset. */
>       QEMUTimer *stimer; /* Internal timer for S-mode interrupt */
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index f88c503cf4..77132a3e0c 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -124,6 +124,7 @@ void cpu_get_tb_cpu_state(CPURISCVState *env, target_ulong *pc,
>   void riscv_cpu_update_mask(CPURISCVState *env)
>   {
>       target_ulong mask = -1, base = 0;
> +    bool insn = false;
>       /*
>        * TODO: Current RVJ spec does not specify
>        * how the extension interacts with XLEN.
> @@ -135,18 +136,21 @@ void riscv_cpu_update_mask(CPURISCVState *env)
>               if (env->mmte & M_PM_ENABLE) {
>                   mask = env->mpmmask;
>                   base = env->mpmbase;
> +                insn = env->mmte & MMTE_M_PM_INSN;
>               }
>               break;
>           case PRV_S:
>               if (env->mmte & S_PM_ENABLE) {
>                   mask = env->spmmask;
>                   base = env->spmbase;
> +                insn = env->mmte & MMTE_S_PM_INSN;
>               }
>               break;
>           case PRV_U:
>               if (env->mmte & U_PM_ENABLE) {
>                   mask = env->upmmask;
>                   base = env->upmbase;
> +                insn = env->mmte & MMTE_U_PM_INSN;
>               }
>               break;
>           default:
> @@ -161,6 +165,7 @@ void riscv_cpu_update_mask(CPURISCVState *env)
>           env->cur_pmmask = mask;
>           env->cur_pmbase = base;
>       }
> +    env->cur_pminsn = insn;
>   }
>   
>   #ifndef CONFIG_USER_ONLY
> @@ -1225,6 +1230,17 @@ static void pmu_tlb_fill_incr_ctr(RISCVCPU *cpu, MMUAccessType access_type)
>       riscv_pmu_incr_ctr(cpu, pmu_event_type);
>   }
>   
> +static target_ulong adjust_pc_address(CPURISCVState *env, target_ulong pc)
> +{
> +    target_ulong adjust_pc = pc;
> +
> +    if (env->cur_pminsn) {
> +        adjust_pc = (adjust_pc & ~env->cur_pmmask) | env->cur_pmbase;
> +    }
> +
> +    return adjust_pc;
> +}
> +
>   bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                           MMUAccessType access_type, int mmu_idx,
>                           bool probe, uintptr_t retaddr)
> @@ -1232,6 +1248,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       RISCVCPU *cpu = RISCV_CPU(cs);
>       CPURISCVState *env = &cpu->env;
>       vaddr im_address;
> +    vaddr orig_address = address;
>       hwaddr pa = 0;
>       int prot, prot2, prot_pmp;
>       bool pmp_violation = false;
> @@ -1248,6 +1265,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>                     __func__, address, access_type, mmu_idx);
>   
> +    if (access_type == MMU_INST_FETCH) {
> +        address = adjust_pc_address(env, address);
> +    }
> +
>       /* MPRV does not affect the virtual-machine load/store
>          instructions, HLV, HLVX, and HSV. */
>       if (riscv_cpu_two_stage_lookup(mmu_idx)) {
> @@ -1351,13 +1372,13 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       }
>   
>       if (ret == TRANSLATE_SUCCESS) {
> -        tlb_set_page(cs, address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
> +        tlb_set_page(cs, orig_address & ~(tlb_size - 1), pa & ~(tlb_size - 1),
>                        prot, mmu_idx, tlb_size);
>           return true;
>       } else if (probe) {
>           return false;
>       } else {
> -        raise_mmu_exception(env, address, access_type, pmp_violation,
> +        raise_mmu_exception(env, orig_address, access_type, pmp_violation,
>                               first_stage_error,
>                               riscv_cpu_virt_enabled(env) ||
>                                   riscv_cpu_two_stage_lookup(mmu_idx),
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index d522efc0b6..4544c9d934 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -3511,8 +3511,6 @@ static RISCVException write_mmte(CPURISCVState *env, int csrno,
>       /* for machine mode pm.current is hardwired to 1 */
>       wpri_val |= MMTE_M_PM_CURRENT;
>   
> -    /* hardwiring pm.instruction bit to 0, since it's not supported yet */
> -    wpri_val &= ~(MMTE_M_PM_INSN | MMTE_S_PM_INSN | MMTE_U_PM_INSN);
>       env->mmte = wpri_val | PM_EXT_DIRTY;
>       riscv_cpu_update_mask(env);
>   

Re: [PATCH 5/5] target/riscv: Add pointer mask support for instruction fetch

[Richard Henderson]

On 3/27/23 03:00, Weiwei Li wrote:
> @@ -1248,6 +1265,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>                     __func__, address, access_type, mmu_idx);
>   
> +    if (access_type == MMU_INST_FETCH) {
> +        address = adjust_pc_address(env, address);
> +    }

Why do you want to do this so late, as opposed to earlier in cpu_get_tb_cpu_state?


r~

Re: [PATCH 5/5] target/riscv: Add pointer mask support for instruction fetch

[liweiwei]

On 2023/3/28 02:04, Richard Henderson wrote:
> On 3/27/23 03:00, Weiwei Li wrote:
>> @@ -1248,6 +1265,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr 
>> address, int size,
>>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx 
>> %d\n",
>>                     __func__, address, access_type, mmu_idx);
>>   +    if (access_type == MMU_INST_FETCH) {
>> +        address = adjust_pc_address(env, address);
>> +    }
>
> Why do you want to do this so late, as opposed to earlier in 
> cpu_get_tb_cpu_state?

In this way, the pc for tb may be different from the reg pc. Then the pc 
register will be wrong if sync from tb.

Regards,

Weiwei Li

>
>
> r~

Re: [PATCH 5/5] target/riscv: Add pointer mask support for instruction fetch

[LIU Zhiwei]

On 2023/3/28 9:55, liweiwei wrote:
>
> On 2023/3/28 02:04, Richard Henderson wrote:
>> On 3/27/23 03:00, Weiwei Li wrote:
>>> @@ -1248,6 +1265,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr 
>>> address, int size,
>>>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d 
>>> mmu_idx %d\n",
>>>                     __func__, address, access_type, mmu_idx);
>>>   +    if (access_type == MMU_INST_FETCH) {
>>> +        address = adjust_pc_address(env, address);
>>> +    }
>>
>> Why do you want to do this so late, as opposed to earlier in 
>> cpu_get_tb_cpu_state?
>
> In this way, the pc for tb may be different from the reg pc. 
I don't understand.
> Then the pc register will be wrong if sync from tb.

I think you should give an explain here why it is wrong.

Zhiwei

>
> Regards,
>
> Weiwei Li
>
>>
>>
>> r~

Re: [PATCH 5/5] target/riscv: Add pointer mask support for instruction fetch

[liweiwei]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]


On 2023/3/28 10:31, LIU Zhiwei wrote:
>
> On 2023/3/28 9:55, liweiwei wrote:
>>
>> On 2023/3/28 02:04, Richard Henderson wrote:
>>> On 3/27/23 03:00, Weiwei Li wrote:
>>>> @@ -1248,6 +1265,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr 
>>>> address, int size,
>>>>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d 
>>>> mmu_idx %d\n",
>>>>                     __func__, address, access_type, mmu_idx);
>>>>   +    if (access_type == MMU_INST_FETCH) {
>>>> +        address = adjust_pc_address(env, address);
>>>> +    }
>>>
>>> Why do you want to do this so late, as opposed to earlier in 
>>> cpu_get_tb_cpu_state?
>>
>> In this way, the pc for tb may be different from the reg pc. 
> I don't understand.
>> Then the pc register will be wrong if sync from tb.
>
> I think you should give an explain here why it is wrong.
>
> Zhiwei

Assume the pc is 0x1fff 0000, pmmask is 0xf000 0000, if we adjust pc in  
cpu_get_tb_cpu_state,

then the tb->pc will be 0x0fff 0000.

If we sync pc from tb by riscv_cpu_synchronize_from_tb()

Then the pc will be updated to 0x0fff 0000 in this case, which will 
different from the original value.

I ignore many internal steps in above case. Any critical condition I 
missed? or any misunderstood?

Regards,

Weiwei Li

>
>>
>> Regards,
>>
>> Weiwei Li
>>
>>>
>>>
>>> r~

[-- Attachment #2: Type: text/html, Size: 2784 bytes --]

Re: [PATCH 5/5] target/riscv: Add pointer mask support for instruction fetch

[Richard Henderson]

On 3/27/23 18:55, liweiwei wrote:
> 
> On 2023/3/28 02:04, Richard Henderson wrote:
>> On 3/27/23 03:00, Weiwei Li wrote:
>>> @@ -1248,6 +1265,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>>>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>>>                     __func__, address, access_type, mmu_idx);
>>>   +    if (access_type == MMU_INST_FETCH) {
>>> +        address = adjust_pc_address(env, address);
>>> +    }
>>
>> Why do you want to do this so late, as opposed to earlier in cpu_get_tb_cpu_state?
> 
> In this way, the pc for tb may be different from the reg pc. Then the pc register will be 
> wrong if sync from tb.

Hmm, true.

But you certainly cannot adjust the address in tlb_fill, as you'll be producing different 
result for read/write and exec.  You could plausibly use a separate mmu_idx, but that's 
not ideal either.

The best solution might be to implement pc-relative translation (CF_PCREL).  At which 
point cpu_pc always has the correct results and we make relative adjustments to that.


r~

Re: [PATCH 5/5] target/riscv: Add pointer mask support for instruction fetch

[liweiwei]

On 2023/3/28 11:31, Richard Henderson wrote:
> On 3/27/23 18:55, liweiwei wrote:
>>
>> On 2023/3/28 02:04, Richard Henderson wrote:
>>> On 3/27/23 03:00, Weiwei Li wrote:
>>>> @@ -1248,6 +1265,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr 
>>>> address, int size,
>>>>       qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d 
>>>> mmu_idx %d\n",
>>>>                     __func__, address, access_type, mmu_idx);
>>>>   +    if (access_type == MMU_INST_FETCH) {
>>>> +        address = adjust_pc_address(env, address);
>>>> +    }
>>>
>>> Why do you want to do this so late, as opposed to earlier in 
>>> cpu_get_tb_cpu_state?
>>
>> In this way, the pc for tb may be different from the reg pc. Then the 
>> pc register will be wrong if sync from tb.
>
> Hmm, true.
>
> But you certainly cannot adjust the address in tlb_fill, as you'll be 
> producing different result for read/write and exec.  You could 
> plausibly use a separate mmu_idx, but that's not ideal either.
>
> The best solution might be to implement pc-relative translation 
> (CF_PCREL).  At which point cpu_pc always has the correct results and 
> we make relative adjustments to that.

I'm not very familiar with how CF_PCREL works currently. I'll try this 
way later.

Regards,

Weiwei Li

>
>
> r~