All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] RISC-V: KVM: Ensure SBI extension is enabled
@ 2023-04-26 17:13 ` Andrew Jones
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-04-26 17:13 UTC (permalink / raw)
  To: kvm-riscv

Ensure guests can't attempt to invoke SBI extension functions when the
SBI extension's probe function has stated that the extension is not
available.

Andrew Jones (3):
  RISC-V: KVM: Disable SBI extension when its probe fails
  RISC-V: KVM: Rename dis_idx to ext_idx
  RISC-V: KVM: Introduce extension_enabled

 arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 ++++
 arch/riscv/kvm/vcpu_sbi.c             | 62 +++++++++++++++++----------
 2 files changed, 48 insertions(+), 23 deletions(-)

-- 
2.39.2



^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH 0/3] RISC-V: KVM: Ensure SBI extension is enabled
@ 2023-04-26 17:13 ` Andrew Jones
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-04-26 17:13 UTC (permalink / raw)
  To: kvm-riscv, linux-riscv
  Cc: 'Palmer Dabbelt ', 'Anup Patel ',
	'Atish Patra ', 'Albert Ou ',
	'Paul Walmsley '

Ensure guests can't attempt to invoke SBI extension functions when the
SBI extension's probe function has stated that the extension is not
available.

Andrew Jones (3):
  RISC-V: KVM: Disable SBI extension when its probe fails
  RISC-V: KVM: Rename dis_idx to ext_idx
  RISC-V: KVM: Introduce extension_enabled

 arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 ++++
 arch/riscv/kvm/vcpu_sbi.c             | 62 +++++++++++++++++----------
 2 files changed, 48 insertions(+), 23 deletions(-)

-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH 1/3] RISC-V: KVM: Disable SBI extension when its probe fails
  2023-04-26 17:13 ` Andrew Jones
@ 2023-04-26 17:13   ` Andrew Jones
  -1 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-04-26 17:13 UTC (permalink / raw)
  To: kvm-riscv

When an SBI extension specific probe function exists and fails, then
use the extension_disabled context to disable the extension. This
ensures the extension's functions cannot be invoked. Doing the
disabling in kvm_vcpu_sbi_find_ext() allows it to be done lazily
on its first use. Checking extension_disabled prior to probing
ensures the probe is only executed once for disabled extensions.
Later patches ensure we only execute probe once for enabled
extensions as well.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/kvm/vcpu_sbi.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index e52fde504433..aa3c126d2e3c 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -307,18 +307,25 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu,
 const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
 				struct kvm_vcpu *vcpu, unsigned long extid)
 {
-	int i;
-	const struct kvm_riscv_sbi_extension_entry *sext;
 	struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
+	const struct kvm_riscv_sbi_extension_entry *entry;
+	const struct kvm_vcpu_sbi_extension *ext;
+	int i;
 
 	for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
-		sext = &sbi_ext[i];
-		if (sext->ext_ptr->extid_start <= extid &&
-		    sext->ext_ptr->extid_end >= extid) {
-			if (sext->dis_idx < KVM_RISCV_SBI_EXT_MAX &&
-			    scontext->extension_disabled[sext->dis_idx])
+		entry = &sbi_ext[i];
+		ext = entry->ext_ptr;
+
+		if (ext->extid_start <= extid && ext->extid_end >= extid) {
+			if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
+				return ext;
+			if (scontext->extension_disabled[entry->dis_idx])
+				return NULL;
+			if (ext->probe && !ext->probe(vcpu)) {
+				scontext->extension_disabled[entry->dis_idx] = true;
 				return NULL;
-			return sbi_ext[i].ext_ptr;
+			}
+			return ext;
 		}
 	}
 
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [PATCH 1/3] RISC-V: KVM: Disable SBI extension when its probe fails
@ 2023-04-26 17:13   ` Andrew Jones
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-04-26 17:13 UTC (permalink / raw)
  To: kvm-riscv, linux-riscv
  Cc: 'Palmer Dabbelt ', 'Anup Patel ',
	'Atish Patra ', 'Albert Ou ',
	'Paul Walmsley '

When an SBI extension specific probe function exists and fails, then
use the extension_disabled context to disable the extension. This
ensures the extension's functions cannot be invoked. Doing the
disabling in kvm_vcpu_sbi_find_ext() allows it to be done lazily
on its first use. Checking extension_disabled prior to probing
ensures the probe is only executed once for disabled extensions.
Later patches ensure we only execute probe once for enabled
extensions as well.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/kvm/vcpu_sbi.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index e52fde504433..aa3c126d2e3c 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -307,18 +307,25 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu,
 const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
 				struct kvm_vcpu *vcpu, unsigned long extid)
 {
-	int i;
-	const struct kvm_riscv_sbi_extension_entry *sext;
 	struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
+	const struct kvm_riscv_sbi_extension_entry *entry;
+	const struct kvm_vcpu_sbi_extension *ext;
+	int i;
 
 	for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
-		sext = &sbi_ext[i];
-		if (sext->ext_ptr->extid_start <= extid &&
-		    sext->ext_ptr->extid_end >= extid) {
-			if (sext->dis_idx < KVM_RISCV_SBI_EXT_MAX &&
-			    scontext->extension_disabled[sext->dis_idx])
+		entry = &sbi_ext[i];
+		ext = entry->ext_ptr;
+
+		if (ext->extid_start <= extid && ext->extid_end >= extid) {
+			if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
+				return ext;
+			if (scontext->extension_disabled[entry->dis_idx])
+				return NULL;
+			if (ext->probe && !ext->probe(vcpu)) {
+				scontext->extension_disabled[entry->dis_idx] = true;
 				return NULL;
-			return sbi_ext[i].ext_ptr;
+			}
+			return ext;
 		}
 	}
 
-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [PATCH 2/3] RISC-V: KVM: Rename dis_idx to ext_idx
  2023-04-26 17:13 ` Andrew Jones
@ 2023-04-26 17:13   ` Andrew Jones
  -1 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-04-26 17:13 UTC (permalink / raw)
  To: kvm-riscv

Make the name of the extension_disabled[] index more general in
order to expand its application.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/kvm/vcpu_sbi.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index aa3c126d2e3c..a1a82f0fbad2 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -31,49 +31,49 @@ static const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_pmu = {
 #endif
 
 struct kvm_riscv_sbi_extension_entry {
-	enum KVM_RISCV_SBI_EXT_ID dis_idx;
+	enum KVM_RISCV_SBI_EXT_ID ext_idx;
 	const struct kvm_vcpu_sbi_extension *ext_ptr;
 };
 
 static const struct kvm_riscv_sbi_extension_entry sbi_ext[] = {
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_V01,
+		.ext_idx = KVM_RISCV_SBI_EXT_V01,
 		.ext_ptr = &vcpu_sbi_ext_v01,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_MAX, /* Can't be disabled */
+		.ext_idx = KVM_RISCV_SBI_EXT_MAX, /* Can't be disabled */
 		.ext_ptr = &vcpu_sbi_ext_base,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_TIME,
+		.ext_idx = KVM_RISCV_SBI_EXT_TIME,
 		.ext_ptr = &vcpu_sbi_ext_time,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_IPI,
+		.ext_idx = KVM_RISCV_SBI_EXT_IPI,
 		.ext_ptr = &vcpu_sbi_ext_ipi,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_RFENCE,
+		.ext_idx = KVM_RISCV_SBI_EXT_RFENCE,
 		.ext_ptr = &vcpu_sbi_ext_rfence,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_SRST,
+		.ext_idx = KVM_RISCV_SBI_EXT_SRST,
 		.ext_ptr = &vcpu_sbi_ext_srst,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_HSM,
+		.ext_idx = KVM_RISCV_SBI_EXT_HSM,
 		.ext_ptr = &vcpu_sbi_ext_hsm,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_PMU,
+		.ext_idx = KVM_RISCV_SBI_EXT_PMU,
 		.ext_ptr = &vcpu_sbi_ext_pmu,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
+		.ext_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
 		.ext_ptr = &vcpu_sbi_ext_experimental,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_VENDOR,
+		.ext_idx = KVM_RISCV_SBI_EXT_VENDOR,
 		.ext_ptr = &vcpu_sbi_ext_vendor,
 	},
 };
@@ -147,7 +147,7 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
-		if (sbi_ext[i].dis_idx == reg_num) {
+		if (sbi_ext[i].ext_idx == reg_num) {
 			sext = &sbi_ext[i];
 			break;
 		}
@@ -155,7 +155,7 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
 	if (!sext)
 		return -ENOENT;
 
-	scontext->extension_disabled[sext->dis_idx] = !reg_val;
+	scontext->extension_disabled[sext->ext_idx] = !reg_val;
 
 	return 0;
 }
@@ -172,7 +172,7 @@ static int riscv_vcpu_get_sbi_ext_single(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
-		if (sbi_ext[i].dis_idx == reg_num) {
+		if (sbi_ext[i].ext_idx == reg_num) {
 			sext = &sbi_ext[i];
 			break;
 		}
@@ -180,7 +180,7 @@ static int riscv_vcpu_get_sbi_ext_single(struct kvm_vcpu *vcpu,
 	if (!sext)
 		return -ENOENT;
 
-	*reg_val = !scontext->extension_disabled[sext->dis_idx];
+	*reg_val = !scontext->extension_disabled[sext->ext_idx];
 
 	return 0;
 }
@@ -317,12 +317,12 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
 		ext = entry->ext_ptr;
 
 		if (ext->extid_start <= extid && ext->extid_end >= extid) {
-			if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
+			if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
 				return ext;
-			if (scontext->extension_disabled[entry->dis_idx])
+			if (scontext->extension_disabled[entry->ext_idx])
 				return NULL;
 			if (ext->probe && !ext->probe(vcpu)) {
-				scontext->extension_disabled[entry->dis_idx] = true;
+				scontext->extension_disabled[entry->ext_idx] = true;
 				return NULL;
 			}
 			return ext;
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [PATCH 2/3] RISC-V: KVM: Rename dis_idx to ext_idx
@ 2023-04-26 17:13   ` Andrew Jones
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-04-26 17:13 UTC (permalink / raw)
  To: kvm-riscv, linux-riscv
  Cc: 'Palmer Dabbelt ', 'Anup Patel ',
	'Atish Patra ', 'Albert Ou ',
	'Paul Walmsley '

Make the name of the extension_disabled[] index more general in
order to expand its application.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/kvm/vcpu_sbi.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index aa3c126d2e3c..a1a82f0fbad2 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -31,49 +31,49 @@ static const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_pmu = {
 #endif
 
 struct kvm_riscv_sbi_extension_entry {
-	enum KVM_RISCV_SBI_EXT_ID dis_idx;
+	enum KVM_RISCV_SBI_EXT_ID ext_idx;
 	const struct kvm_vcpu_sbi_extension *ext_ptr;
 };
 
 static const struct kvm_riscv_sbi_extension_entry sbi_ext[] = {
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_V01,
+		.ext_idx = KVM_RISCV_SBI_EXT_V01,
 		.ext_ptr = &vcpu_sbi_ext_v01,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_MAX, /* Can't be disabled */
+		.ext_idx = KVM_RISCV_SBI_EXT_MAX, /* Can't be disabled */
 		.ext_ptr = &vcpu_sbi_ext_base,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_TIME,
+		.ext_idx = KVM_RISCV_SBI_EXT_TIME,
 		.ext_ptr = &vcpu_sbi_ext_time,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_IPI,
+		.ext_idx = KVM_RISCV_SBI_EXT_IPI,
 		.ext_ptr = &vcpu_sbi_ext_ipi,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_RFENCE,
+		.ext_idx = KVM_RISCV_SBI_EXT_RFENCE,
 		.ext_ptr = &vcpu_sbi_ext_rfence,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_SRST,
+		.ext_idx = KVM_RISCV_SBI_EXT_SRST,
 		.ext_ptr = &vcpu_sbi_ext_srst,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_HSM,
+		.ext_idx = KVM_RISCV_SBI_EXT_HSM,
 		.ext_ptr = &vcpu_sbi_ext_hsm,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_PMU,
+		.ext_idx = KVM_RISCV_SBI_EXT_PMU,
 		.ext_ptr = &vcpu_sbi_ext_pmu,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
+		.ext_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
 		.ext_ptr = &vcpu_sbi_ext_experimental,
 	},
 	{
-		.dis_idx = KVM_RISCV_SBI_EXT_VENDOR,
+		.ext_idx = KVM_RISCV_SBI_EXT_VENDOR,
 		.ext_ptr = &vcpu_sbi_ext_vendor,
 	},
 };
@@ -147,7 +147,7 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
-		if (sbi_ext[i].dis_idx == reg_num) {
+		if (sbi_ext[i].ext_idx == reg_num) {
 			sext = &sbi_ext[i];
 			break;
 		}
@@ -155,7 +155,7 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
 	if (!sext)
 		return -ENOENT;
 
-	scontext->extension_disabled[sext->dis_idx] = !reg_val;
+	scontext->extension_disabled[sext->ext_idx] = !reg_val;
 
 	return 0;
 }
@@ -172,7 +172,7 @@ static int riscv_vcpu_get_sbi_ext_single(struct kvm_vcpu *vcpu,
 		return -EINVAL;
 
 	for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
-		if (sbi_ext[i].dis_idx == reg_num) {
+		if (sbi_ext[i].ext_idx == reg_num) {
 			sext = &sbi_ext[i];
 			break;
 		}
@@ -180,7 +180,7 @@ static int riscv_vcpu_get_sbi_ext_single(struct kvm_vcpu *vcpu,
 	if (!sext)
 		return -ENOENT;
 
-	*reg_val = !scontext->extension_disabled[sext->dis_idx];
+	*reg_val = !scontext->extension_disabled[sext->ext_idx];
 
 	return 0;
 }
@@ -317,12 +317,12 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
 		ext = entry->ext_ptr;
 
 		if (ext->extid_start <= extid && ext->extid_end >= extid) {
-			if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
+			if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
 				return ext;
-			if (scontext->extension_disabled[entry->dis_idx])
+			if (scontext->extension_disabled[entry->ext_idx])
 				return NULL;
 			if (ext->probe && !ext->probe(vcpu)) {
-				scontext->extension_disabled[entry->dis_idx] = true;
+				scontext->extension_disabled[entry->ext_idx] = true;
 				return NULL;
 			}
 			return ext;
-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [PATCH 3/3] RISC-V: KVM: Introduce extension_enabled
  2023-04-26 17:13 ` Andrew Jones
@ 2023-04-26 17:13   ` Andrew Jones
  -1 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-04-26 17:13 UTC (permalink / raw)
  To: kvm-riscv

We need three SBI extension status values (uninitialized, enabled,
and disabled). Pairing another boolean array, extension_enabled[],
with the boolean array extension_disabled[] provides four states.
Using a pair of boolean arrays, which may eventually be changed to
a pair of bitmaps, is more space efficient than using one enum
status field. Apply the new (enabled=1,disabled=0) state, which
means either the extension doesn't have a probe function or that
its probe was successful, to avoid more than one probe of the
extension.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 +++++++++
 arch/riscv/kvm/vcpu_sbi.c             | 11 ++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
index 4278125a38a5..e3c5e1d15e93 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
@@ -16,6 +16,15 @@
 
 struct kvm_vcpu_sbi_context {
 	int return_handled;
+	/*
+	 * extension_enabled[] and extension_disabled[] provide SBI
+	 * extensions four status values, of which we need three:
+	 * (0,0) uninitialized, (1,0) enabled, (0,1) disabled. Using
+	 * a pair of boolean arrays, which may eventually be changed
+	 * to a pair of bitmaps, is more space efficient than using
+	 * one enum status field.
+	 */
+	bool extension_enabled[KVM_RISCV_SBI_EXT_MAX];
 	bool extension_disabled[KVM_RISCV_SBI_EXT_MAX];
 };
 
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index a1a82f0fbad2..344d38bbe06a 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -155,6 +155,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
 	if (!sext)
 		return -ENOENT;
 
+	/*
+	 * We can't set scontext->extension_enabled[] to reg_val since the
+	 * extension may have a probe() function which needs to confirm
+	 * enablement first. Only set extension_disabled[] here and leave
+	 * the extension_enabled[] setting to kvm_vcpu_sbi_find_ext().
+	 */
 	scontext->extension_disabled[sext->ext_idx] = !reg_val;
 
 	return 0;
@@ -317,7 +323,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
 		ext = entry->ext_ptr;
 
 		if (ext->extid_start <= extid && ext->extid_end >= extid) {
-			if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
+			if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX ||
+			    scontext->extension_enabled[entry->ext_idx])
 				return ext;
 			if (scontext->extension_disabled[entry->ext_idx])
 				return NULL;
@@ -325,6 +332,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
 				scontext->extension_disabled[entry->ext_idx] = true;
 				return NULL;
 			}
+
+			scontext->extension_enabled[entry->ext_idx] = true;
 			return ext;
 		}
 	}
-- 
2.39.2



^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [PATCH 3/3] RISC-V: KVM: Introduce extension_enabled
@ 2023-04-26 17:13   ` Andrew Jones
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-04-26 17:13 UTC (permalink / raw)
  To: kvm-riscv, linux-riscv
  Cc: 'Palmer Dabbelt ', 'Anup Patel ',
	'Atish Patra ', 'Albert Ou ',
	'Paul Walmsley '

We need three SBI extension status values (uninitialized, enabled,
and disabled). Pairing another boolean array, extension_enabled[],
with the boolean array extension_disabled[] provides four states.
Using a pair of boolean arrays, which may eventually be changed to
a pair of bitmaps, is more space efficient than using one enum
status field. Apply the new (enabled=1,disabled=0) state, which
means either the extension doesn't have a probe function or that
its probe was successful, to avoid more than one probe of the
extension.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 +++++++++
 arch/riscv/kvm/vcpu_sbi.c             | 11 ++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
index 4278125a38a5..e3c5e1d15e93 100644
--- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
+++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
@@ -16,6 +16,15 @@
 
 struct kvm_vcpu_sbi_context {
 	int return_handled;
+	/*
+	 * extension_enabled[] and extension_disabled[] provide SBI
+	 * extensions four status values, of which we need three:
+	 * (0,0) uninitialized, (1,0) enabled, (0,1) disabled. Using
+	 * a pair of boolean arrays, which may eventually be changed
+	 * to a pair of bitmaps, is more space efficient than using
+	 * one enum status field.
+	 */
+	bool extension_enabled[KVM_RISCV_SBI_EXT_MAX];
 	bool extension_disabled[KVM_RISCV_SBI_EXT_MAX];
 };
 
diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index a1a82f0fbad2..344d38bbe06a 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -155,6 +155,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
 	if (!sext)
 		return -ENOENT;
 
+	/*
+	 * We can't set scontext->extension_enabled[] to reg_val since the
+	 * extension may have a probe() function which needs to confirm
+	 * enablement first. Only set extension_disabled[] here and leave
+	 * the extension_enabled[] setting to kvm_vcpu_sbi_find_ext().
+	 */
 	scontext->extension_disabled[sext->ext_idx] = !reg_val;
 
 	return 0;
@@ -317,7 +323,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
 		ext = entry->ext_ptr;
 
 		if (ext->extid_start <= extid && ext->extid_end >= extid) {
-			if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
+			if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX ||
+			    scontext->extension_enabled[entry->ext_idx])
 				return ext;
 			if (scontext->extension_disabled[entry->ext_idx])
 				return NULL;
@@ -325,6 +332,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
 				scontext->extension_disabled[entry->ext_idx] = true;
 				return NULL;
 			}
+
+			scontext->extension_enabled[entry->ext_idx] = true;
 			return ext;
 		}
 	}
-- 
2.39.2


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [PATCH 1/3] RISC-V: KVM: Disable SBI extension when its probe fails
  2023-04-26 17:13   ` Andrew Jones
@ 2023-05-19 14:57     ` Anup Patel
  -1 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-19 14:57 UTC (permalink / raw)
  To: kvm-riscv

On Wed, Apr 26, 2023 at 10:43?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> When an SBI extension specific probe function exists and fails, then
> use the extension_disabled context to disable the extension. This
> ensures the extension's functions cannot be invoked. Doing the
> disabling in kvm_vcpu_sbi_find_ext() allows it to be done lazily
> on its first use. Checking extension_disabled prior to probing
> ensures the probe is only executed once for disabled extensions.
> Later patches ensure we only execute probe once for enabled
> extensions as well.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/kvm/vcpu_sbi.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index e52fde504433..aa3c126d2e3c 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -307,18 +307,25 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu,
>  const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
>                                 struct kvm_vcpu *vcpu, unsigned long extid)
>  {
> -       int i;
> -       const struct kvm_riscv_sbi_extension_entry *sext;
>         struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
> +       const struct kvm_riscv_sbi_extension_entry *entry;
> +       const struct kvm_vcpu_sbi_extension *ext;
> +       int i;
>
>         for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
> -               sext = &sbi_ext[i];
> -               if (sext->ext_ptr->extid_start <= extid &&
> -                   sext->ext_ptr->extid_end >= extid) {
> -                       if (sext->dis_idx < KVM_RISCV_SBI_EXT_MAX &&
> -                           scontext->extension_disabled[sext->dis_idx])
> +               entry = &sbi_ext[i];
> +               ext = entry->ext_ptr;
> +
> +               if (ext->extid_start <= extid && ext->extid_end >= extid) {
> +                       if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
> +                               return ext;
> +                       if (scontext->extension_disabled[entry->dis_idx])
> +                               return NULL;
> +                       if (ext->probe && !ext->probe(vcpu)) {

Calling probe() upon every kvm_vcpu_sbi_find_ext() will simply slow down
all SBI calls.

How about caching probe return values in "struct kvm_vcpu_sbi_context"
as an array?

Maybe we can have two arrays:
1) probe_done[] : Boolean array to check whether probe was done.
2) probe_retval[] : Cached probe() return values.

The SBI_EXT_BASE_PROBE_EXT call should also return the
cached value.

> +                               scontext->extension_disabled[entry->dis_idx] = true;
>                                 return NULL;
> -                       return sbi_ext[i].ext_ptr;
> +                       }
> +                       return ext;
>                 }
>         }
>
> --
> 2.39.2
>

Regards,
Anup


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] RISC-V: KVM: Disable SBI extension when its probe fails
@ 2023-05-19 14:57     ` Anup Patel
  0 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-19 14:57 UTC (permalink / raw)
  To: Andrew Jones
  Cc: kvm-riscv, linux-riscv, Palmer Dabbelt, Atish Patra, Albert Ou,
	Paul Walmsley

On Wed, Apr 26, 2023 at 10:43 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> When an SBI extension specific probe function exists and fails, then
> use the extension_disabled context to disable the extension. This
> ensures the extension's functions cannot be invoked. Doing the
> disabling in kvm_vcpu_sbi_find_ext() allows it to be done lazily
> on its first use. Checking extension_disabled prior to probing
> ensures the probe is only executed once for disabled extensions.
> Later patches ensure we only execute probe once for enabled
> extensions as well.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/kvm/vcpu_sbi.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index e52fde504433..aa3c126d2e3c 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -307,18 +307,25 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu,
>  const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
>                                 struct kvm_vcpu *vcpu, unsigned long extid)
>  {
> -       int i;
> -       const struct kvm_riscv_sbi_extension_entry *sext;
>         struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
> +       const struct kvm_riscv_sbi_extension_entry *entry;
> +       const struct kvm_vcpu_sbi_extension *ext;
> +       int i;
>
>         for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
> -               sext = &sbi_ext[i];
> -               if (sext->ext_ptr->extid_start <= extid &&
> -                   sext->ext_ptr->extid_end >= extid) {
> -                       if (sext->dis_idx < KVM_RISCV_SBI_EXT_MAX &&
> -                           scontext->extension_disabled[sext->dis_idx])
> +               entry = &sbi_ext[i];
> +               ext = entry->ext_ptr;
> +
> +               if (ext->extid_start <= extid && ext->extid_end >= extid) {
> +                       if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
> +                               return ext;
> +                       if (scontext->extension_disabled[entry->dis_idx])
> +                               return NULL;
> +                       if (ext->probe && !ext->probe(vcpu)) {

Calling probe() upon every kvm_vcpu_sbi_find_ext() will simply slow down
all SBI calls.

How about caching probe return values in "struct kvm_vcpu_sbi_context"
as an array?

Maybe we can have two arrays:
1) probe_done[] : Boolean array to check whether probe was done.
2) probe_retval[] : Cached probe() return values.

The SBI_EXT_BASE_PROBE_EXT call should also return the
cached value.

> +                               scontext->extension_disabled[entry->dis_idx] = true;
>                                 return NULL;
> -                       return sbi_ext[i].ext_ptr;
> +                       }
> +                       return ext;
>                 }
>         }
>
> --
> 2.39.2
>

Regards,
Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH 2/3] RISC-V: KVM: Rename dis_idx to ext_idx
  2023-04-26 17:13   ` Andrew Jones
@ 2023-05-19 14:58     ` Anup Patel
  -1 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-19 14:58 UTC (permalink / raw)
  To: kvm-riscv

On Wed, Apr 26, 2023 at 10:43?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> Make the name of the extension_disabled[] index more general in
> order to expand its application.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/kvm/vcpu_sbi.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index aa3c126d2e3c..a1a82f0fbad2 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -31,49 +31,49 @@ static const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_pmu = {
>  #endif
>
>  struct kvm_riscv_sbi_extension_entry {
> -       enum KVM_RISCV_SBI_EXT_ID dis_idx;
> +       enum KVM_RISCV_SBI_EXT_ID ext_idx;
>         const struct kvm_vcpu_sbi_extension *ext_ptr;
>  };
>
>  static const struct kvm_riscv_sbi_extension_entry sbi_ext[] = {
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_V01,
> +               .ext_idx = KVM_RISCV_SBI_EXT_V01,
>                 .ext_ptr = &vcpu_sbi_ext_v01,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_MAX, /* Can't be disabled */
> +               .ext_idx = KVM_RISCV_SBI_EXT_MAX, /* Can't be disabled */
>                 .ext_ptr = &vcpu_sbi_ext_base,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_TIME,
> +               .ext_idx = KVM_RISCV_SBI_EXT_TIME,
>                 .ext_ptr = &vcpu_sbi_ext_time,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_IPI,
> +               .ext_idx = KVM_RISCV_SBI_EXT_IPI,
>                 .ext_ptr = &vcpu_sbi_ext_ipi,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_RFENCE,
> +               .ext_idx = KVM_RISCV_SBI_EXT_RFENCE,
>                 .ext_ptr = &vcpu_sbi_ext_rfence,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_SRST,
> +               .ext_idx = KVM_RISCV_SBI_EXT_SRST,
>                 .ext_ptr = &vcpu_sbi_ext_srst,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_HSM,
> +               .ext_idx = KVM_RISCV_SBI_EXT_HSM,
>                 .ext_ptr = &vcpu_sbi_ext_hsm,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_PMU,
> +               .ext_idx = KVM_RISCV_SBI_EXT_PMU,
>                 .ext_ptr = &vcpu_sbi_ext_pmu,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
> +               .ext_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
>                 .ext_ptr = &vcpu_sbi_ext_experimental,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_VENDOR,
> +               .ext_idx = KVM_RISCV_SBI_EXT_VENDOR,
>                 .ext_ptr = &vcpu_sbi_ext_vendor,
>         },
>  };
> @@ -147,7 +147,7 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
>                 return -EINVAL;
>
>         for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
> -               if (sbi_ext[i].dis_idx == reg_num) {
> +               if (sbi_ext[i].ext_idx == reg_num) {
>                         sext = &sbi_ext[i];
>                         break;
>                 }
> @@ -155,7 +155,7 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
>         if (!sext)
>                 return -ENOENT;
>
> -       scontext->extension_disabled[sext->dis_idx] = !reg_val;
> +       scontext->extension_disabled[sext->ext_idx] = !reg_val;
>
>         return 0;
>  }
> @@ -172,7 +172,7 @@ static int riscv_vcpu_get_sbi_ext_single(struct kvm_vcpu *vcpu,
>                 return -EINVAL;
>
>         for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
> -               if (sbi_ext[i].dis_idx == reg_num) {
> +               if (sbi_ext[i].ext_idx == reg_num) {
>                         sext = &sbi_ext[i];
>                         break;
>                 }
> @@ -180,7 +180,7 @@ static int riscv_vcpu_get_sbi_ext_single(struct kvm_vcpu *vcpu,
>         if (!sext)
>                 return -ENOENT;
>
> -       *reg_val = !scontext->extension_disabled[sext->dis_idx];
> +       *reg_val = !scontext->extension_disabled[sext->ext_idx];
>
>         return 0;
>  }
> @@ -317,12 +317,12 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
>                 ext = entry->ext_ptr;
>
>                 if (ext->extid_start <= extid && ext->extid_end >= extid) {
> -                       if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
> +                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
>                                 return ext;
> -                       if (scontext->extension_disabled[entry->dis_idx])
> +                       if (scontext->extension_disabled[entry->ext_idx])
>                                 return NULL;
>                         if (ext->probe && !ext->probe(vcpu)) {
> -                               scontext->extension_disabled[entry->dis_idx] = true;
> +                               scontext->extension_disabled[entry->ext_idx] = true;
>                                 return NULL;
>                         }
>                         return ext;
> --
> 2.39.2
>


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 2/3] RISC-V: KVM: Rename dis_idx to ext_idx
@ 2023-05-19 14:58     ` Anup Patel
  0 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-19 14:58 UTC (permalink / raw)
  To: Andrew Jones
  Cc: kvm-riscv, linux-riscv, Palmer Dabbelt, Atish Patra, Albert Ou,
	Paul Walmsley

On Wed, Apr 26, 2023 at 10:43 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> Make the name of the extension_disabled[] index more general in
> order to expand its application.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  arch/riscv/kvm/vcpu_sbi.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index aa3c126d2e3c..a1a82f0fbad2 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -31,49 +31,49 @@ static const struct kvm_vcpu_sbi_extension vcpu_sbi_ext_pmu = {
>  #endif
>
>  struct kvm_riscv_sbi_extension_entry {
> -       enum KVM_RISCV_SBI_EXT_ID dis_idx;
> +       enum KVM_RISCV_SBI_EXT_ID ext_idx;
>         const struct kvm_vcpu_sbi_extension *ext_ptr;
>  };
>
>  static const struct kvm_riscv_sbi_extension_entry sbi_ext[] = {
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_V01,
> +               .ext_idx = KVM_RISCV_SBI_EXT_V01,
>                 .ext_ptr = &vcpu_sbi_ext_v01,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_MAX, /* Can't be disabled */
> +               .ext_idx = KVM_RISCV_SBI_EXT_MAX, /* Can't be disabled */
>                 .ext_ptr = &vcpu_sbi_ext_base,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_TIME,
> +               .ext_idx = KVM_RISCV_SBI_EXT_TIME,
>                 .ext_ptr = &vcpu_sbi_ext_time,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_IPI,
> +               .ext_idx = KVM_RISCV_SBI_EXT_IPI,
>                 .ext_ptr = &vcpu_sbi_ext_ipi,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_RFENCE,
> +               .ext_idx = KVM_RISCV_SBI_EXT_RFENCE,
>                 .ext_ptr = &vcpu_sbi_ext_rfence,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_SRST,
> +               .ext_idx = KVM_RISCV_SBI_EXT_SRST,
>                 .ext_ptr = &vcpu_sbi_ext_srst,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_HSM,
> +               .ext_idx = KVM_RISCV_SBI_EXT_HSM,
>                 .ext_ptr = &vcpu_sbi_ext_hsm,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_PMU,
> +               .ext_idx = KVM_RISCV_SBI_EXT_PMU,
>                 .ext_ptr = &vcpu_sbi_ext_pmu,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
> +               .ext_idx = KVM_RISCV_SBI_EXT_EXPERIMENTAL,
>                 .ext_ptr = &vcpu_sbi_ext_experimental,
>         },
>         {
> -               .dis_idx = KVM_RISCV_SBI_EXT_VENDOR,
> +               .ext_idx = KVM_RISCV_SBI_EXT_VENDOR,
>                 .ext_ptr = &vcpu_sbi_ext_vendor,
>         },
>  };
> @@ -147,7 +147,7 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
>                 return -EINVAL;
>
>         for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
> -               if (sbi_ext[i].dis_idx == reg_num) {
> +               if (sbi_ext[i].ext_idx == reg_num) {
>                         sext = &sbi_ext[i];
>                         break;
>                 }
> @@ -155,7 +155,7 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
>         if (!sext)
>                 return -ENOENT;
>
> -       scontext->extension_disabled[sext->dis_idx] = !reg_val;
> +       scontext->extension_disabled[sext->ext_idx] = !reg_val;
>
>         return 0;
>  }
> @@ -172,7 +172,7 @@ static int riscv_vcpu_get_sbi_ext_single(struct kvm_vcpu *vcpu,
>                 return -EINVAL;
>
>         for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
> -               if (sbi_ext[i].dis_idx == reg_num) {
> +               if (sbi_ext[i].ext_idx == reg_num) {
>                         sext = &sbi_ext[i];
>                         break;
>                 }
> @@ -180,7 +180,7 @@ static int riscv_vcpu_get_sbi_ext_single(struct kvm_vcpu *vcpu,
>         if (!sext)
>                 return -ENOENT;
>
> -       *reg_val = !scontext->extension_disabled[sext->dis_idx];
> +       *reg_val = !scontext->extension_disabled[sext->ext_idx];
>
>         return 0;
>  }
> @@ -317,12 +317,12 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
>                 ext = entry->ext_ptr;
>
>                 if (ext->extid_start <= extid && ext->extid_end >= extid) {
> -                       if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
> +                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
>                                 return ext;
> -                       if (scontext->extension_disabled[entry->dis_idx])
> +                       if (scontext->extension_disabled[entry->ext_idx])
>                                 return NULL;
>                         if (ext->probe && !ext->probe(vcpu)) {
> -                               scontext->extension_disabled[entry->dis_idx] = true;
> +                               scontext->extension_disabled[entry->ext_idx] = true;
>                                 return NULL;
>                         }
>                         return ext;
> --
> 2.39.2
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH 1/3] RISC-V: KVM: Disable SBI extension when its probe fails
  2023-05-19 14:57     ` Anup Patel
@ 2023-05-19 15:02       ` Anup Patel
  -1 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-19 15:02 UTC (permalink / raw)
  To: kvm-riscv

On Fri, May 19, 2023 at 8:27?PM Anup Patel <anup@brainfault.org> wrote:
>
> On Wed, Apr 26, 2023 at 10:43?PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > When an SBI extension specific probe function exists and fails, then
> > use the extension_disabled context to disable the extension. This
> > ensures the extension's functions cannot be invoked. Doing the
> > disabling in kvm_vcpu_sbi_find_ext() allows it to be done lazily
> > on its first use. Checking extension_disabled prior to probing
> > ensures the probe is only executed once for disabled extensions.
> > Later patches ensure we only execute probe once for enabled
> > extensions as well.
> >
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/kvm/vcpu_sbi.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> > index e52fde504433..aa3c126d2e3c 100644
> > --- a/arch/riscv/kvm/vcpu_sbi.c
> > +++ b/arch/riscv/kvm/vcpu_sbi.c
> > @@ -307,18 +307,25 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu,
> >  const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> >                                 struct kvm_vcpu *vcpu, unsigned long extid)
> >  {
> > -       int i;
> > -       const struct kvm_riscv_sbi_extension_entry *sext;
> >         struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
> > +       const struct kvm_riscv_sbi_extension_entry *entry;
> > +       const struct kvm_vcpu_sbi_extension *ext;
> > +       int i;
> >
> >         for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
> > -               sext = &sbi_ext[i];
> > -               if (sext->ext_ptr->extid_start <= extid &&
> > -                   sext->ext_ptr->extid_end >= extid) {
> > -                       if (sext->dis_idx < KVM_RISCV_SBI_EXT_MAX &&
> > -                           scontext->extension_disabled[sext->dis_idx])
> > +               entry = &sbi_ext[i];
> > +               ext = entry->ext_ptr;
> > +
> > +               if (ext->extid_start <= extid && ext->extid_end >= extid) {
> > +                       if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
> > +                               return ext;
> > +                       if (scontext->extension_disabled[entry->dis_idx])
> > +                               return NULL;
> > +                       if (ext->probe && !ext->probe(vcpu)) {
>
> Calling probe() upon every kvm_vcpu_sbi_find_ext() will simply slow down
> all SBI calls.
>
> How about caching probe return values in "struct kvm_vcpu_sbi_context"
> as an array?
>
> Maybe we can have two arrays:
> 1) probe_done[] : Boolean array to check whether probe was done.
> 2) probe_retval[] : Cached probe() return values.
>
> The SBI_EXT_BASE_PROBE_EXT call should also return the
> cached value.

Ignore this comment. Your PATCH3 does the same thing.

Regards,
Anup

>
> > +                               scontext->extension_disabled[entry->dis_idx] = true;
> >                                 return NULL;
> > -                       return sbi_ext[i].ext_ptr;
> > +                       }
> > +                       return ext;
> >                 }
> >         }
> >
> > --
> > 2.39.2
> >
>
> Regards,
> Anup


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 1/3] RISC-V: KVM: Disable SBI extension when its probe fails
@ 2023-05-19 15:02       ` Anup Patel
  0 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-19 15:02 UTC (permalink / raw)
  To: Andrew Jones
  Cc: kvm-riscv, linux-riscv, Palmer Dabbelt, Atish Patra, Albert Ou,
	Paul Walmsley

On Fri, May 19, 2023 at 8:27 PM Anup Patel <anup@brainfault.org> wrote:
>
> On Wed, Apr 26, 2023 at 10:43 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > When an SBI extension specific probe function exists and fails, then
> > use the extension_disabled context to disable the extension. This
> > ensures the extension's functions cannot be invoked. Doing the
> > disabling in kvm_vcpu_sbi_find_ext() allows it to be done lazily
> > on its first use. Checking extension_disabled prior to probing
> > ensures the probe is only executed once for disabled extensions.
> > Later patches ensure we only execute probe once for enabled
> > extensions as well.
> >
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/kvm/vcpu_sbi.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> > index e52fde504433..aa3c126d2e3c 100644
> > --- a/arch/riscv/kvm/vcpu_sbi.c
> > +++ b/arch/riscv/kvm/vcpu_sbi.c
> > @@ -307,18 +307,25 @@ int kvm_riscv_vcpu_get_reg_sbi_ext(struct kvm_vcpu *vcpu,
> >  const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> >                                 struct kvm_vcpu *vcpu, unsigned long extid)
> >  {
> > -       int i;
> > -       const struct kvm_riscv_sbi_extension_entry *sext;
> >         struct kvm_vcpu_sbi_context *scontext = &vcpu->arch.sbi_context;
> > +       const struct kvm_riscv_sbi_extension_entry *entry;
> > +       const struct kvm_vcpu_sbi_extension *ext;
> > +       int i;
> >
> >         for (i = 0; i < ARRAY_SIZE(sbi_ext); i++) {
> > -               sext = &sbi_ext[i];
> > -               if (sext->ext_ptr->extid_start <= extid &&
> > -                   sext->ext_ptr->extid_end >= extid) {
> > -                       if (sext->dis_idx < KVM_RISCV_SBI_EXT_MAX &&
> > -                           scontext->extension_disabled[sext->dis_idx])
> > +               entry = &sbi_ext[i];
> > +               ext = entry->ext_ptr;
> > +
> > +               if (ext->extid_start <= extid && ext->extid_end >= extid) {
> > +                       if (entry->dis_idx >= KVM_RISCV_SBI_EXT_MAX)
> > +                               return ext;
> > +                       if (scontext->extension_disabled[entry->dis_idx])
> > +                               return NULL;
> > +                       if (ext->probe && !ext->probe(vcpu)) {
>
> Calling probe() upon every kvm_vcpu_sbi_find_ext() will simply slow down
> all SBI calls.
>
> How about caching probe return values in "struct kvm_vcpu_sbi_context"
> as an array?
>
> Maybe we can have two arrays:
> 1) probe_done[] : Boolean array to check whether probe was done.
> 2) probe_retval[] : Cached probe() return values.
>
> The SBI_EXT_BASE_PROBE_EXT call should also return the
> cached value.

Ignore this comment. Your PATCH3 does the same thing.

Regards,
Anup

>
> > +                               scontext->extension_disabled[entry->dis_idx] = true;
> >                                 return NULL;
> > -                       return sbi_ext[i].ext_ptr;
> > +                       }
> > +                       return ext;
> >                 }
> >         }
> >
> > --
> > 2.39.2
> >
>
> Regards,
> Anup

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH 3/3] RISC-V: KVM: Introduce extension_enabled
  2023-04-26 17:13   ` Andrew Jones
@ 2023-05-19 15:05     ` Anup Patel
  -1 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-19 15:05 UTC (permalink / raw)
  To: kvm-riscv

On Wed, Apr 26, 2023 at 10:43?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> We need three SBI extension status values (uninitialized, enabled,
> and disabled). Pairing another boolean array, extension_enabled[],
> with the boolean array extension_disabled[] provides four states.
> Using a pair of boolean arrays, which may eventually be changed to
> a pair of bitmaps, is more space efficient than using one enum
> status field. Apply the new (enabled=1,disabled=0) state, which
> means either the extension doesn't have a probe function or that
> its probe was successful, to avoid more than one probe of the
> extension.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 +++++++++
>  arch/riscv/kvm/vcpu_sbi.c             | 11 ++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index 4278125a38a5..e3c5e1d15e93 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -16,6 +16,15 @@
>
>  struct kvm_vcpu_sbi_context {
>         int return_handled;
> +       /*
> +        * extension_enabled[] and extension_disabled[] provide SBI
> +        * extensions four status values, of which we need three:
> +        * (0,0) uninitialized, (1,0) enabled, (0,1) disabled. Using
> +        * a pair of boolean arrays, which may eventually be changed
> +        * to a pair of bitmaps, is more space efficient than using
> +        * one enum status field.
> +        */
> +       bool extension_enabled[KVM_RISCV_SBI_EXT_MAX];

The extension_enabled[] is not an appropriate name.

How about extension_probe_allowed[] ?

Any better name ?

Regards,
Anup

>         bool extension_disabled[KVM_RISCV_SBI_EXT_MAX];
>  };
>
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index a1a82f0fbad2..344d38bbe06a 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -155,6 +155,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
>         if (!sext)
>                 return -ENOENT;
>
> +       /*
> +        * We can't set scontext->extension_enabled[] to reg_val since the
> +        * extension may have a probe() function which needs to confirm
> +        * enablement first. Only set extension_disabled[] here and leave
> +        * the extension_enabled[] setting to kvm_vcpu_sbi_find_ext().
> +        */
>         scontext->extension_disabled[sext->ext_idx] = !reg_val;
>
>         return 0;
> @@ -317,7 +323,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
>                 ext = entry->ext_ptr;
>
>                 if (ext->extid_start <= extid && ext->extid_end >= extid) {
> -                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
> +                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX ||
> +                           scontext->extension_enabled[entry->ext_idx])
>                                 return ext;
>                         if (scontext->extension_disabled[entry->ext_idx])
>                                 return NULL;
> @@ -325,6 +332,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
>                                 scontext->extension_disabled[entry->ext_idx] = true;
>                                 return NULL;
>                         }
> +
> +                       scontext->extension_enabled[entry->ext_idx] = true;
>                         return ext;
>                 }
>         }
> --
> 2.39.2
>


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] RISC-V: KVM: Introduce extension_enabled
@ 2023-05-19 15:05     ` Anup Patel
  0 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-19 15:05 UTC (permalink / raw)
  To: Andrew Jones
  Cc: kvm-riscv, linux-riscv, Palmer Dabbelt, Atish Patra, Albert Ou,
	Paul Walmsley

On Wed, Apr 26, 2023 at 10:43 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> We need three SBI extension status values (uninitialized, enabled,
> and disabled). Pairing another boolean array, extension_enabled[],
> with the boolean array extension_disabled[] provides four states.
> Using a pair of boolean arrays, which may eventually be changed to
> a pair of bitmaps, is more space efficient than using one enum
> status field. Apply the new (enabled=1,disabled=0) state, which
> means either the extension doesn't have a probe function or that
> its probe was successful, to avoid more than one probe of the
> extension.
>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 +++++++++
>  arch/riscv/kvm/vcpu_sbi.c             | 11 ++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> index 4278125a38a5..e3c5e1d15e93 100644
> --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> @@ -16,6 +16,15 @@
>
>  struct kvm_vcpu_sbi_context {
>         int return_handled;
> +       /*
> +        * extension_enabled[] and extension_disabled[] provide SBI
> +        * extensions four status values, of which we need three:
> +        * (0,0) uninitialized, (1,0) enabled, (0,1) disabled. Using
> +        * a pair of boolean arrays, which may eventually be changed
> +        * to a pair of bitmaps, is more space efficient than using
> +        * one enum status field.
> +        */
> +       bool extension_enabled[KVM_RISCV_SBI_EXT_MAX];

The extension_enabled[] is not an appropriate name.

How about extension_probe_allowed[] ?

Any better name ?

Regards,
Anup

>         bool extension_disabled[KVM_RISCV_SBI_EXT_MAX];
>  };
>
> diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> index a1a82f0fbad2..344d38bbe06a 100644
> --- a/arch/riscv/kvm/vcpu_sbi.c
> +++ b/arch/riscv/kvm/vcpu_sbi.c
> @@ -155,6 +155,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
>         if (!sext)
>                 return -ENOENT;
>
> +       /*
> +        * We can't set scontext->extension_enabled[] to reg_val since the
> +        * extension may have a probe() function which needs to confirm
> +        * enablement first. Only set extension_disabled[] here and leave
> +        * the extension_enabled[] setting to kvm_vcpu_sbi_find_ext().
> +        */
>         scontext->extension_disabled[sext->ext_idx] = !reg_val;
>
>         return 0;
> @@ -317,7 +323,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
>                 ext = entry->ext_ptr;
>
>                 if (ext->extid_start <= extid && ext->extid_end >= extid) {
> -                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
> +                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX ||
> +                           scontext->extension_enabled[entry->ext_idx])
>                                 return ext;
>                         if (scontext->extension_disabled[entry->ext_idx])
>                                 return NULL;
> @@ -325,6 +332,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
>                                 scontext->extension_disabled[entry->ext_idx] = true;
>                                 return NULL;
>                         }
> +
> +                       scontext->extension_enabled[entry->ext_idx] = true;
>                         return ext;
>                 }
>         }
> --
> 2.39.2
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH 3/3] RISC-V: KVM: Introduce extension_enabled
  2023-05-19 15:05     ` Anup Patel
@ 2023-05-22  9:28       ` Andrew Jones
  -1 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-05-22  9:28 UTC (permalink / raw)
  To: kvm-riscv

On Fri, May 19, 2023 at 08:35:31PM +0530, Anup Patel wrote:
> On Wed, Apr 26, 2023 at 10:43?PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > We need three SBI extension status values (uninitialized, enabled,
> > and disabled). Pairing another boolean array, extension_enabled[],
> > with the boolean array extension_disabled[] provides four states.
> > Using a pair of boolean arrays, which may eventually be changed to
> > a pair of bitmaps, is more space efficient than using one enum
> > status field. Apply the new (enabled=1,disabled=0) state, which
> > means either the extension doesn't have a probe function or that
> > its probe was successful, to avoid more than one probe of the
> > extension.
> >
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 +++++++++
> >  arch/riscv/kvm/vcpu_sbi.c             | 11 ++++++++++-
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > index 4278125a38a5..e3c5e1d15e93 100644
> > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > @@ -16,6 +16,15 @@
> >
> >  struct kvm_vcpu_sbi_context {
> >         int return_handled;
> > +       /*
> > +        * extension_enabled[] and extension_disabled[] provide SBI
> > +        * extensions four status values, of which we need three:
> > +        * (0,0) uninitialized, (1,0) enabled, (0,1) disabled. Using
> > +        * a pair of boolean arrays, which may eventually be changed
> > +        * to a pair of bitmaps, is more space efficient than using
> > +        * one enum status field.
> > +        */
> > +       bool extension_enabled[KVM_RISCV_SBI_EXT_MAX];
> 
> The extension_enabled[] is not an appropriate name.
> 
> How about extension_probe_allowed[] ?

"probe allowed" sounds to me like "we may call probe", but the meaning we
want is "we've already called probe and it succeeded, so the extension
is available" (or that we don't have probe, meaning the extension is
assumed available). I prefer 'available' for extensions over 'enabled',
but we already have 'disabled', so 'enabled' appeared to fit better.
Actually, it's probably best to improve the readability and simplicity,
at the expense of size, by dropping both the extension_enabled and
extension_disabled boolean arrays and switching to an 'extension_status'
enum, where we have SBI_EXT_UNINITIALIZED, SBI_EXT_AVAILABLE, and
SBI_EXT_UNAVAILABLE.

Thanks,
drew


> 
> Any better name ?
> 
> Regards,
> Anup
> 
> >         bool extension_disabled[KVM_RISCV_SBI_EXT_MAX];
> >  };
> >
> > diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> > index a1a82f0fbad2..344d38bbe06a 100644
> > --- a/arch/riscv/kvm/vcpu_sbi.c
> > +++ b/arch/riscv/kvm/vcpu_sbi.c
> > @@ -155,6 +155,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
> >         if (!sext)
> >                 return -ENOENT;
> >
> > +       /*
> > +        * We can't set scontext->extension_enabled[] to reg_val since the
> > +        * extension may have a probe() function which needs to confirm
> > +        * enablement first. Only set extension_disabled[] here and leave
> > +        * the extension_enabled[] setting to kvm_vcpu_sbi_find_ext().
> > +        */
> >         scontext->extension_disabled[sext->ext_idx] = !reg_val;
> >
> >         return 0;
> > @@ -317,7 +323,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> >                 ext = entry->ext_ptr;
> >
> >                 if (ext->extid_start <= extid && ext->extid_end >= extid) {
> > -                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
> > +                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX ||
> > +                           scontext->extension_enabled[entry->ext_idx])
> >                                 return ext;
> >                         if (scontext->extension_disabled[entry->ext_idx])
> >                                 return NULL;
> > @@ -325,6 +332,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> >                                 scontext->extension_disabled[entry->ext_idx] = true;
> >                                 return NULL;
> >                         }
> > +
> > +                       scontext->extension_enabled[entry->ext_idx] = true;
> >                         return ext;
> >                 }
> >         }
> > --
> > 2.39.2
> >


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] RISC-V: KVM: Introduce extension_enabled
@ 2023-05-22  9:28       ` Andrew Jones
  0 siblings, 0 replies; 20+ messages in thread
From: Andrew Jones @ 2023-05-22  9:28 UTC (permalink / raw)
  To: Anup Patel
  Cc: kvm-riscv, linux-riscv, Palmer Dabbelt, Atish Patra, Albert Ou,
	Paul Walmsley

On Fri, May 19, 2023 at 08:35:31PM +0530, Anup Patel wrote:
> On Wed, Apr 26, 2023 at 10:43 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > We need three SBI extension status values (uninitialized, enabled,
> > and disabled). Pairing another boolean array, extension_enabled[],
> > with the boolean array extension_disabled[] provides four states.
> > Using a pair of boolean arrays, which may eventually be changed to
> > a pair of bitmaps, is more space efficient than using one enum
> > status field. Apply the new (enabled=1,disabled=0) state, which
> > means either the extension doesn't have a probe function or that
> > its probe was successful, to avoid more than one probe of the
> > extension.
> >
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 +++++++++
> >  arch/riscv/kvm/vcpu_sbi.c             | 11 ++++++++++-
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > index 4278125a38a5..e3c5e1d15e93 100644
> > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > @@ -16,6 +16,15 @@
> >
> >  struct kvm_vcpu_sbi_context {
> >         int return_handled;
> > +       /*
> > +        * extension_enabled[] and extension_disabled[] provide SBI
> > +        * extensions four status values, of which we need three:
> > +        * (0,0) uninitialized, (1,0) enabled, (0,1) disabled. Using
> > +        * a pair of boolean arrays, which may eventually be changed
> > +        * to a pair of bitmaps, is more space efficient than using
> > +        * one enum status field.
> > +        */
> > +       bool extension_enabled[KVM_RISCV_SBI_EXT_MAX];
> 
> The extension_enabled[] is not an appropriate name.
> 
> How about extension_probe_allowed[] ?

"probe allowed" sounds to me like "we may call probe", but the meaning we
want is "we've already called probe and it succeeded, so the extension
is available" (or that we don't have probe, meaning the extension is
assumed available). I prefer 'available' for extensions over 'enabled',
but we already have 'disabled', so 'enabled' appeared to fit better.
Actually, it's probably best to improve the readability and simplicity,
at the expense of size, by dropping both the extension_enabled and
extension_disabled boolean arrays and switching to an 'extension_status'
enum, where we have SBI_EXT_UNINITIALIZED, SBI_EXT_AVAILABLE, and
SBI_EXT_UNAVAILABLE.

Thanks,
drew


> 
> Any better name ?
> 
> Regards,
> Anup
> 
> >         bool extension_disabled[KVM_RISCV_SBI_EXT_MAX];
> >  };
> >
> > diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> > index a1a82f0fbad2..344d38bbe06a 100644
> > --- a/arch/riscv/kvm/vcpu_sbi.c
> > +++ b/arch/riscv/kvm/vcpu_sbi.c
> > @@ -155,6 +155,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
> >         if (!sext)
> >                 return -ENOENT;
> >
> > +       /*
> > +        * We can't set scontext->extension_enabled[] to reg_val since the
> > +        * extension may have a probe() function which needs to confirm
> > +        * enablement first. Only set extension_disabled[] here and leave
> > +        * the extension_enabled[] setting to kvm_vcpu_sbi_find_ext().
> > +        */
> >         scontext->extension_disabled[sext->ext_idx] = !reg_val;
> >
> >         return 0;
> > @@ -317,7 +323,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> >                 ext = entry->ext_ptr;
> >
> >                 if (ext->extid_start <= extid && ext->extid_end >= extid) {
> > -                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
> > +                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX ||
> > +                           scontext->extension_enabled[entry->ext_idx])
> >                                 return ext;
> >                         if (scontext->extension_disabled[entry->ext_idx])
> >                                 return NULL;
> > @@ -325,6 +332,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> >                                 scontext->extension_disabled[entry->ext_idx] = true;
> >                                 return NULL;
> >                         }
> > +
> > +                       scontext->extension_enabled[entry->ext_idx] = true;
> >                         return ext;
> >                 }
> >         }
> > --
> > 2.39.2
> >

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread
* [PATCH 3/3] RISC-V: KVM: Introduce extension_enabled
  2023-05-22  9:28       ` Andrew Jones
@ 2023-05-25 15:58         ` Anup Patel
  -1 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-25 15:58 UTC (permalink / raw)
  To: kvm-riscv

On Mon, May 22, 2023 at 2:59?PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, May 19, 2023 at 08:35:31PM +0530, Anup Patel wrote:
> > On Wed, Apr 26, 2023 at 10:43?PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >
> > > We need three SBI extension status values (uninitialized, enabled,
> > > and disabled). Pairing another boolean array, extension_enabled[],
> > > with the boolean array extension_disabled[] provides four states.
> > > Using a pair of boolean arrays, which may eventually be changed to
> > > a pair of bitmaps, is more space efficient than using one enum
> > > status field. Apply the new (enabled=1,disabled=0) state, which
> > > means either the extension doesn't have a probe function or that
> > > its probe was successful, to avoid more than one probe of the
> > > extension.
> > >
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 +++++++++
> > >  arch/riscv/kvm/vcpu_sbi.c             | 11 ++++++++++-
> > >  2 files changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > index 4278125a38a5..e3c5e1d15e93 100644
> > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > @@ -16,6 +16,15 @@
> > >
> > >  struct kvm_vcpu_sbi_context {
> > >         int return_handled;
> > > +       /*
> > > +        * extension_enabled[] and extension_disabled[] provide SBI
> > > +        * extensions four status values, of which we need three:
> > > +        * (0,0) uninitialized, (1,0) enabled, (0,1) disabled. Using
> > > +        * a pair of boolean arrays, which may eventually be changed
> > > +        * to a pair of bitmaps, is more space efficient than using
> > > +        * one enum status field.
> > > +        */
> > > +       bool extension_enabled[KVM_RISCV_SBI_EXT_MAX];
> >
> > The extension_enabled[] is not an appropriate name.
> >
> > How about extension_probe_allowed[] ?
>
> "probe allowed" sounds to me like "we may call probe", but the meaning we
> want is "we've already called probe and it succeeded, so the extension
> is available" (or that we don't have probe, meaning the extension is
> assumed available). I prefer 'available' for extensions over 'enabled',
> but we already have 'disabled', so 'enabled' appeared to fit better.
> Actually, it's probably best to improve the readability and simplicity,
> at the expense of size, by dropping both the extension_enabled and
> extension_disabled boolean arrays and switching to an 'extension_status'
> enum, where we have SBI_EXT_UNINITIALIZED, SBI_EXT_AVAILABLE, and
> SBI_EXT_UNAVAILABLE.

I agree. Let's switch to enum.

Regards,
Anup

>
> Thanks,
> drew
>
>
> >
> > Any better name ?
> >
> > Regards,
> > Anup
> >
> > >         bool extension_disabled[KVM_RISCV_SBI_EXT_MAX];
> > >  };
> > >
> > > diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> > > index a1a82f0fbad2..344d38bbe06a 100644
> > > --- a/arch/riscv/kvm/vcpu_sbi.c
> > > +++ b/arch/riscv/kvm/vcpu_sbi.c
> > > @@ -155,6 +155,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
> > >         if (!sext)
> > >                 return -ENOENT;
> > >
> > > +       /*
> > > +        * We can't set scontext->extension_enabled[] to reg_val since the
> > > +        * extension may have a probe() function which needs to confirm
> > > +        * enablement first. Only set extension_disabled[] here and leave
> > > +        * the extension_enabled[] setting to kvm_vcpu_sbi_find_ext().
> > > +        */
> > >         scontext->extension_disabled[sext->ext_idx] = !reg_val;
> > >
> > >         return 0;
> > > @@ -317,7 +323,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> > >                 ext = entry->ext_ptr;
> > >
> > >                 if (ext->extid_start <= extid && ext->extid_end >= extid) {
> > > -                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
> > > +                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX ||
> > > +                           scontext->extension_enabled[entry->ext_idx])
> > >                                 return ext;
> > >                         if (scontext->extension_disabled[entry->ext_idx])
> > >                                 return NULL;
> > > @@ -325,6 +332,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> > >                                 scontext->extension_disabled[entry->ext_idx] = true;
> > >                                 return NULL;
> > >                         }
> > > +
> > > +                       scontext->extension_enabled[entry->ext_idx] = true;
> > >                         return ext;
> > >                 }
> > >         }
> > > --
> > > 2.39.2
> > >
>
> --
> kvm-riscv mailing list
> kvm-riscv at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [PATCH 3/3] RISC-V: KVM: Introduce extension_enabled
@ 2023-05-25 15:58         ` Anup Patel
  0 siblings, 0 replies; 20+ messages in thread
From: Anup Patel @ 2023-05-25 15:58 UTC (permalink / raw)
  To: Andrew Jones
  Cc: Anup Patel, kvm-riscv, linux-riscv, Palmer Dabbelt, Atish Patra,
	Albert Ou, Paul Walmsley

On Mon, May 22, 2023 at 2:59 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Fri, May 19, 2023 at 08:35:31PM +0530, Anup Patel wrote:
> > On Wed, Apr 26, 2023 at 10:43 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> > >
> > > We need three SBI extension status values (uninitialized, enabled,
> > > and disabled). Pairing another boolean array, extension_enabled[],
> > > with the boolean array extension_disabled[] provides four states.
> > > Using a pair of boolean arrays, which may eventually be changed to
> > > a pair of bitmaps, is more space efficient than using one enum
> > > status field. Apply the new (enabled=1,disabled=0) state, which
> > > means either the extension doesn't have a probe function or that
> > > its probe was successful, to avoid more than one probe of the
> > > extension.
> > >
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/kvm_vcpu_sbi.h |  9 +++++++++
> > >  arch/riscv/kvm/vcpu_sbi.c             | 11 ++++++++++-
> > >  2 files changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/include/asm/kvm_vcpu_sbi.h b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > index 4278125a38a5..e3c5e1d15e93 100644
> > > --- a/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > +++ b/arch/riscv/include/asm/kvm_vcpu_sbi.h
> > > @@ -16,6 +16,15 @@
> > >
> > >  struct kvm_vcpu_sbi_context {
> > >         int return_handled;
> > > +       /*
> > > +        * extension_enabled[] and extension_disabled[] provide SBI
> > > +        * extensions four status values, of which we need three:
> > > +        * (0,0) uninitialized, (1,0) enabled, (0,1) disabled. Using
> > > +        * a pair of boolean arrays, which may eventually be changed
> > > +        * to a pair of bitmaps, is more space efficient than using
> > > +        * one enum status field.
> > > +        */
> > > +       bool extension_enabled[KVM_RISCV_SBI_EXT_MAX];
> >
> > The extension_enabled[] is not an appropriate name.
> >
> > How about extension_probe_allowed[] ?
>
> "probe allowed" sounds to me like "we may call probe", but the meaning we
> want is "we've already called probe and it succeeded, so the extension
> is available" (or that we don't have probe, meaning the extension is
> assumed available). I prefer 'available' for extensions over 'enabled',
> but we already have 'disabled', so 'enabled' appeared to fit better.
> Actually, it's probably best to improve the readability and simplicity,
> at the expense of size, by dropping both the extension_enabled and
> extension_disabled boolean arrays and switching to an 'extension_status'
> enum, where we have SBI_EXT_UNINITIALIZED, SBI_EXT_AVAILABLE, and
> SBI_EXT_UNAVAILABLE.

I agree. Let's switch to enum.

Regards,
Anup

>
> Thanks,
> drew
>
>
> >
> > Any better name ?
> >
> > Regards,
> > Anup
> >
> > >         bool extension_disabled[KVM_RISCV_SBI_EXT_MAX];
> > >  };
> > >
> > > diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
> > > index a1a82f0fbad2..344d38bbe06a 100644
> > > --- a/arch/riscv/kvm/vcpu_sbi.c
> > > +++ b/arch/riscv/kvm/vcpu_sbi.c
> > > @@ -155,6 +155,12 @@ static int riscv_vcpu_set_sbi_ext_single(struct kvm_vcpu *vcpu,
> > >         if (!sext)
> > >                 return -ENOENT;
> > >
> > > +       /*
> > > +        * We can't set scontext->extension_enabled[] to reg_val since the
> > > +        * extension may have a probe() function which needs to confirm
> > > +        * enablement first. Only set extension_disabled[] here and leave
> > > +        * the extension_enabled[] setting to kvm_vcpu_sbi_find_ext().
> > > +        */
> > >         scontext->extension_disabled[sext->ext_idx] = !reg_val;
> > >
> > >         return 0;
> > > @@ -317,7 +323,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> > >                 ext = entry->ext_ptr;
> > >
> > >                 if (ext->extid_start <= extid && ext->extid_end >= extid) {
> > > -                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX)
> > > +                       if (entry->ext_idx >= KVM_RISCV_SBI_EXT_MAX ||
> > > +                           scontext->extension_enabled[entry->ext_idx])
> > >                                 return ext;
> > >                         if (scontext->extension_disabled[entry->ext_idx])
> > >                                 return NULL;
> > > @@ -325,6 +332,8 @@ const struct kvm_vcpu_sbi_extension *kvm_vcpu_sbi_find_ext(
> > >                                 scontext->extension_disabled[entry->ext_idx] = true;
> > >                                 return NULL;
> > >                         }
> > > +
> > > +                       scontext->extension_enabled[entry->ext_idx] = true;
> > >                         return ext;
> > >                 }
> > >         }
> > > --
> > > 2.39.2
> > >
>
> --
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 20+ messages in thread
end of thread, other threads:[~2023-05-25 15:58 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-26 17:13 [PATCH 0/3] RISC-V: KVM: Ensure SBI extension is enabled Andrew Jones
2023-04-26 17:13 ` Andrew Jones
2023-04-26 17:13 ` [PATCH 1/3] RISC-V: KVM: Disable SBI extension when its probe fails Andrew Jones
2023-04-26 17:13   ` Andrew Jones
2023-05-19 14:57   ` Anup Patel
2023-05-19 14:57     ` Anup Patel
2023-05-19 15:02     ` Anup Patel
2023-05-19 15:02       ` Anup Patel
2023-04-26 17:13 ` [PATCH 2/3] RISC-V: KVM: Rename dis_idx to ext_idx Andrew Jones
2023-04-26 17:13   ` Andrew Jones
2023-05-19 14:58   ` Anup Patel
2023-05-19 14:58     ` Anup Patel
2023-04-26 17:13 ` [PATCH 3/3] RISC-V: KVM: Introduce extension_enabled Andrew Jones
2023-04-26 17:13   ` Andrew Jones
2023-05-19 15:05   ` Anup Patel
2023-05-19 15:05     ` Anup Patel
2023-05-22  9:28     ` Andrew Jones
2023-05-22  9:28       ` Andrew Jones
2023-05-25 15:58       ` Anup Patel
2023-05-25 15:58         ` Anup Patel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.