| Message ID | 20230327100027.61160-2-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/riscv: Fix pointer mask related support | expand |
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); > }
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); > }
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); >> }
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~
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~
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~
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~
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~
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); }