[PATCH] KVM: RISC-V: reset smstateen CSRs

[PATCH] KVM: RISC-V: reset smstateen CSRs

[Radim Krčmář]

Hi, I'm sending this early to ventana-sw as we hit the issue in today's
slack discussion.  I only compile-tested it so far and it will take me a
while to trigger a bug and verify the solution.

---8<--
The smstateen CSRs control which stateful features are enabled in
VU-mode.  SU-mode must properly context switch the state of all enabled
features.

Reset the smstateen CSRs, because SU-mode might not know that it must
context switch the state.  Reset unconditionally as it is shorter and
safer, and not that much slower.

Fixes: 81f0f314fec9 ("RISCV: KVM: Add sstateen0 context save/restore")
Cc: stable@vger.kernel.org
Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_host.h | 3 +++
 arch/riscv/kvm/vcpu.c             | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index cc33e35cd628..1e9fe3cbecd3 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -234,6 +234,9 @@ struct kvm_vcpu_arch {
 	/* CPU CSR context upon Guest VCPU reset */
 	struct kvm_vcpu_csr guest_reset_csr;
 
+	/* CPU smstateen CSR context upon Guest VCPU reset */
+	struct kvm_vcpu_smstateen_csr reset_smstateen_csr;
+
 	/*
 	 * VCPU interrupts
 	 *
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index 60d684c76c58..b11b4027a859 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -57,6 +57,8 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
 	struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
 	struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
 	struct kvm_cpu_context *reset_cntx = &vcpu->arch.guest_reset_context;
+	struct kvm_vcpu_smstateen_csr *smstateen_csr = &vcpu->arch.smstateen_csr;
+	struct kvm_vcpu_smstateen_csr *reset_smstateen_csr = &vcpu->arch.reset_smstateen_csr;
 	bool loaded;
 
 	/**
@@ -73,6 +75,8 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
 
 	memcpy(csr, reset_csr, sizeof(*csr));
 
+	memcpy(smstateen_csr, reset_smstateen_csr, sizeof(*smstateen_csr));
+
 	spin_lock(&vcpu->arch.reset_cntx_lock);
 	memcpy(cntx, reset_cntx, sizeof(*cntx));
 	spin_unlock(&vcpu->arch.reset_cntx_lock);
-- 
2.48.1

Re: [PATCH] KVM: RISC-V: reset smstateen CSRs

[Anup Patel]

On Tue, Mar 25, 2025 at 12:01 AM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> Hi, I'm sending this early to ventana-sw as we hit the issue in today's
> slack discussion.  I only compile-tested it so far and it will take me a
> while to trigger a bug and verify the solution.
>
> ---8<--
> The smstateen CSRs control which stateful features are enabled in
> VU-mode.  SU-mode must properly context switch the state of all enabled
> features.
>
> Reset the smstateen CSRs, because SU-mode might not know that it must
> context switch the state.  Reset unconditionally as it is shorter and
> safer, and not that much slower.
>
> Fixes: 81f0f314fec9 ("RISCV: KVM: Add sstateen0 context save/restore")
> Cc: stable@vger.kernel.org
> Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>

How about moving "struct kvm_vcpu_smstateen_csr smstateen" from
"struct kvm_vcpu_arch" to "struct kvm_vcpu_csr". This way we will not
need an extra "struct kvm_vcpu_smstateen_csr reset_smstateen_csr"
in "struct kvm_vcpu_csr".

Regards,
Anup

> ---
>  arch/riscv/include/asm/kvm_host.h | 3 +++
>  arch/riscv/kvm/vcpu.c             | 4 ++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index cc33e35cd628..1e9fe3cbecd3 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -234,6 +234,9 @@ struct kvm_vcpu_arch {
>         /* CPU CSR context upon Guest VCPU reset */
>         struct kvm_vcpu_csr guest_reset_csr;
>
> +       /* CPU smstateen CSR context upon Guest VCPU reset */
> +       struct kvm_vcpu_smstateen_csr reset_smstateen_csr;
> +
>         /*
>          * VCPU interrupts
>          *
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index 60d684c76c58..b11b4027a859 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -57,6 +57,8 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
>         struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
>         struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
>         struct kvm_cpu_context *reset_cntx = &vcpu->arch.guest_reset_context;
> +       struct kvm_vcpu_smstateen_csr *smstateen_csr = &vcpu->arch.smstateen_csr;
> +       struct kvm_vcpu_smstateen_csr *reset_smstateen_csr = &vcpu->arch.reset_smstateen_csr;
>         bool loaded;
>
>         /**
> @@ -73,6 +75,8 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
>
>         memcpy(csr, reset_csr, sizeof(*csr));
>
> +       memcpy(smstateen_csr, reset_smstateen_csr, sizeof(*smstateen_csr));
> +
>         spin_lock(&vcpu->arch.reset_cntx_lock);
>         memcpy(cntx, reset_cntx, sizeof(*cntx));
>         spin_unlock(&vcpu->arch.reset_cntx_lock);
> --
> 2.48.1
>

Re: [PATCH] KVM: RISC-V: reset smstateen CSRs

[Anup Patel]

On Tue, Mar 25, 2025 at 8:58 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Tue, Mar 25, 2025 at 12:01 AM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >
> > Hi, I'm sending this early to ventana-sw as we hit the issue in today's
> > slack discussion.  I only compile-tested it so far and it will take me a
> > while to trigger a bug and verify the solution.
> >
> > ---8<--
> > The smstateen CSRs control which stateful features are enabled in
> > VU-mode.  SU-mode must properly context switch the state of all enabled
> > features.
> >
> > Reset the smstateen CSRs, because SU-mode might not know that it must
> > context switch the state.  Reset unconditionally as it is shorter and
> > safer, and not that much slower.
> >
> > Fixes: 81f0f314fec9 ("RISCV: KVM: Add sstateen0 context save/restore")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
>
> How about moving "struct kvm_vcpu_smstateen_csr smstateen" from
> "struct kvm_vcpu_arch" to "struct kvm_vcpu_csr". This way we will not
> need an extra "struct kvm_vcpu_smstateen_csr reset_smstateen_csr"
> in "struct kvm_vcpu_csr".

Other than my comment, this looks good for upstreaming.

Thanks,
Anup

>
> Regards,
> Anup
>
> > ---
> >  arch/riscv/include/asm/kvm_host.h | 3 +++
> >  arch/riscv/kvm/vcpu.c             | 4 ++++
> >  2 files changed, 7 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > index cc33e35cd628..1e9fe3cbecd3 100644
> > --- a/arch/riscv/include/asm/kvm_host.h
> > +++ b/arch/riscv/include/asm/kvm_host.h
> > @@ -234,6 +234,9 @@ struct kvm_vcpu_arch {
> >         /* CPU CSR context upon Guest VCPU reset */
> >         struct kvm_vcpu_csr guest_reset_csr;
> >
> > +       /* CPU smstateen CSR context upon Guest VCPU reset */
> > +       struct kvm_vcpu_smstateen_csr reset_smstateen_csr;
> > +
> >         /*
> >          * VCPU interrupts
> >          *
> > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > index 60d684c76c58..b11b4027a859 100644
> > --- a/arch/riscv/kvm/vcpu.c
> > +++ b/arch/riscv/kvm/vcpu.c
> > @@ -57,6 +57,8 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
> >         struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
> >         struct kvm_cpu_context *cntx = &vcpu->arch.guest_context;
> >         struct kvm_cpu_context *reset_cntx = &vcpu->arch.guest_reset_context;
> > +       struct kvm_vcpu_smstateen_csr *smstateen_csr = &vcpu->arch.smstateen_csr;
> > +       struct kvm_vcpu_smstateen_csr *reset_smstateen_csr = &vcpu->arch.reset_smstateen_csr;
> >         bool loaded;
> >
> >         /**
> > @@ -73,6 +75,8 @@ static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
> >
> >         memcpy(csr, reset_csr, sizeof(*csr));
> >
> > +       memcpy(smstateen_csr, reset_smstateen_csr, sizeof(*smstateen_csr));
> > +
> >         spin_lock(&vcpu->arch.reset_cntx_lock);
> >         memcpy(cntx, reset_cntx, sizeof(*cntx));
> >         spin_unlock(&vcpu->arch.reset_cntx_lock);
> > --
> > 2.48.1
> >

Re: [PATCH] KVM: RISC-V: reset smstateen CSRs

[Radim Krčmář]

2025-03-25T22:27:41+05:30, Anup Patel <apatel@ventanamicro.com>:
> On Tue, Mar 25, 2025 at 8:58 PM Anup Patel <apatel@ventanamicro.com> wrote:
>>
>> On Tue, Mar 25, 2025 at 12:01 AM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> >
>> > Hi, I'm sending this early to ventana-sw as we hit the issue in today's
>> > slack discussion.  I only compile-tested it so far and it will take me a
>> > while to trigger a bug and verify the solution.
>> >
>> > ---8<--
>> > The smstateen CSRs control which stateful features are enabled in
>> > VU-mode.  SU-mode must properly context switch the state of all enabled
>> > features.
>> >
>> > Reset the smstateen CSRs, because SU-mode might not know that it must
>> > context switch the state.  Reset unconditionally as it is shorter and
>> > safer, and not that much slower.
>> >
>> > Fixes: 81f0f314fec9 ("RISCV: KVM: Add sstateen0 context save/restore")
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
>>
>> How about moving "struct kvm_vcpu_smstateen_csr smstateen" from
>> "struct kvm_vcpu_arch" to "struct kvm_vcpu_csr". This way we will not
>> need an extra "struct kvm_vcpu_smstateen_csr reset_smstateen_csr"
>> in "struct kvm_vcpu_csr".

It is tricky, because kvm_riscv_vcpu_general_set_csr calculates the
amount of registers accessible to userspace based the on size of
kvm_vcpu_csr.

We'd have to make changes to logic before expanding kvm_vcpu_csr with
kvm_vcpu_smstateen_csr.  At that point, I think it be much easier to
just put all csrs to kvm_vcpu_csr directly, which would also simplify
future extensions.

> Other than my comment, this looks good for upstreaming.

I think the current version is more appropriate for stable, and we can
implement your suggestion afterwards.

Thanks.

Re: [PATCH] KVM: RISC-V: reset smstateen CSRs

[Anup Patel]

On Tue, Mar 25, 2025 at 10:38 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>
> 2025-03-25T22:27:41+05:30, Anup Patel <apatel@ventanamicro.com>:
> > On Tue, Mar 25, 2025 at 8:58 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >>
> >> On Tue, Mar 25, 2025 at 12:01 AM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
> >> >
> >> > Hi, I'm sending this early to ventana-sw as we hit the issue in today's
> >> > slack discussion.  I only compile-tested it so far and it will take me a
> >> > while to trigger a bug and verify the solution.
> >> >
> >> > ---8<--
> >> > The smstateen CSRs control which stateful features are enabled in
> >> > VU-mode.  SU-mode must properly context switch the state of all enabled
> >> > features.
> >> >
> >> > Reset the smstateen CSRs, because SU-mode might not know that it must
> >> > context switch the state.  Reset unconditionally as it is shorter and
> >> > safer, and not that much slower.
> >> >
> >> > Fixes: 81f0f314fec9 ("RISCV: KVM: Add sstateen0 context save/restore")
> >> > Cc: stable@vger.kernel.org
> >> > Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
> >>
> >> How about moving "struct kvm_vcpu_smstateen_csr smstateen" from
> >> "struct kvm_vcpu_arch" to "struct kvm_vcpu_csr". This way we will not
> >> need an extra "struct kvm_vcpu_smstateen_csr reset_smstateen_csr"
> >> in "struct kvm_vcpu_csr".
>
> It is tricky, because kvm_riscv_vcpu_general_set_csr calculates the
> amount of registers accessible to userspace based the on size of
> kvm_vcpu_csr.
>
> We'd have to make changes to logic before expanding kvm_vcpu_csr with
> kvm_vcpu_smstateen_csr.  At that point, I think it be much easier to
> just put all csrs to kvm_vcpu_csr directly, which would also simplify
> future extensions.
>
> > Other than my comment, this looks good for upstreaming.
>
> I think the current version is more appropriate for stable, and we can
> implement your suggestion afterwards.

Sounds good.

Regards,
Anup