[PATCH 0/5] KVM: arm64: Miscellaneous improvements

[PATCH 0/5] KVM: arm64: Miscellaneous improvements

[Alexandru Elisei]

The documentation update in the first patch was suggested by Marc [1]. When
I was going through the code to track down all the places error codes were
coming from I noticed a few things that in my opinion could be improved.
The following patches aim to do just that. I'm fine dropping them if the
churn looks unjustified.

Tested the Documentation changes by building pdfdocs, didn't notice any
warnings regarding api.rst.

Tested the other patches on a rockpro64 on the little cores. I ran
kvm-unit-tests with qemu and kvmtool. I also ran a Linux guest with qemu
and ran perf:

$ perf record -a -- iperf3 -c 127.0.0.1 -t 60

I checked that interrupts were firing and nothing looked out of the
ordinary. I used qemu because qemu VCPUs do initialization concurrently
from their own thread, not from the main thread like kvmtool.

To check that kvm_timer_enable() is never reached if the VGIC is not
initialized, I hacked kvmtool to remove the ioctl
KVM_DEV_ARM_VGIC_GRP_CTRL(KVM_DEV_ARM_VGIC_CTRL_INIT) from gic__init_gic().
When trying to run a guest, I got the following error message:

KVM_RUN failed: Device or resource busy

which is consistent with the EBUSY return code from
vgic_v3_map_resources(). Double checked that that's where the code is
coming from by adding a pr_info statement to
kvm_arch_vcpu_first_run_init().

[1] https://www.spinics.net/lists/arm-kernel/msg858024.html

Alexandru Elisei (5):
  KVM: Documentation: Add arm64 KVM_RUN error codes
  KVM: arm64: arch_timer: Remove VGIC initialization check
  KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()
  KVM: arm64: Update comment in kvm_vgic_map_resources()
  KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

 Documentation/virt/kvm/api.rst  | 9 +++++++--
 arch/arm64/kvm/arch_timer.c     | 3 ---
 arch/arm64/kvm/arm.c            | 8 +++-----
 arch/arm64/kvm/pmu-emul.c       | 2 --
 arch/arm64/kvm/vgic/vgic-init.c | 9 ++++++++-
 arch/arm64/kvm/vgic/vgic-v2.c   | 3 ---
 arch/arm64/kvm/vgic/vgic-v3.c   | 3 ---
 7 files changed, 18 insertions(+), 19 deletions(-)

-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 0/5] KVM: arm64: Miscellaneous improvements

[Alexandru Elisei]

The documentation update in the first patch was suggested by Marc [1]. When
I was going through the code to track down all the places error codes were
coming from I noticed a few things that in my opinion could be improved.
The following patches aim to do just that. I'm fine dropping them if the
churn looks unjustified.

Tested the Documentation changes by building pdfdocs, didn't notice any
warnings regarding api.rst.

Tested the other patches on a rockpro64 on the little cores. I ran
kvm-unit-tests with qemu and kvmtool. I also ran a Linux guest with qemu
and ran perf:

$ perf record -a -- iperf3 -c 127.0.0.1 -t 60

I checked that interrupts were firing and nothing looked out of the
ordinary. I used qemu because qemu VCPUs do initialization concurrently
from their own thread, not from the main thread like kvmtool.

To check that kvm_timer_enable() is never reached if the VGIC is not
initialized, I hacked kvmtool to remove the ioctl
KVM_DEV_ARM_VGIC_GRP_CTRL(KVM_DEV_ARM_VGIC_CTRL_INIT) from gic__init_gic().
When trying to run a guest, I got the following error message:

KVM_RUN failed: Device or resource busy

which is consistent with the EBUSY return code from
vgic_v3_map_resources(). Double checked that that's where the code is
coming from by adding a pr_info statement to
kvm_arch_vcpu_first_run_init().

[1] https://www.spinics.net/lists/arm-kernel/msg858024.html

Alexandru Elisei (5):
  KVM: Documentation: Add arm64 KVM_RUN error codes
  KVM: arm64: arch_timer: Remove VGIC initialization check
  KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()
  KVM: arm64: Update comment in kvm_vgic_map_resources()
  KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

 Documentation/virt/kvm/api.rst  | 9 +++++++--
 arch/arm64/kvm/arch_timer.c     | 3 ---
 arch/arm64/kvm/arm.c            | 8 +++-----
 arch/arm64/kvm/pmu-emul.c       | 2 --
 arch/arm64/kvm/vgic/vgic-init.c | 9 ++++++++-
 arch/arm64/kvm/vgic/vgic-v2.c   | 3 ---
 arch/arm64/kvm/vgic/vgic-v3.c   | 3 ---
 7 files changed, 18 insertions(+), 19 deletions(-)

-- 
2.29.2


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

[PATCH 1/5] KVM: Documentation: Add arm64 KVM_RUN error codes

[Alexandru Elisei]

The API documentation states that general error codes are not detailed, but
errors with specific meanings are. On arm64, KVM_RUN can return error
numbers with a different meaning than what is described by POSIX or the C99
standard (as taken from man 3 errno).

Absent from the newly documented error codes is ERANGE which can be
returned when making a change to the EL2 stage 1 tables if the address is
larger than the largest supported input address. Assuming no bugs in the
implementation, that is not possible because the input addresses which are
mapped are the result of applying the macro kern_hyp_va() on kernel virtual
addresses.

CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 Documentation/virt/kvm/api.rst | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 36d5f1f3c6dd..090a2331b1f2 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -380,9 +380,14 @@ This ioctl is obsolete and has been removed.
 
 Errors:
 
-  =====      =============================
+  =======    ==============================================================
   EINTR      an unmasked signal is pending
-  =====      =============================
+  ENOEXEC    the vcpu hasn't been initialized or the guest tried to execute
+             instructions from device memory (arm64)
+  ENOSYS     data abort outside memslots with no syndrome info and
+             KVM_CAP_ARM_NISV_TO_USER not enabled (arm64)
+  EPERM      SVE feature set but not finalized (arm64)
+  =======    ==============================================================
 
 This ioctl is used to run a guest virtual cpu.  While there are no
 explicit parameters, there is an implicit parameter block that can be
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/5] KVM: Documentation: Add arm64 KVM_RUN error codes

[Alexandru Elisei]

The API documentation states that general error codes are not detailed, but
errors with specific meanings are. On arm64, KVM_RUN can return error
numbers with a different meaning than what is described by POSIX or the C99
standard (as taken from man 3 errno).

Absent from the newly documented error codes is ERANGE which can be
returned when making a change to the EL2 stage 1 tables if the address is
larger than the largest supported input address. Assuming no bugs in the
implementation, that is not possible because the input addresses which are
mapped are the result of applying the macro kern_hyp_va() on kernel virtual
addresses.

CC: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 Documentation/virt/kvm/api.rst | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 36d5f1f3c6dd..090a2331b1f2 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -380,9 +380,14 @@ This ioctl is obsolete and has been removed.
 
 Errors:
 
-  =====      =============================
+  =======    ==============================================================
   EINTR      an unmasked signal is pending
-  =====      =============================
+  ENOEXEC    the vcpu hasn't been initialized or the guest tried to execute
+             instructions from device memory (arm64)
+  ENOSYS     data abort outside memslots with no syndrome info and
+             KVM_CAP_ARM_NISV_TO_USER not enabled (arm64)
+  EPERM      SVE feature set but not finalized (arm64)
+  =======    ==============================================================
 
 This ioctl is used to run a guest virtual cpu.  While there are no
 explicit parameters, there is an implicit parameter block that can be
-- 
2.29.2


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

[PATCH 2/5] KVM: arm64: arch_timer: Remove VGIC initialization check

[Alexandru Elisei]

kvm_timer_enable() is called in kvm_vcpu_first_run_init() after
kvm_vgic_map_resources() if the VGIC wasn't ready. kvm_vgic_map_resources()
is the only place where kvm->arch.vgic.ready is set to true.

For a v2 VGIC, kvm_vgic_map_resources() will attempt to initialize the VGIC
and set the initialized flag.

For a v3 VGIC, kvm_vgic_map_resources() will return an error code if the
VGIC isn't already initialized.

The end result is that if we've reached kvm_timer_enable(), the VGIC is
initialzed and ready and vgic_initialized() will always be true, so remove
this check.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/arch_timer.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 32ba6fbc3814..2e0ca8bcc6ff 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -1129,9 +1129,6 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	if (!irqchip_in_kernel(vcpu->kvm))
 		goto no_vgic;
 
-	if (!vgic_initialized(vcpu->kvm))
-		return -ENODEV;
-
 	if (!timer_irqs_are_valid(vcpu)) {
 		kvm_debug("incorrectly configured timer irqs\n");
 		return -EINVAL;
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/5] KVM: arm64: arch_timer: Remove VGIC initialization check

[Alexandru Elisei]

kvm_timer_enable() is called in kvm_vcpu_first_run_init() after
kvm_vgic_map_resources() if the VGIC wasn't ready. kvm_vgic_map_resources()
is the only place where kvm->arch.vgic.ready is set to true.

For a v2 VGIC, kvm_vgic_map_resources() will attempt to initialize the VGIC
and set the initialized flag.

For a v3 VGIC, kvm_vgic_map_resources() will return an error code if the
VGIC isn't already initialized.

The end result is that if we've reached kvm_timer_enable(), the VGIC is
initialzed and ready and vgic_initialized() will always be true, so remove
this check.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/arch_timer.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 32ba6fbc3814..2e0ca8bcc6ff 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -1129,9 +1129,6 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	if (!irqchip_in_kernel(vcpu->kvm))
 		goto no_vgic;
 
-	if (!vgic_initialized(vcpu->kvm))
-		return -ENODEV;
-
 	if (!timer_irqs_are_valid(vcpu)) {
 		kvm_debug("incorrectly configured timer irqs\n");
 		return -EINVAL;
-- 
2.29.2


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

[PATCH 3/5] KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()

[Alexandru Elisei]

kvm_vgic_map_resources() is called when a VCPU if first run and it maps all
the VGIC MMIO regions. To prevent double-initialization, the VGIC uses the
ready variable to keep track of the state of resources and the global KVM
mutex to protect against concurrent accesses. After the lock is taken, the
variable is checked again in case another VCPU took the lock between the
current VCPU reading ready equals false and taking the lock.

The double-checked lock pattern is spread across four different functions:
in kvm_vcpu_first_run_init(), in kvm_vgic_map_resource() and in
vgic_{v2,v3}_map_resources(), which makes it hard to reason about and
introduces minor code duplication. Consolidate the checks in
kvm_vgic_map_resources(), where the lock is taken.

No functional change intended.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/arm.c            | 8 +++-----
 arch/arm64/kvm/vgic/vgic-init.c | 6 ++++++
 arch/arm64/kvm/vgic/vgic-v2.c   | 3 ---
 arch/arm64/kvm/vgic/vgic-v3.c   | 3 ---
 4 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 9d69d2bf6943..65a5e89f5133 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -530,11 +530,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 		 * Map the VGIC hardware resources before running a vcpu the
 		 * first time on this VM.
 		 */
-		if (unlikely(!vgic_ready(kvm))) {
-			ret = kvm_vgic_map_resources(kvm);
-			if (ret)
-				return ret;
-		}
+		ret = kvm_vgic_map_resources(kvm);
+		if (ret)
+			return ret;
 	} else {
 		/*
 		 * Tell the rest of the code that there are userspace irqchip
diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 32e32d67a127..a2f4d1c85f00 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -428,7 +428,13 @@ int kvm_vgic_map_resources(struct kvm *kvm)
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	int ret = 0;
 
+	if (likely(vgic_ready(kvm)))
+		return 0;
+
 	mutex_lock(&kvm->lock);
+	if (vgic_ready(kvm))
+		goto out;
+
 	if (!irqchip_in_kernel(kvm))
 		goto out;
 
diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index ebf53a4e1296..7f38c1a93639 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c
@@ -306,9 +306,6 @@ int vgic_v2_map_resources(struct kvm *kvm)
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	int ret = 0;
 
-	if (vgic_ready(kvm))
-		goto out;
-
 	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) ||
 	    IS_VGIC_ADDR_UNDEF(dist->vgic_cpu_base)) {
 		kvm_err("Need to set vgic cpu and dist addresses first\n");
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 9cdf39a94a63..35029c5cb0f1 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -500,9 +500,6 @@ int vgic_v3_map_resources(struct kvm *kvm)
 	int ret = 0;
 	int c;
 
-	if (vgic_ready(kvm))
-		goto out;
-
 	kvm_for_each_vcpu(c, vcpu, kvm) {
 		struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 3/5] KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()

[Alexandru Elisei]

kvm_vgic_map_resources() is called when a VCPU if first run and it maps all
the VGIC MMIO regions. To prevent double-initialization, the VGIC uses the
ready variable to keep track of the state of resources and the global KVM
mutex to protect against concurrent accesses. After the lock is taken, the
variable is checked again in case another VCPU took the lock between the
current VCPU reading ready equals false and taking the lock.

The double-checked lock pattern is spread across four different functions:
in kvm_vcpu_first_run_init(), in kvm_vgic_map_resource() and in
vgic_{v2,v3}_map_resources(), which makes it hard to reason about and
introduces minor code duplication. Consolidate the checks in
kvm_vgic_map_resources(), where the lock is taken.

No functional change intended.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/arm.c            | 8 +++-----
 arch/arm64/kvm/vgic/vgic-init.c | 6 ++++++
 arch/arm64/kvm/vgic/vgic-v2.c   | 3 ---
 arch/arm64/kvm/vgic/vgic-v3.c   | 3 ---
 4 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 9d69d2bf6943..65a5e89f5133 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -530,11 +530,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
 		 * Map the VGIC hardware resources before running a vcpu the
 		 * first time on this VM.
 		 */
-		if (unlikely(!vgic_ready(kvm))) {
-			ret = kvm_vgic_map_resources(kvm);
-			if (ret)
-				return ret;
-		}
+		ret = kvm_vgic_map_resources(kvm);
+		if (ret)
+			return ret;
 	} else {
 		/*
 		 * Tell the rest of the code that there are userspace irqchip
diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index 32e32d67a127..a2f4d1c85f00 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -428,7 +428,13 @@ int kvm_vgic_map_resources(struct kvm *kvm)
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	int ret = 0;
 
+	if (likely(vgic_ready(kvm)))
+		return 0;
+
 	mutex_lock(&kvm->lock);
+	if (vgic_ready(kvm))
+		goto out;
+
 	if (!irqchip_in_kernel(kvm))
 		goto out;
 
diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index ebf53a4e1296..7f38c1a93639 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c
@@ -306,9 +306,6 @@ int vgic_v2_map_resources(struct kvm *kvm)
 	struct vgic_dist *dist = &kvm->arch.vgic;
 	int ret = 0;
 
-	if (vgic_ready(kvm))
-		goto out;
-
 	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) ||
 	    IS_VGIC_ADDR_UNDEF(dist->vgic_cpu_base)) {
 		kvm_err("Need to set vgic cpu and dist addresses first\n");
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 9cdf39a94a63..35029c5cb0f1 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -500,9 +500,6 @@ int vgic_v3_map_resources(struct kvm *kvm)
 	int ret = 0;
 	int c;
 
-	if (vgic_ready(kvm))
-		goto out;
-
 	kvm_for_each_vcpu(c, vcpu, kvm) {
 		struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 
-- 
2.29.2


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

[PATCH 4/5] KVM: arm64: Update comment in kvm_vgic_map_resources()

[Alexandru Elisei]

vgic_v3_map_resources() returns -EBUSY if the VGIC isn't initialized,
update the comment to kvm_vgic_map_resources() to match what the function
does.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/vgic/vgic-init.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index a2f4d1c85f00..8942b2191bcf 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -419,7 +419,8 @@ int vgic_lazy_init(struct kvm *kvm)
  * Map the MMIO regions depending on the VGIC model exposed to the guest
  * called on the first VCPU run.
  * Also map the virtual CPU interface into the VM.
- * v2/v3 derivatives call vgic_init if not already done.
+ * v2 calls vgic_init if not already done.
+ * v3 and derivatives return an error if the VGIC is not initialized.
  * vgic_ready() returns true if this function has succeeded.
  * @kvm: kvm struct pointer
  */
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 4/5] KVM: arm64: Update comment in kvm_vgic_map_resources()

[Alexandru Elisei]

vgic_v3_map_resources() returns -EBUSY if the VGIC isn't initialized,
update the comment to kvm_vgic_map_resources() to match what the function
does.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/vgic/vgic-init.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
index a2f4d1c85f00..8942b2191bcf 100644
--- a/arch/arm64/kvm/vgic/vgic-init.c
+++ b/arch/arm64/kvm/vgic/vgic-init.c
@@ -419,7 +419,8 @@ int vgic_lazy_init(struct kvm *kvm)
  * Map the MMIO regions depending on the VGIC model exposed to the guest
  * called on the first VCPU run.
  * Also map the virtual CPU interface into the VM.
- * v2/v3 derivatives call vgic_init if not already done.
+ * v2 calls vgic_init if not already done.
+ * v3 and derivatives return an error if the VGIC is not initialized.
  * vgic_ready() returns true if this function has succeeded.
  * @kvm: kvm struct pointer
  */
-- 
2.29.2


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

[PATCH 5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

[Alexandru Elisei]

KVM_ARM_VCPU_INIT ioctl calls kvm_reset_vcpu(), which in turn resets the
PMU with a call to kvm_pmu_vcpu_reset(). The function zeroes the PMU
chained counters bitmap and stops all the counters with a perf event
attached. Because it is called before the VCPU has had the chance to run,
no perf events are in use and none are released.

kvm_arm_pmu_v3_enable(), called by kvm_vcpu_first_run_init() only if the
VCPU has not been initialized, also resets the PMU. kvm_pmu_vcpu_reset() in
this case does the exact same thing as the previous call, so remove it.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/pmu-emul.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 398f6df1bbe4..4ad66a532e38 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -850,8 +850,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 		   return -EINVAL;
 	}
 
-	kvm_pmu_vcpu_reset(vcpu);
-
 	return 0;
 }
 
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

[Alexandru Elisei]

KVM_ARM_VCPU_INIT ioctl calls kvm_reset_vcpu(), which in turn resets the
PMU with a call to kvm_pmu_vcpu_reset(). The function zeroes the PMU
chained counters bitmap and stops all the counters with a perf event
attached. Because it is called before the VCPU has had the chance to run,
no perf events are in use and none are released.

kvm_arm_pmu_v3_enable(), called by kvm_vcpu_first_run_init() only if the
VCPU has not been initialized, also resets the PMU. kvm_pmu_vcpu_reset() in
this case does the exact same thing as the previous call, so remove it.

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arch/arm64/kvm/pmu-emul.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 398f6df1bbe4..4ad66a532e38 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -850,8 +850,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 		   return -EINVAL;
 	}
 
-	kvm_pmu_vcpu_reset(vcpu);
-
 	return 0;
 }
 
-- 
2.29.2


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

Re: [PATCH 2/5] KVM: arm64: arch_timer: Remove VGIC initialization check

[Auger Eric]

Hi Alexandru,

On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> kvm_timer_enable() is called in kvm_vcpu_first_run_init() after
> kvm_vgic_map_resources() if the VGIC wasn't ready. kvm_vgic_map_resources()
> is the only place where kvm->arch.vgic.ready is set to true.
> 
> For a v2 VGIC, kvm_vgic_map_resources() will attempt to initialize the VGIC
> and set the initialized flag.
> 
> For a v3 VGIC, kvm_vgic_map_resources() will return an error code if the
> VGIC isn't already initialized.
> 
> The end result is that if we've reached kvm_timer_enable(), the VGIC is
> initialzed and ready and vgic_initialized() will always be true, so remove
> this check.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/arch_timer.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 32ba6fbc3814..2e0ca8bcc6ff 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -1129,9 +1129,6 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>  	if (!irqchip_in_kernel(vcpu->kvm))
>  		goto no_vgic;
>  
> -	if (!vgic_initialized(vcpu->kvm))
> -		return -ENODEV;
> -
>  	if (!timer_irqs_are_valid(vcpu)) {
>  		kvm_debug("incorrectly configured timer irqs\n");
>  		return -EINVAL;
>
The only concern I have is if some future contributor forgets the
constraint. Maybe add a comment (as bug on is frown upon by scripts).

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 2/5] KVM: arm64: arch_timer: Remove VGIC initialization check

[Auger Eric]

Hi Alexandru,

On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> kvm_timer_enable() is called in kvm_vcpu_first_run_init() after
> kvm_vgic_map_resources() if the VGIC wasn't ready. kvm_vgic_map_resources()
> is the only place where kvm->arch.vgic.ready is set to true.
> 
> For a v2 VGIC, kvm_vgic_map_resources() will attempt to initialize the VGIC
> and set the initialized flag.
> 
> For a v3 VGIC, kvm_vgic_map_resources() will return an error code if the
> VGIC isn't already initialized.
> 
> The end result is that if we've reached kvm_timer_enable(), the VGIC is
> initialzed and ready and vgic_initialized() will always be true, so remove
> this check.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/arch_timer.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 32ba6fbc3814..2e0ca8bcc6ff 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -1129,9 +1129,6 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>  	if (!irqchip_in_kernel(vcpu->kvm))
>  		goto no_vgic;
>  
> -	if (!vgic_initialized(vcpu->kvm))
> -		return -ENODEV;
> -
>  	if (!timer_irqs_are_valid(vcpu)) {
>  		kvm_debug("incorrectly configured timer irqs\n");
>  		return -EINVAL;
>
The only concern I have is if some future contributor forgets the
constraint. Maybe add a comment (as bug on is frown upon by scripts).

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric



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

Re: [PATCH 3/5] KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()

[Auger Eric]

Hi Alexandru,

On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> kvm_vgic_map_resources() is called when a VCPU if first run and it maps all
> the VGIC MMIO regions. To prevent double-initialization, the VGIC uses the
> ready variable to keep track of the state of resources and the global KVM
> mutex to protect against concurrent accesses. After the lock is taken, the
> variable is checked again in case another VCPU took the lock between the
> current VCPU reading ready equals false and taking the lock.
> 
> The double-checked lock pattern is spread across four different functions:
> in kvm_vcpu_first_run_init(), in kvm_vgic_map_resource() and in
> vgic_{v2,v3}_map_resources(), which makes it hard to reason about and
> introduces minor code duplication. Consolidate the checks in
> kvm_vgic_map_resources(), where the lock is taken.
> 
> No functional change intended.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/arm.c            | 8 +++-----
>  arch/arm64/kvm/vgic/vgic-init.c | 6 ++++++
>  arch/arm64/kvm/vgic/vgic-v2.c   | 3 ---
>  arch/arm64/kvm/vgic/vgic-v3.c   | 3 ---
>  4 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 9d69d2bf6943..65a5e89f5133 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -530,11 +530,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  		 * Map the VGIC hardware resources before running a vcpu the
>  		 * first time on this VM.
>  		 */
> -		if (unlikely(!vgic_ready(kvm))) {
> -			ret = kvm_vgic_map_resources(kvm);
> -			if (ret)
> -				return ret;
> -		}
> +		ret = kvm_vgic_map_resources(kvm);
> +		if (ret)
> +			return ret;
>  	} else {
>  		/*
>  		 * Tell the rest of the code that there are userspace irqchip
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 32e32d67a127..a2f4d1c85f00 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -428,7 +428,13 @@ int kvm_vgic_map_resources(struct kvm *kvm)
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	int ret = 0;
>  
> +	if (likely(vgic_ready(kvm)))
> +		return 0;
> +
>  	mutex_lock(&kvm->lock);
> +	if (vgic_ready(kvm))
> +		goto out;
> +
While we are at it, the setting of dist->ready may be moved in
kvm_vgic_map_resources() too.

Thanks

Eric
>  	if (!irqchip_in_kernel(kvm))
>  		goto out;
>  
> diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
> index ebf53a4e1296..7f38c1a93639 100644
> --- a/arch/arm64/kvm/vgic/vgic-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-v2.c
> @@ -306,9 +306,6 @@ int vgic_v2_map_resources(struct kvm *kvm)
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	int ret = 0;
>  
> -	if (vgic_ready(kvm))
> -		goto out;
> -
>  	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) ||
>  	    IS_VGIC_ADDR_UNDEF(dist->vgic_cpu_base)) {
>  		kvm_err("Need to set vgic cpu and dist addresses first\n");
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 9cdf39a94a63..35029c5cb0f1 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -500,9 +500,6 @@ int vgic_v3_map_resources(struct kvm *kvm)
>  	int ret = 0;
>  	int c;
>  
> -	if (vgic_ready(kvm))
> -		goto out;
> -
>  	kvm_for_each_vcpu(c, vcpu, kvm) {
>  		struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/5] KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()

[Auger Eric]

Hi Alexandru,

On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> kvm_vgic_map_resources() is called when a VCPU if first run and it maps all
> the VGIC MMIO regions. To prevent double-initialization, the VGIC uses the
> ready variable to keep track of the state of resources and the global KVM
> mutex to protect against concurrent accesses. After the lock is taken, the
> variable is checked again in case another VCPU took the lock between the
> current VCPU reading ready equals false and taking the lock.
> 
> The double-checked lock pattern is spread across four different functions:
> in kvm_vcpu_first_run_init(), in kvm_vgic_map_resource() and in
> vgic_{v2,v3}_map_resources(), which makes it hard to reason about and
> introduces minor code duplication. Consolidate the checks in
> kvm_vgic_map_resources(), where the lock is taken.
> 
> No functional change intended.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/arm.c            | 8 +++-----
>  arch/arm64/kvm/vgic/vgic-init.c | 6 ++++++
>  arch/arm64/kvm/vgic/vgic-v2.c   | 3 ---
>  arch/arm64/kvm/vgic/vgic-v3.c   | 3 ---
>  4 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 9d69d2bf6943..65a5e89f5133 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -530,11 +530,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
>  		 * Map the VGIC hardware resources before running a vcpu the
>  		 * first time on this VM.
>  		 */
> -		if (unlikely(!vgic_ready(kvm))) {
> -			ret = kvm_vgic_map_resources(kvm);
> -			if (ret)
> -				return ret;
> -		}
> +		ret = kvm_vgic_map_resources(kvm);
> +		if (ret)
> +			return ret;
>  	} else {
>  		/*
>  		 * Tell the rest of the code that there are userspace irqchip
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index 32e32d67a127..a2f4d1c85f00 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -428,7 +428,13 @@ int kvm_vgic_map_resources(struct kvm *kvm)
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	int ret = 0;
>  
> +	if (likely(vgic_ready(kvm)))
> +		return 0;
> +
>  	mutex_lock(&kvm->lock);
> +	if (vgic_ready(kvm))
> +		goto out;
> +
While we are at it, the setting of dist->ready may be moved in
kvm_vgic_map_resources() too.

Thanks

Eric
>  	if (!irqchip_in_kernel(kvm))
>  		goto out;
>  
> diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
> index ebf53a4e1296..7f38c1a93639 100644
> --- a/arch/arm64/kvm/vgic/vgic-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-v2.c
> @@ -306,9 +306,6 @@ int vgic_v2_map_resources(struct kvm *kvm)
>  	struct vgic_dist *dist = &kvm->arch.vgic;
>  	int ret = 0;
>  
> -	if (vgic_ready(kvm))
> -		goto out;
> -
>  	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) ||
>  	    IS_VGIC_ADDR_UNDEF(dist->vgic_cpu_base)) {
>  		kvm_err("Need to set vgic cpu and dist addresses first\n");
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 9cdf39a94a63..35029c5cb0f1 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -500,9 +500,6 @@ int vgic_v3_map_resources(struct kvm *kvm)
>  	int ret = 0;
>  	int c;
>  
> -	if (vgic_ready(kvm))
> -		goto out;
> -
>  	kvm_for_each_vcpu(c, vcpu, kvm) {
>  		struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>  
> 


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

Re: [PATCH 4/5] KVM: arm64: Update comment in kvm_vgic_map_resources()

[Auger Eric]

Hi Alexandru,

On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> vgic_v3_map_resources() returns -EBUSY if the VGIC isn't initialized,
> update the comment to kvm_vgic_map_resources() to match what the function
> does.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index a2f4d1c85f00..8942b2191bcf 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -419,7 +419,8 @@ int vgic_lazy_init(struct kvm *kvm)
>   * Map the MMIO regions depending on the VGIC model exposed to the guest
>   * called on the first VCPU run.
>   * Also map the virtual CPU interface into the VM.
> - * v2/v3 derivatives call vgic_init if not already done.
> + * v2 calls vgic_init if not already done.
nit: s/vgic_init/vgic_init()?
> + * v3 and derivatives return an error if the VGIC is not initialized.
>   * vgic_ready() returns true if this function has succeeded.
>   * @kvm: kvm struct pointer
>   */
> 
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 4/5] KVM: arm64: Update comment in kvm_vgic_map_resources()

[Auger Eric]

Hi Alexandru,

On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> vgic_v3_map_resources() returns -EBUSY if the VGIC isn't initialized,
> update the comment to kvm_vgic_map_resources() to match what the function
> does.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/vgic/vgic-init.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index a2f4d1c85f00..8942b2191bcf 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -419,7 +419,8 @@ int vgic_lazy_init(struct kvm *kvm)
>   * Map the MMIO regions depending on the VGIC model exposed to the guest
>   * called on the first VCPU run.
>   * Also map the virtual CPU interface into the VM.
> - * v2/v3 derivatives call vgic_init if not already done.
> + * v2 calls vgic_init if not already done.
nit: s/vgic_init/vgic_init()?
> + * v3 and derivatives return an error if the VGIC is not initialized.
>   * vgic_ready() returns true if this function has succeeded.
>   * @kvm: kvm struct pointer
>   */
> 
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric


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

Re: [PATCH 5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

[Auger Eric]

Alexandru,

On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> KVM_ARM_VCPU_INIT ioctl calls kvm_reset_vcpu(), which in turn resets the
> PMU with a call to kvm_pmu_vcpu_reset(). The function zeroes the PMU
> chained counters bitmap and stops all the counters with a perf event
> attached. Because it is called before the VCPU has had the chance to run,
> no perf events are in use and none are released.
> 
> kvm_arm_pmu_v3_enable(), called by kvm_vcpu_first_run_init() only if the
> VCPU has not been initialized, also resets the PMU. kvm_pmu_vcpu_reset() in
This sounds strange to me. kvm_vcpu_first_run_init() only is called if
kvm_vcpu_initialized(vcpu) is true.
> this case does the exact same thing as the previous call, so remove it.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/pmu-emul.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 398f6df1bbe4..4ad66a532e38 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -850,8 +850,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  		   return -EINVAL;
>  	}
>  
> -	kvm_pmu_vcpu_reset(vcpu);
> -
this patch does not apply for me (vcpu->arch.pmu.ready = true; ?)

Thanks

Eric
>  	return 0;
>  }
>  
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

[Auger Eric]

Alexandru,

On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> KVM_ARM_VCPU_INIT ioctl calls kvm_reset_vcpu(), which in turn resets the
> PMU with a call to kvm_pmu_vcpu_reset(). The function zeroes the PMU
> chained counters bitmap and stops all the counters with a perf event
> attached. Because it is called before the VCPU has had the chance to run,
> no perf events are in use and none are released.
> 
> kvm_arm_pmu_v3_enable(), called by kvm_vcpu_first_run_init() only if the
> VCPU has not been initialized, also resets the PMU. kvm_pmu_vcpu_reset() in
This sounds strange to me. kvm_vcpu_first_run_init() only is called if
kvm_vcpu_initialized(vcpu) is true.
> this case does the exact same thing as the previous call, so remove it.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arch/arm64/kvm/pmu-emul.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 398f6df1bbe4..4ad66a532e38 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -850,8 +850,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  		   return -EINVAL;
>  	}
>  
> -	kvm_pmu_vcpu_reset(vcpu);
> -
this patch does not apply for me (vcpu->arch.pmu.ready = true; ?)

Thanks

Eric
>  	return 0;
>  }
>  
> 


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

Re: [PATCH 5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

[Alexandru Elisei]

Hi Eric,

Thanks for having a look!

On 12/14/20 1:48 PM, Auger Eric wrote:
> Alexandru,
>
> On 12/1/20 4:01 PM, Alexandru Elisei wrote:
>> KVM_ARM_VCPU_INIT ioctl calls kvm_reset_vcpu(), which in turn resets the
>> PMU with a call to kvm_pmu_vcpu_reset(). The function zeroes the PMU
>> chained counters bitmap and stops all the counters with a perf event
>> attached. Because it is called before the VCPU has had the chance to run,
>> no perf events are in use and none are released.
>>
>> kvm_arm_pmu_v3_enable(), called by kvm_vcpu_first_run_init() only if the
>> VCPU has not been initialized, also resets the PMU. kvm_pmu_vcpu_reset() in
> This sounds strange to me. kvm_vcpu_first_run_init() only is called if
> kvm_vcpu_initialized(vcpu) is true.

Typo on my part, the "not" should not be there. kvm_vcpu_first_run_init() is
called only if kvm_vcpu_initialized() returns true in kvm_arch_vcpu_ioctl_run().

>> this case does the exact same thing as the previous call, so remove it.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arch/arm64/kvm/pmu-emul.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index 398f6df1bbe4..4ad66a532e38 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -850,8 +850,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>  		   return -EINVAL;
>>  	}
>>  
>> -	kvm_pmu_vcpu_reset(vcpu);
>> -
> this patch does not apply for me (vcpu->arch.pmu.ready = true; ?)

I should have mentioned it in the cover letter, this is on top of Marc's pmu-undef
branch:

https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-undef

it should work on top of commit 7521c3a9e630 ("KVM: arm64: Get ready of the PMU
ready state"), the branch was rebased since I sent the patches.

Thanks,
Alex
>
> Thanks
>
> Eric
>>  	return 0;
>>  }
>>  
>>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

[Alexandru Elisei]

Hi Eric,

Thanks for having a look!

On 12/14/20 1:48 PM, Auger Eric wrote:
> Alexandru,
>
> On 12/1/20 4:01 PM, Alexandru Elisei wrote:
>> KVM_ARM_VCPU_INIT ioctl calls kvm_reset_vcpu(), which in turn resets the
>> PMU with a call to kvm_pmu_vcpu_reset(). The function zeroes the PMU
>> chained counters bitmap and stops all the counters with a perf event
>> attached. Because it is called before the VCPU has had the chance to run,
>> no perf events are in use and none are released.
>>
>> kvm_arm_pmu_v3_enable(), called by kvm_vcpu_first_run_init() only if the
>> VCPU has not been initialized, also resets the PMU. kvm_pmu_vcpu_reset() in
> This sounds strange to me. kvm_vcpu_first_run_init() only is called if
> kvm_vcpu_initialized(vcpu) is true.

Typo on my part, the "not" should not be there. kvm_vcpu_first_run_init() is
called only if kvm_vcpu_initialized() returns true in kvm_arch_vcpu_ioctl_run().

>> this case does the exact same thing as the previous call, so remove it.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arch/arm64/kvm/pmu-emul.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index 398f6df1bbe4..4ad66a532e38 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -850,8 +850,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>  		   return -EINVAL;
>>  	}
>>  
>> -	kvm_pmu_vcpu_reset(vcpu);
>> -
> this patch does not apply for me (vcpu->arch.pmu.ready = true; ?)

I should have mentioned it in the cover letter, this is on top of Marc's pmu-undef
branch:

https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-undef

it should work on top of commit 7521c3a9e630 ("KVM: arm64: Get ready of the PMU
ready state"), the branch was rebased since I sent the patches.

Thanks,
Alex
>
> Thanks
>
> Eric
>>  	return 0;
>>  }
>>  
>>

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

Re: [PATCH 5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

[Auger Eric]

Hi Alexandru,

On 12/14/20 3:02 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> Thanks for having a look!
> 
> On 12/14/20 1:48 PM, Auger Eric wrote:
>> Alexandru,
>>
>> On 12/1/20 4:01 PM, Alexandru Elisei wrote:
>>> KVM_ARM_VCPU_INIT ioctl calls kvm_reset_vcpu(), which in turn resets the
>>> PMU with a call to kvm_pmu_vcpu_reset(). The function zeroes the PMU
>>> chained counters bitmap and stops all the counters with a perf event
>>> attached. Because it is called before the VCPU has had the chance to run,
>>> no perf events are in use and none are released.
>>>
>>> kvm_arm_pmu_v3_enable(), called by kvm_vcpu_first_run_init() only if the
>>> VCPU has not been initialized, also resets the PMU. kvm_pmu_vcpu_reset() in
>> This sounds strange to me. kvm_vcpu_first_run_init() only is called if
>> kvm_vcpu_initialized(vcpu) is true.
> 
> Typo on my part, the "not" should not be there. kvm_vcpu_first_run_init() is
> called only if kvm_vcpu_initialized() returns true in kvm_arch_vcpu_ioctl_run().
> 
>>> this case does the exact same thing as the previous call, so remove it.
>>>
>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>> ---
>>>  arch/arm64/kvm/pmu-emul.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>> index 398f6df1bbe4..4ad66a532e38 100644
>>> --- a/arch/arm64/kvm/pmu-emul.c
>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>> @@ -850,8 +850,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>>  		   return -EINVAL;
>>>  	}
>>>  
>>> -	kvm_pmu_vcpu_reset(vcpu);
>>> -
>> this patch does not apply for me (vcpu->arch.pmu.ready = true; ?)
> 
> I should have mentioned it in the cover letter, this is on top of Marc's pmu-undef
> branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-undef
> 
> it should work on top of commit 7521c3a9e630 ("KVM: arm64: Get ready of the PMU
> ready state"), the branch was rebased since I sent the patches.
OK noted.

Thanks

Eric
> 
> Thanks,
> Alex
>>
>> Thanks
>>
>> Eric
>>>  	return 0;
>>>  }
>>>  
>>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()

[Auger Eric]

Hi Alexandru,

On 12/14/20 3:02 PM, Alexandru Elisei wrote:
> Hi Eric,
> 
> Thanks for having a look!
> 
> On 12/14/20 1:48 PM, Auger Eric wrote:
>> Alexandru,
>>
>> On 12/1/20 4:01 PM, Alexandru Elisei wrote:
>>> KVM_ARM_VCPU_INIT ioctl calls kvm_reset_vcpu(), which in turn resets the
>>> PMU with a call to kvm_pmu_vcpu_reset(). The function zeroes the PMU
>>> chained counters bitmap and stops all the counters with a perf event
>>> attached. Because it is called before the VCPU has had the chance to run,
>>> no perf events are in use and none are released.
>>>
>>> kvm_arm_pmu_v3_enable(), called by kvm_vcpu_first_run_init() only if the
>>> VCPU has not been initialized, also resets the PMU. kvm_pmu_vcpu_reset() in
>> This sounds strange to me. kvm_vcpu_first_run_init() only is called if
>> kvm_vcpu_initialized(vcpu) is true.
> 
> Typo on my part, the "not" should not be there. kvm_vcpu_first_run_init() is
> called only if kvm_vcpu_initialized() returns true in kvm_arch_vcpu_ioctl_run().
> 
>>> this case does the exact same thing as the previous call, so remove it.
>>>
>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>> ---
>>>  arch/arm64/kvm/pmu-emul.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>> index 398f6df1bbe4..4ad66a532e38 100644
>>> --- a/arch/arm64/kvm/pmu-emul.c
>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>> @@ -850,8 +850,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>>  		   return -EINVAL;
>>>  	}
>>>  
>>> -	kvm_pmu_vcpu_reset(vcpu);
>>> -
>> this patch does not apply for me (vcpu->arch.pmu.ready = true; ?)
> 
> I should have mentioned it in the cover letter, this is on top of Marc's pmu-undef
> branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-undef
> 
> it should work on top of commit 7521c3a9e630 ("KVM: arm64: Get ready of the PMU
> ready state"), the branch was rebased since I sent the patches.
OK noted.

Thanks

Eric
> 
> Thanks,
> Alex
>>
>> Thanks
>>
>> Eric
>>>  	return 0;
>>>  }
>>>  
>>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


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

Re: [PATCH 3/5] KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()

[Marc Zyngier]

On Mon, 14 Dec 2020 12:55:51 +0000,
Auger Eric <eric.auger@redhat.com> wrote:
> 
> Hi Alexandru,
> 
> On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> > kvm_vgic_map_resources() is called when a VCPU if first run and it maps all
> > the VGIC MMIO regions. To prevent double-initialization, the VGIC uses the
> > ready variable to keep track of the state of resources and the global KVM
> > mutex to protect against concurrent accesses. After the lock is taken, the
> > variable is checked again in case another VCPU took the lock between the
> > current VCPU reading ready equals false and taking the lock.
> > 
> > The double-checked lock pattern is spread across four different functions:
> > in kvm_vcpu_first_run_init(), in kvm_vgic_map_resource() and in
> > vgic_{v2,v3}_map_resources(), which makes it hard to reason about and
> > introduces minor code duplication. Consolidate the checks in
> > kvm_vgic_map_resources(), where the lock is taken.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  arch/arm64/kvm/arm.c            | 8 +++-----
> >  arch/arm64/kvm/vgic/vgic-init.c | 6 ++++++
> >  arch/arm64/kvm/vgic/vgic-v2.c   | 3 ---
> >  arch/arm64/kvm/vgic/vgic-v3.c   | 3 ---
> >  4 files changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 9d69d2bf6943..65a5e89f5133 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -530,11 +530,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> >  		 * Map the VGIC hardware resources before running a vcpu the
> >  		 * first time on this VM.
> >  		 */
> > -		if (unlikely(!vgic_ready(kvm))) {
> > -			ret = kvm_vgic_map_resources(kvm);
> > -			if (ret)
> > -				return ret;
> > -		}
> > +		ret = kvm_vgic_map_resources(kvm);
> > +		if (ret)
> > +			return ret;
> >  	} else {
> >  		/*
> >  		 * Tell the rest of the code that there are userspace irqchip
> > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> > index 32e32d67a127..a2f4d1c85f00 100644
> > --- a/arch/arm64/kvm/vgic/vgic-init.c
> > +++ b/arch/arm64/kvm/vgic/vgic-init.c
> > @@ -428,7 +428,13 @@ int kvm_vgic_map_resources(struct kvm *kvm)
> >  	struct vgic_dist *dist = &kvm->arch.vgic;
> >  	int ret = 0;
> >  
> > +	if (likely(vgic_ready(kvm)))
> > +		return 0;
> > +
> >  	mutex_lock(&kvm->lock);
> > +	if (vgic_ready(kvm))
> > +		goto out;
> > +
> While we are at it, the setting of dist->ready may be moved in
> kvm_vgic_map_resources() too.

Good point. FWIW, I have added an extra patch on top to that effect.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 3/5] KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()

[Marc Zyngier]

On Mon, 14 Dec 2020 12:55:51 +0000,
Auger Eric <eric.auger@redhat.com> wrote:
> 
> Hi Alexandru,
> 
> On 12/1/20 4:01 PM, Alexandru Elisei wrote:
> > kvm_vgic_map_resources() is called when a VCPU if first run and it maps all
> > the VGIC MMIO regions. To prevent double-initialization, the VGIC uses the
> > ready variable to keep track of the state of resources and the global KVM
> > mutex to protect against concurrent accesses. After the lock is taken, the
> > variable is checked again in case another VCPU took the lock between the
> > current VCPU reading ready equals false and taking the lock.
> > 
> > The double-checked lock pattern is spread across four different functions:
> > in kvm_vcpu_first_run_init(), in kvm_vgic_map_resource() and in
> > vgic_{v2,v3}_map_resources(), which makes it hard to reason about and
> > introduces minor code duplication. Consolidate the checks in
> > kvm_vgic_map_resources(), where the lock is taken.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  arch/arm64/kvm/arm.c            | 8 +++-----
> >  arch/arm64/kvm/vgic/vgic-init.c | 6 ++++++
> >  arch/arm64/kvm/vgic/vgic-v2.c   | 3 ---
> >  arch/arm64/kvm/vgic/vgic-v3.c   | 3 ---
> >  4 files changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 9d69d2bf6943..65a5e89f5133 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -530,11 +530,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
> >  		 * Map the VGIC hardware resources before running a vcpu the
> >  		 * first time on this VM.
> >  		 */
> > -		if (unlikely(!vgic_ready(kvm))) {
> > -			ret = kvm_vgic_map_resources(kvm);
> > -			if (ret)
> > -				return ret;
> > -		}
> > +		ret = kvm_vgic_map_resources(kvm);
> > +		if (ret)
> > +			return ret;
> >  	} else {
> >  		/*
> >  		 * Tell the rest of the code that there are userspace irqchip
> > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> > index 32e32d67a127..a2f4d1c85f00 100644
> > --- a/arch/arm64/kvm/vgic/vgic-init.c
> > +++ b/arch/arm64/kvm/vgic/vgic-init.c
> > @@ -428,7 +428,13 @@ int kvm_vgic_map_resources(struct kvm *kvm)
> >  	struct vgic_dist *dist = &kvm->arch.vgic;
> >  	int ret = 0;
> >  
> > +	if (likely(vgic_ready(kvm)))
> > +		return 0;
> > +
> >  	mutex_lock(&kvm->lock);
> > +	if (vgic_ready(kvm))
> > +		goto out;
> > +
> While we are at it, the setting of dist->ready may be moved in
> kvm_vgic_map_resources() too.

Good point. FWIW, I have added an extra patch on top to that effect.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

Re: [PATCH 0/5] KVM: arm64: Miscellaneous improvements

[Marc Zyngier]

On Tue, 1 Dec 2020 15:01:52 +0000, Alexandru Elisei wrote:
> The documentation update in the first patch was suggested by Marc [1]. When
> I was going through the code to track down all the places error codes were
> coming from I noticed a few things that in my opinion could be improved.
> The following patches aim to do just that. I'm fine dropping them if the
> churn looks unjustified.
> 
> Tested the Documentation changes by building pdfdocs, didn't notice any
> warnings regarding api.rst.
> 
> [...]

Applied to next, thanks!

[1/5] KVM: Documentation: Add arm64 KVM_RUN error codes
      commit: 3557ae187c32203d1bb8b48ee1e2e7bdb23d98d5
[2/5] KVM: arm64: arch_timer: Remove VGIC initialization check
      commit: f16570ba47ff2b3766ebeaba6f4b80ad48cfd6a1
[3/5] KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()
      commit: 1c91f06d296de4f0c27022f5ec464e047d471215
[4/5] KVM: arm64: Update comment in kvm_vgic_map_resources()
      commit: 9e5c23b9bd71d00b07720b2a8037b019d356e9df
[5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()
      commit: 282ff80135717cc43f1e33ddd4b0cd9e760d060b

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 0/5] KVM: arm64: Miscellaneous improvements

[Marc Zyngier]

On Tue, 1 Dec 2020 15:01:52 +0000, Alexandru Elisei wrote:
> The documentation update in the first patch was suggested by Marc [1]. When
> I was going through the code to track down all the places error codes were
> coming from I noticed a few things that in my opinion could be improved.
> The following patches aim to do just that. I'm fine dropping them if the
> churn looks unjustified.
> 
> Tested the Documentation changes by building pdfdocs, didn't notice any
> warnings regarding api.rst.
> 
> [...]

Applied to next, thanks!

[1/5] KVM: Documentation: Add arm64 KVM_RUN error codes
      commit: 3557ae187c32203d1bb8b48ee1e2e7bdb23d98d5
[2/5] KVM: arm64: arch_timer: Remove VGIC initialization check
      commit: f16570ba47ff2b3766ebeaba6f4b80ad48cfd6a1
[3/5] KVM: arm64: Move double-checked lock to kvm_vgic_map_resources()
      commit: 1c91f06d296de4f0c27022f5ec464e047d471215
[4/5] KVM: arm64: Update comment in kvm_vgic_map_resources()
      commit: 9e5c23b9bd71d00b07720b2a8037b019d356e9df
[5/5] KVM: arm64: Remove redundant call to kvm_pmu_vcpu_reset()
      commit: 282ff80135717cc43f1e33ddd4b0cd9e760d060b

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.



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