[PATCH 1/3] target/riscv: Remove the redundant initialization of env->misa_mxl

[PATCH 1/3] target/riscv: Remove the redundant initialization of env->misa_mxl

[Bin Meng]

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;
     }
 
     riscv_cpu_register_gdb_regs_for_features(cs);
-- 
2.34.1

[PATCH 2/3] target/riscv: kvm: Set env->misa_ext_mask to the supported value

[Bin Meng]

env->misa_ext_mask might be set to the same value of env->misa_ext
in riscv_cpu_realize() based on given properties, but it may differ
from what KVM tells us.

Let's set the correct env->misa_ext_mask in kvm_arch_init_vcpu().

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 target/riscv/kvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 70b4cff06f..c592980299 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -410,7 +410,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
     if (ret) {
         return ret;
     }
-    env->misa_ext = isa;
+    env->misa_ext_mask = env->misa_ext = isa;
 
     return ret;
 }
-- 
2.34.1

[PATCH 3/3] target/riscv: Skip parsing extensions from properties for KVM

[Bin Meng]

When running with accel=kvm, the extensions are actually told by
KVM, so let's skip the parsing logic from properties for KVM.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 target/riscv/cpu.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 61d1737741..ff911017c3 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -581,8 +581,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
     }
     assert(env->misa_mxl_max == env->misa_mxl);
 
-    /* If only MISA_EXT is unset for misa, then set it from properties */
-    if (env->misa_ext == 0) {
+    /*
+     * If only MISA_EXT is unset for misa, then set it from properties.
+     * For KVM, misa is told by KVM so properties are ignored.
+     */
+    if (!kvm_enabled() && env->misa_ext == 0) {
         uint32_t ext = 0;
 
         /* Do some ISA extension error checking */
-- 
2.34.1

Re: [PATCH 1/3] target/riscv: Remove the redundant initialization of env->misa_mxl

[Alistair Francis]

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
>
>

Re: [PATCH 2/3] target/riscv: kvm: Set env->misa_ext_mask to the supported value

[Alistair Francis]

On Thu, Jun 9, 2022 at 11:09 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> env->misa_ext_mask might be set to the same value of env->misa_ext
> in riscv_cpu_realize() based on given properties, but it may differ
> from what KVM tells us.
>
> Let's set the correct env->misa_ext_mask in kvm_arch_init_vcpu().
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
>  target/riscv/kvm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
> index 70b4cff06f..c592980299 100644
> --- a/target/riscv/kvm.c
> +++ b/target/riscv/kvm.c
> @@ -410,7 +410,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      if (ret) {
>          return ret;
>      }
> -    env->misa_ext = isa;
> +    env->misa_ext_mask = env->misa_ext = isa;
>
>      return ret;
>  }
> --
> 2.34.1
>
>

Re: [PATCH 3/3] target/riscv: Skip parsing extensions from properties for KVM

[Alistair Francis]

On Thu, Jun 9, 2022 at 11:11 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> When running with accel=kvm, the extensions are actually told by
> KVM, so let's skip the parsing logic from properties for KVM.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
>  target/riscv/cpu.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 61d1737741..ff911017c3 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -581,8 +581,11 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
>      }
>      assert(env->misa_mxl_max == env->misa_mxl);
>
> -    /* If only MISA_EXT is unset for misa, then set it from properties */
> -    if (env->misa_ext == 0) {
> +    /*
> +     * If only MISA_EXT is unset for misa, then set it from properties.
> +     * For KVM, misa is told by KVM so properties are ignored.
> +     */
> +    if (!kvm_enabled() && env->misa_ext == 0) {
>          uint32_t ext = 0;
>
>          /* Do some ISA extension error checking */
> --
> 2.34.1
>
>

Re: [PATCH 1/3] target/riscv: Remove the redundant initialization of env->misa_mxl

[Bin Meng]

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

Re: [PATCH 1/3] target/riscv: Remove the redundant initialization of env->misa_mxl

[Alistair Francis]

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

Re: [PATCH 1/3] target/riscv: Remove the redundant initialization of env->misa_mxl

[Bin Meng]

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