| Message ID | 20220609010724.104033-1-bmeng.cn@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] target/riscv: Remove the redundant initialization of env->misa_mxl | expand |
On Thu, Jun 9, 2022 at 11:08 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > env->misa_mxl was already set in the RISC-V cpu init routine, and > validated at the beginning of riscv_cpu_realize(). There is no need > to do a redundant initialization later. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > target/riscv/cpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index a91253d4bd..61d1737741 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -752,7 +752,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > ext |= RVJ; > } > > - set_misa(env, env->misa_mxl, ext); > + env->misa_ext_mask = env->misa_ext = ext; You're right that we don't need to set `misa_mxl`, but isn't it cleaner calling the helper function here instead of manually assigning it? Alistair > } > > riscv_cpu_register_gdb_regs_for_features(cs); > -- > 2.34.1 > >
On Mon, Jun 13, 2022 at 8:33 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Thu, Jun 9, 2022 at 11:08 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > env->misa_mxl was already set in the RISC-V cpu init routine, and > > validated at the beginning of riscv_cpu_realize(). There is no need > > to do a redundant initialization later. > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > > > target/riscv/cpu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > index a91253d4bd..61d1737741 100644 > > --- a/target/riscv/cpu.c > > +++ b/target/riscv/cpu.c > > @@ -752,7 +752,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > > ext |= RVJ; > > } > > > > - set_misa(env, env->misa_mxl, ext); > > + env->misa_ext_mask = env->misa_ext = ext; > > You're right that we don't need to set `misa_mxl`, but isn't it > cleaner calling the helper function here instead of manually assigning > it? > There is no helper for assigning misa_ext only. Do you want a new helper for that? Regards, Bin
On Mon, Jun 13, 2022 at 10:30 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > On Mon, Jun 13, 2022 at 8:33 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > On Thu, Jun 9, 2022 at 11:08 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > env->misa_mxl was already set in the RISC-V cpu init routine, and > > > validated at the beginning of riscv_cpu_realize(). There is no need > > > to do a redundant initialization later. > > > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > --- > > > > > > target/riscv/cpu.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > > index a91253d4bd..61d1737741 100644 > > > --- a/target/riscv/cpu.c > > > +++ b/target/riscv/cpu.c > > > @@ -752,7 +752,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > > > ext |= RVJ; > > > } > > > > > > - set_misa(env, env->misa_mxl, ext); > > > + env->misa_ext_mask = env->misa_ext = ext; > > > > You're right that we don't need to set `misa_mxl`, but isn't it > > cleaner calling the helper function here instead of manually assigning > > it? > > > > There is no helper for assigning misa_ext only. Do you want a new > helper for that? No, I don't think we need a new helper. I mean, is there any harm in just calling `set_misa()` even if that means we are performing a redundant operation? Alistair > > Regards, > Bin
On Thu, Jun 16, 2022 at 10:34 AM Alistair Francis <alistair23@gmail.com> wrote: > > On Mon, Jun 13, 2022 at 10:30 PM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > On Mon, Jun 13, 2022 at 8:33 AM Alistair Francis <alistair23@gmail.com> wrote: > > > > > > On Thu, Jun 9, 2022 at 11:08 AM Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > > > env->misa_mxl was already set in the RISC-V cpu init routine, and > > > > validated at the beginning of riscv_cpu_realize(). There is no need > > > > to do a redundant initialization later. > > > > > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > > --- > > > > > > > > target/riscv/cpu.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > > > > index a91253d4bd..61d1737741 100644 > > > > --- a/target/riscv/cpu.c > > > > +++ b/target/riscv/cpu.c > > > > @@ -752,7 +752,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > > > > ext |= RVJ; > > > > } > > > > > > > > - set_misa(env, env->misa_mxl, ext); > > > > + env->misa_ext_mask = env->misa_ext = ext; > > > > > > You're right that we don't need to set `misa_mxl`, but isn't it > > > cleaner calling the helper function here instead of manually assigning > > > it? > > > > > > > There is no helper for assigning misa_ext only. Do you want a new > > helper for that? > > No, I don't think we need a new helper. I mean, is there any harm in > just calling `set_misa()` even if that means we are performing a > redundant operation? > No there is no harm to perform a redundant initialization. Feel free to drop this patch then. Regards, Bin
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index a91253d4bd..61d1737741 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -752,7 +752,7 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) ext |= RVJ; } - set_misa(env, env->misa_mxl, ext); + env->misa_ext_mask = env->misa_ext = ext; } riscv_cpu_register_gdb_regs_for_features(cs);
env->misa_mxl was already set in the RISC-V cpu init routine, and validated at the beginning of riscv_cpu_realize(). There is no need to do a redundant initialization later. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- target/riscv/cpu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)