riscv: KVM: Fix a few SBI issues

PATCH In review Andrew Jones 2 participants 12 messages Feb 17, 2025 – Feb 20, 2025

Subsystems (3)

KVM RISC-V KVM RISC-V

lore

Share & tools

lore

Andrew Jones

author

2025-02-17 08:45 UTC #0 lore raw

cover riscv: KVM: Fix a few SBI issues

Fix issues found with kvm-unit-tests[1], which is currently focused on SBI.

[1] https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi

Andrew Jones (5):
riscv: KVM: Fix hart suspend status check
riscv: KVM: Fix hart suspend_type use
riscv: KVM: Fix SBI IPI error generation
riscv: KVM: Fix SBI TIME error generation
riscv: KVM: Fix SBI sleep_type use

 arch/riscv/kvm/vcpu_sbi_hsm.c     | 11 ++++++-----
 arch/riscv/kvm/vcpu_sbi_replace.c | 15 ++++++++++++---
 arch/riscv/kvm/vcpu_sbi_system.c  |  3 ++-
 3 files changed, 20 insertions(+), 9 deletions(-)

Signature · 1 line

2.48.1

Andrew Jones

author

2025-02-17 08:45 UTC #1 lore raw

PATCH 1/5 riscv: KVM: Fix hart suspend status check

"Not stopped" means started or suspended so we need to check for a single state in order to have a chance to check for each state. Also, we need to use target_vcpu when checking for the suspend state.

Fixes 763c8bed8c05 ("RISC-V: KVM: Implement SBI HSM suspend call")

Signed-off-by Andrew Jones <[email protected]>

---
 arch/riscv/kvm/vcpu_sbi_hsm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/riscv/kvm/vcpu_sbi_hsm.c +4 -4

index dce667f4b6ab..13a35eb77e8e 100644
@@ -79,12 +79,12 @@ static int kvm_sbi_hsm_vcpu_get_status(struct kvm_vcpu *vcpu)
 	target_vcpu = kvm_get_vcpu_by_id(vcpu->kvm, target_vcpuid);
 	if (!target_vcpu)
 		return SBI_ERR_INVALID_PARAM;
-	if (!kvm_riscv_vcpu_stopped(target_vcpu))
-		return SBI_HSM_STATE_STARTED;
-	else if (vcpu->stat.generic.blocking)
+	if (kvm_riscv_vcpu_stopped(target_vcpu))
+		return SBI_HSM_STATE_STOPPED;
+	else if (target_vcpu->stat.generic.blocking)
 		return SBI_HSM_STATE_SUSPENDED;
 	else
-		return SBI_HSM_STATE_STOPPED;
+		return SBI_HSM_STATE_STARTED;
 }
 
 static int kvm_sbi_ext_hsm_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,

Signature · 1 line

2.48.1

Anup Patel

maintainer Reviewed-by on v1 1/5

2025-02-17 10:36 UTC #2 lore raw

Re: [PATCH 1/5] riscv: KVM: Fix hart suspend status check

Quoted 8 lines · Andrew Jones

"Not stopped" means started or suspended so we need to check for a single state in order to have a chance to check for each state. Also, we need to use target_vcpu when checking for the suspend state.
Fixes 763c8bed8c05 ("RISC-V: KVM: Implement SBI HSM suspend call")
Signed-off-by Andrew Jones <[email protected]>

LGTM.

Reviewed-by Anup Patel <[email protected]>

Regards,
Anup

Quoted 28 lines

---
 arch/riscv/kvm/vcpu_sbi_hsm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/riscv/kvm/vcpu_sbi_hsm.c +4 -5

index dce667f4b6ab..13a35eb77e8e 100644
@@ -79,12 +79,12 @@ static int kvm_sbi_hsm_vcpu_get_status(struct kvm_vcpu *vcpu)
        target_vcpu = kvm_get_vcpu_by_id(vcpu->kvm, target_vcpuid);
        if (!target_vcpu)
                return SBI_ERR_INVALID_PARAM;
-       if (!kvm_riscv_vcpu_stopped(target_vcpu))
-               return SBI_HSM_STATE_STARTED;
-       else if (vcpu->stat.generic.blocking)
+       if (kvm_riscv_vcpu_stopped(target_vcpu))
+               return SBI_HSM_STATE_STOPPED;
+       else if (target_vcpu->stat.generic.blocking)
                return SBI_HSM_STATE_SUSPENDED;
        else
-               return SBI_HSM_STATE_STOPPED;
+               return SBI_HSM_STATE_STARTED;
 }
 
 static int kvm_sbi_ext_hsm_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
--

2.48.1

Andrew Jones

author

2025-02-17 08:45 UTC #3 lore raw

PATCH 2/5 riscv: KVM: Fix hart suspend_type use

In reply to Andrew Jones

The spec says suspend_type is 32 bits wide and "In case the data is defined as 32bit wide, higher privilege software must ensure that it only uses 32 bit data." Mask off upper bits of suspend_type before using it.

Fixes 763c8bed8c05 ("RISC-V: KVM: Implement SBI HSM suspend call")

Signed-off-by Andrew Jones <[email protected]>

---
 arch/riscv/kvm/vcpu_sbi_hsm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/riscv/kvm/vcpu_sbi_hsm.c +2 -1

index 13a35eb77e8e..3070bb31745d 100644
@@ -9,6 +9,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/kvm_host.h>
+#include <linux/wordpart.h>
 #include <asm/sbi.h>
 #include <asm/kvm_vcpu_sbi.h>

@@ -109,7 +110,7 @@ static int kvm_sbi_ext_hsm_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		}
 		return 0;
 	case SBI_EXT_HSM_HART_SUSPEND:
-		switch (cp->a0) {
+		switch (lower_32_bits(cp->a0)) {
 		case SBI_HSM_SUSPEND_RET_DEFAULT:
 			kvm_riscv_vcpu_wfi(vcpu);
 			break;

Signature · 1 line

2.48.1

Anup Patel

maintainer Reviewed-by on v1 2/5

2025-02-17 10:37 UTC #4 lore raw

Re: [PATCH 2/5] riscv: KVM: Fix hart suspend_type use

Quoted 8 lines · Andrew Jones

The spec says suspend_type is 32 bits wide and "In case the data is defined as 32bit wide, higher privilege software must ensure that it only uses 32 bit data." Mask off upper bits of suspend_type before using it.
Fixes 763c8bed8c05 ("RISC-V: KVM: Implement SBI HSM suspend call")
Signed-off-by Andrew Jones <[email protected]>

LGTM.

Reviewed-by Anup Patel <[email protected]>

Regards,
Anup

Quoted 28 lines

---
 arch/riscv/kvm/vcpu_sbi_hsm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/riscv/kvm/vcpu_sbi_hsm.c +2 -2

index 13a35eb77e8e..3070bb31745d 100644
@@ -9,6 +9,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/kvm_host.h>
+#include <linux/wordpart.h>
 #include <asm/sbi.h>
 #include <asm/kvm_vcpu_sbi.h>

@@ -109,7 +110,7 @@ static int kvm_sbi_ext_hsm_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
                }
                return 0;
        case SBI_EXT_HSM_HART_SUSPEND:
-               switch (cp->a0) {
+               switch (lower_32_bits(cp->a0)) {
                case SBI_HSM_SUSPEND_RET_DEFAULT:
                        kvm_riscv_vcpu_wfi(vcpu);
                        break;
--

2.48.1

Andrew Jones

author

2025-02-17 08:45 UTC #5 lore raw

PATCH 3/5 riscv: KVM: Fix SBI IPI error generation

In reply to Andrew Jones

When an invalid function ID of an SBI extension is used we should return not-supported, not invalid-param. Also, when we see that at least one hartid constructed from the base and mask parameters is invalid, then we should return invalid-param. Finally, rather than relying on overflowing a left shift to result in zero and then using that zero in a condition which [correctly] skips sending an IPI (but loops unnecessarily), explicitly check for overflow and exit the loop immediately.

Fixes 5f862df5585c ("RISC-V: KVM: Add v0.1 replacement SBI extensions defined in v0.2")

Signed-off-by Andrew Jones <[email protected]>

---
 arch/riscv/kvm/vcpu_sbi_replace.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

arch/riscv/kvm/vcpu_sbi_replace.c +11 -2

index 9c2ab3dfa93a..74e3a38c6a29 100644
@@ -51,9 +51,10 @@ static int kvm_sbi_ext_ipi_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
 	unsigned long hmask = cp->a0;
 	unsigned long hbase = cp->a1;
+	unsigned long hart_bit = 0, sentmask = 0;
 
 	if (cp->a6 != SBI_EXT_IPI_SEND_IPI) {
-		retdata->err_val = SBI_ERR_INVALID_PARAM;
+		retdata->err_val = SBI_ERR_NOT_SUPPORTED;
 		return 0;
 	}

@@ -62,15 +63,23 @@ static int kvm_sbi_ext_ipi_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		if (hbase != -1UL) {
 			if (tmp->vcpu_id < hbase)
 				continue;
-			if (!(hmask & (1UL << (tmp->vcpu_id - hbase))))
+			hart_bit = tmp->vcpu_id - hbase;
+			if (hart_bit >= __riscv_xlen)
+				goto done;
+			if (!(hmask & (1UL << hart_bit)))
 				continue;
 		}
 		ret = kvm_riscv_vcpu_set_interrupt(tmp, IRQ_VS_SOFT);
 		if (ret < 0)
 			break;
+		sentmask |= 1UL << hart_bit;
 		kvm_riscv_vcpu_pmu_incr_fw(tmp, SBI_PMU_FW_IPI_RCVD);
 	}
 
+done:
+	if (hbase != -1UL && (hmask ^ sentmask))
+		retdata->err_val = SBI_ERR_INVALID_PARAM;
+
 	return ret;
 }

Signature · 1 line

2.48.1

Anup Patel

maintainer Reviewed-by on v1 3/5

2025-02-17 10:43 UTC #6 lore raw

Re: [PATCH 3/5] riscv: KVM: Fix SBI IPI error generation

Quoted 10 lines · Andrew Jones

When an invalid function ID of an SBI extension is used we should return not-supported, not invalid-param. Also, when we see that at least one hartid constructed from the base and mask parameters is invalid, then we should return invalid-param. Finally, rather than relying on overflowing a left shift to result in zero and then using that zero in a condition which [correctly] skips sending an IPI (but loops unnecessarily), explicitly check for overflow and exit the loop immediately.

Quoted 2 lines

Fixes 5f862df5585c ("RISC-V: KVM: Add v0.1 replacement SBI extensions defined in v0.2")
Signed-off-by Andrew Jones <[email protected]>

LGTM.

Reviewed-by Anup Patel <[email protected]>

Regards,
Anup

Quoted 48 lines

---
 arch/riscv/kvm/vcpu_sbi_replace.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

arch/riscv/kvm/vcpu_sbi_replace.c +11 -3

index 9c2ab3dfa93a..74e3a38c6a29 100644
@@ -51,9 +51,10 @@ static int kvm_sbi_ext_ipi_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
        struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
        unsigned long hmask = cp->a0;
        unsigned long hbase = cp->a1;
+       unsigned long hart_bit = 0, sentmask = 0;
 
        if (cp->a6 != SBI_EXT_IPI_SEND_IPI) {
-               retdata->err_val = SBI_ERR_INVALID_PARAM;
+               retdata->err_val = SBI_ERR_NOT_SUPPORTED;
                return 0;
        }

@@ -62,15 +63,23 @@ static int kvm_sbi_ext_ipi_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
                if (hbase != -1UL) {
                        if (tmp->vcpu_id < hbase)
                                continue;
-                       if (!(hmask & (1UL << (tmp->vcpu_id - hbase))))
+                       hart_bit = tmp->vcpu_id - hbase;
+                       if (hart_bit >= __riscv_xlen)
+                               goto done;
+                       if (!(hmask & (1UL << hart_bit)))
                                continue;
                }
                ret = kvm_riscv_vcpu_set_interrupt(tmp, IRQ_VS_SOFT);
                if (ret < 0)
                        break;
+               sentmask |= 1UL << hart_bit;
                kvm_riscv_vcpu_pmu_incr_fw(tmp, SBI_PMU_FW_IPI_RCVD);
        }
 
+done:
+       if (hbase != -1UL && (hmask ^ sentmask))
+               retdata->err_val = SBI_ERR_INVALID_PARAM;
+
        return ret;
 }
 
--

2.48.1

Andrew Jones

author

2025-02-17 08:45 UTC #7 lore raw

PATCH 4/5 riscv: KVM: Fix SBI TIME error generation

In reply to Andrew Jones

When an invalid function ID of an SBI extension is used we should return not-supported, not invalid-param.

Fixes 5f862df5585c ("RISC-V: KVM: Add v0.1 replacement SBI extensions defined in v0.2")

Signed-off-by Andrew Jones <[email protected]>

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

arch/riscv/kvm/vcpu_sbi_replace.c +1 -1

index 74e3a38c6a29..5fbf3f94f1e8 100644
@@ -21,7 +21,7 @@ static int kvm_sbi_ext_time_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
 	u64 next_cycle;
 
 	if (cp->a6 != SBI_EXT_TIME_SET_TIMER) {
-		retdata->err_val = SBI_ERR_INVALID_PARAM;
+		retdata->err_val = SBI_ERR_NOT_SUPPORTED;
 		return 0;
 	}

Signature · 1 line

2.48.1

Anup Patel

maintainer Reviewed-by on v1 4/5

2025-02-17 10:44 UTC #8 lore raw

Re: [PATCH 4/5] riscv: KVM: Fix SBI TIME error generation

Quoted 6 lines · Andrew Jones

When an invalid function ID of an SBI extension is used we should return not-supported, not invalid-param.
Fixes 5f862df5585c ("RISC-V: KVM: Add v0.1 replacement SBI extensions defined in v0.2")
Signed-off-by Andrew Jones <[email protected]>

LGTM.

Reviewed-by Anup Patel <[email protected]>

Regards,
Anup

Quoted 20 lines

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

arch/riscv/kvm/vcpu_sbi_replace.c +1 -2

index 74e3a38c6a29..5fbf3f94f1e8 100644
@@ -21,7 +21,7 @@ static int kvm_sbi_ext_time_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
        u64 next_cycle;
 
        if (cp->a6 != SBI_EXT_TIME_SET_TIMER) {
-               retdata->err_val = SBI_ERR_INVALID_PARAM;
+               retdata->err_val = SBI_ERR_NOT_SUPPORTED;
                return 0;
        }
 
--

2.48.1

Andrew Jones

author

2025-02-17 08:45 UTC #9 lore raw

PATCH 5/5 riscv: KVM: Fix SBI sleep_type use

In reply to Andrew Jones

The spec says sleep_type is 32 bits wide and "In case the data is defined as 32bit wide, higher privilege software must ensure that it only uses 32 bit data." Mask off upper bits of sleep_type before using it.

Fixes 023c15151fbb ("RISC-V: KVM: Add SBI system suspend support")

Signed-off-by Andrew Jones <[email protected]>

---
 arch/riscv/kvm/vcpu_sbi_system.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/riscv/kvm/vcpu_sbi_system.c +2 -1

index 5d55e08791fa..bc0ebba89003 100644
@@ -4,6 +4,7 @@
  */
 
 #include <linux/kvm_host.h>
+#include <linux/wordpart.h>
 
 #include <asm/kvm_vcpu_sbi.h>
 #include <asm/sbi.h>

@@ -19,7 +20,7 @@ static int kvm_sbi_ext_susp_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
 
 	switch (funcid) {
 	case SBI_EXT_SUSP_SYSTEM_SUSPEND:
-		if (cp->a0 != SBI_SUSP_SLEEP_TYPE_SUSPEND_TO_RAM) {
+		if (lower_32_bits(cp->a0) != SBI_SUSP_SLEEP_TYPE_SUSPEND_TO_RAM) {
 			retdata->err_val = SBI_ERR_INVALID_PARAM;
 			return 0;
 		}

Signature · 1 line

2.48.1

Anup Patel

maintainer Reviewed-by on v1 5/5

2025-02-17 10:57 UTC #10 lore raw

Re: [PATCH 5/5] riscv: KVM: Fix SBI sleep_type use

Quoted 8 lines · Andrew Jones

The spec says sleep_type is 32 bits wide and "In case the data is defined as 32bit wide, higher privilege software must ensure that it only uses 32 bit data." Mask off upper bits of sleep_type before using it.
Fixes 023c15151fbb ("RISC-V: KVM: Add SBI system suspend support")
Signed-off-by Andrew Jones <[email protected]>

LGTM.

Reviewed-by Anup Patel <[email protected]>

Regards,
Anup

Quoted 28 lines

---
 arch/riscv/kvm/vcpu_sbi_system.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/riscv/kvm/vcpu_sbi_system.c +2 -2

index 5d55e08791fa..bc0ebba89003 100644
@@ -4,6 +4,7 @@
  */
 
 #include <linux/kvm_host.h>
+#include <linux/wordpart.h>
 
 #include <asm/kvm_vcpu_sbi.h>
 #include <asm/sbi.h>

@@ -19,7 +20,7 @@ static int kvm_sbi_ext_susp_handler(struct kvm_vcpu *vcpu, struct kvm_run *run,
 
        switch (funcid) {
        case SBI_EXT_SUSP_SYSTEM_SUSPEND:
-               if (cp->a0 != SBI_SUSP_SLEEP_TYPE_SUSPEND_TO_RAM) {
+               if (lower_32_bits(cp->a0) != SBI_SUSP_SLEEP_TYPE_SUSPEND_TO_RAM) {
                        retdata->err_val = SBI_ERR_INVALID_PARAM;
                        return 0;
                }
--

2.48.1

Anup Patel

maintainer

2025-02-20 12:18 UTC #11 lore raw

In reply to Andrew Jones

Quoted 4 lines · Andrew Jones

Fix issues found with kvm-unit-tests[1], which is currently focused on SBI.

Quoted 8 lines

[1] https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv/sbi
Andrew Jones (5):
riscv: KVM: Fix hart suspend status check
riscv: KVM: Fix hart suspend_type use
riscv: KVM: Fix SBI IPI error generation
riscv: KVM: Fix SBI TIME error generation
riscv: KVM: Fix SBI sleep_type use

Queued these fixes for Linux-6.14-rcX.

Regards,
Anup

Quoted 9 lines

 arch/riscv/kvm/vcpu_sbi_hsm.c     | 11 ++++++-----
 arch/riscv/kvm/vcpu_sbi_replace.c | 15 ++++++++++++---
 arch/riscv/kvm/vcpu_sbi_system.c  |  3 ++-
 3 files changed, 20 insertions(+), 9 deletions(-)

--
2.48.1