[PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

This series contains 3 fixes which were found while testing
ARM64 Nested Virtualization patch series.

First patch avoids the restart of hrtimer when timer interrupt is
fired/forwarded to Guest-Hypervisor.

Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.

Third patch fixes the NestedVM boot hang seen when Guest Hypersior
configured with 64K pagesize where as Host Hypervisor with 4K.

These patches are rebased on Nested Virtualization V6 patchset[1].

[1] https://www.spinics.net/lists/kvm/msg265656.html

D Scott Phillips (1):
  KVM: arm64: nv: only emulate timers that have not yet fired

Ganapatrao Kulkarni (2):
  KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.
  KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than
    block size.

 arch/arm64/kvm/arch_timer.c | 8 +++++++-
 arch/arm64/kvm/mmu.c        | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.33.1

[PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

This series contains 3 fixes which were found while testing
ARM64 Nested Virtualization patch series.

First patch avoids the restart of hrtimer when timer interrupt is
fired/forwarded to Guest-Hypervisor.

Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.

Third patch fixes the NestedVM boot hang seen when Guest Hypersior
configured with 64K pagesize where as Host Hypervisor with 4K.

These patches are rebased on Nested Virtualization V6 patchset[1].

[1] https://www.spinics.net/lists/kvm/msg265656.html

D Scott Phillips (1):
  KVM: arm64: nv: only emulate timers that have not yet fired

Ganapatrao Kulkarni (2):
  KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.
  KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than
    block size.

 arch/arm64/kvm/arch_timer.c | 8 +++++++-
 arch/arm64/kvm/mmu.c        | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.33.1


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

[PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

This series contains 3 fixes which were found while testing
ARM64 Nested Virtualization patch series.

First patch avoids the restart of hrtimer when timer interrupt is
fired/forwarded to Guest-Hypervisor.

Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.

Third patch fixes the NestedVM boot hang seen when Guest Hypersior
configured with 64K pagesize where as Host Hypervisor with 4K.

These patches are rebased on Nested Virtualization V6 patchset[1].

[1] https://www.spinics.net/lists/kvm/msg265656.html

D Scott Phillips (1):
  KVM: arm64: nv: only emulate timers that have not yet fired

Ganapatrao Kulkarni (2):
  KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.
  KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than
    block size.

 arch/arm64/kvm/arch_timer.c | 8 +++++++-
 arch/arm64/kvm/mmu.c        | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.33.1

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

[PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Ganapatrao Kulkarni]

From: D Scott Phillips <scott@os.amperecomputing.com>

The timer emulation logic goes into an infinite loop when the NestedVM(L2)
timer is being emulated.

While the CPU is executing in L1 context, the L2 timers are emulated using
host hrtimer. When the delta of cval and current time reaches zero, the
vtimer interrupt is fired/forwarded to L2, however the emulation function
in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
set to now, triggering hrtimer to fire immediately and resulting in a
continuous trigger of hrtimer and endless looping in the timer emulation.

Adding a fix to avoid restarting of the hrtimer if the interrupt is
already fired.

Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/arch_timer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 2371796b1ab5..27a6ec46803a 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
 		return;
 	}
 
-	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
+	if (!ctx->irq.level)
+		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
 }
 
 static void timer_save_state(struct arch_timer_context *ctx)
-- 
2.33.1

[PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Ganapatrao Kulkarni]

From: D Scott Phillips <scott@os.amperecomputing.com>

The timer emulation logic goes into an infinite loop when the NestedVM(L2)
timer is being emulated.

While the CPU is executing in L1 context, the L2 timers are emulated using
host hrtimer. When the delta of cval and current time reaches zero, the
vtimer interrupt is fired/forwarded to L2, however the emulation function
in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
set to now, triggering hrtimer to fire immediately and resulting in a
continuous trigger of hrtimer and endless looping in the timer emulation.

Adding a fix to avoid restarting of the hrtimer if the interrupt is
already fired.

Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/arch_timer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 2371796b1ab5..27a6ec46803a 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
 		return;
 	}
 
-	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
+	if (!ctx->irq.level)
+		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
 }
 
 static void timer_save_state(struct arch_timer_context *ctx)
-- 
2.33.1


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

[PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Ganapatrao Kulkarni]

From: D Scott Phillips <scott@os.amperecomputing.com>

The timer emulation logic goes into an infinite loop when the NestedVM(L2)
timer is being emulated.

While the CPU is executing in L1 context, the L2 timers are emulated using
host hrtimer. When the delta of cval and current time reaches zero, the
vtimer interrupt is fired/forwarded to L2, however the emulation function
in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
set to now, triggering hrtimer to fire immediately and resulting in a
continuous trigger of hrtimer and endless looping in the timer emulation.

Adding a fix to avoid restarting of the hrtimer if the interrupt is
already fired.

Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/arch_timer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 2371796b1ab5..27a6ec46803a 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
 		return;
 	}
 
-	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
+	if (!ctx->irq.level)
+		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
 }
 
 static void timer_save_state(struct arch_timer_context *ctx)
-- 
2.33.1

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

[PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Ganapatrao Kulkarni]

Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
loaded timer.

For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
This results in the drop of interrupt from Guest-Hypervisor, where as
Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
are stuck in Idle thread and rcu soft lockups are seen.

This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
trap handler is emulating the ISTATUS bit.

Adding code to set/emulate the ISTATUS when the emulated timers are fired.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/arch_timer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 27a6ec46803a..0b32d943d2d5 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
 			      struct arch_timer_context *timer,
 			      enum kvm_arch_timer_regs treg);
 static bool kvm_arch_timer_get_input_level(int vintid);
+static u64 read_timer_ctl(struct arch_timer_context *timer);
 
 static struct irq_ops arch_timer_irq_ops = {
 	.get_input_level = kvm_arch_timer_get_input_level,
@@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
 		return HRTIMER_RESTART;
 	}
 
+	/* Timer emulated, emulate ISTATUS also */
+	timer_set_ctl(ctx, read_timer_ctl(ctx));
 	kvm_timer_update_irq(vcpu, true, ctx);
 	return HRTIMER_NORESTART;
 }
@@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
 	trace_kvm_timer_emulate(ctx, should_fire);
 
 	if (should_fire != ctx->irq.level) {
+		/* Timer emulated, emulate ISTATUS also */
+		timer_set_ctl(ctx, read_timer_ctl(ctx));
 		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
 		return;
 	}
-- 
2.33.1

[PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Ganapatrao Kulkarni]

Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
loaded timer.

For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
This results in the drop of interrupt from Guest-Hypervisor, where as
Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
are stuck in Idle thread and rcu soft lockups are seen.

This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
trap handler is emulating the ISTATUS bit.

Adding code to set/emulate the ISTATUS when the emulated timers are fired.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/arch_timer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 27a6ec46803a..0b32d943d2d5 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
 			      struct arch_timer_context *timer,
 			      enum kvm_arch_timer_regs treg);
 static bool kvm_arch_timer_get_input_level(int vintid);
+static u64 read_timer_ctl(struct arch_timer_context *timer);
 
 static struct irq_ops arch_timer_irq_ops = {
 	.get_input_level = kvm_arch_timer_get_input_level,
@@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
 		return HRTIMER_RESTART;
 	}
 
+	/* Timer emulated, emulate ISTATUS also */
+	timer_set_ctl(ctx, read_timer_ctl(ctx));
 	kvm_timer_update_irq(vcpu, true, ctx);
 	return HRTIMER_NORESTART;
 }
@@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
 	trace_kvm_timer_emulate(ctx, should_fire);
 
 	if (should_fire != ctx->irq.level) {
+		/* Timer emulated, emulate ISTATUS also */
+		timer_set_ctl(ctx, read_timer_ctl(ctx));
 		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
 		return;
 	}
-- 
2.33.1


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

[PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Ganapatrao Kulkarni]

Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
loaded timer.

For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
This results in the drop of interrupt from Guest-Hypervisor, where as
Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
are stuck in Idle thread and rcu soft lockups are seen.

This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
trap handler is emulating the ISTATUS bit.

Adding code to set/emulate the ISTATUS when the emulated timers are fired.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/arch_timer.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 27a6ec46803a..0b32d943d2d5 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
 			      struct arch_timer_context *timer,
 			      enum kvm_arch_timer_regs treg);
 static bool kvm_arch_timer_get_input_level(int vintid);
+static u64 read_timer_ctl(struct arch_timer_context *timer);
 
 static struct irq_ops arch_timer_irq_ops = {
 	.get_input_level = kvm_arch_timer_get_input_level,
@@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
 		return HRTIMER_RESTART;
 	}
 
+	/* Timer emulated, emulate ISTATUS also */
+	timer_set_ctl(ctx, read_timer_ctl(ctx));
 	kvm_timer_update_irq(vcpu, true, ctx);
 	return HRTIMER_NORESTART;
 }
@@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
 	trace_kvm_timer_emulate(ctx, should_fire);
 
 	if (should_fire != ctx->irq.level) {
+		/* Timer emulated, emulate ISTATUS also */
+		timer_set_ctl(ctx, read_timer_ctl(ctx));
 		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
 		return;
 	}
-- 
2.33.1

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

[PATCH 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Ganapatrao Kulkarni]

In NV case, Shadow stage 2 page table is created using host hypervisor
page table configuration like page size, block size etc. Also, the shadow
stage 2 table uses block level mapping if the Guest Hypervisor IPA is
backed by the THP pages. However, this is resulting in illegal mapping of
NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
hypervisor are configured with different pagesize.

Adding fix to avoid block level mapping in stage 2 mapping if
max_map_size is smaller than the block size.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 6caa48da1b2e..3d4b53f153a1 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	 * backed by a THP and thus use block mapping if possible.
 	 */
 	if (vma_pagesize == PAGE_SIZE &&
-	    !(max_map_size == PAGE_SIZE || device)) {
+	    !(max_map_size < PMD_SIZE || device)) {
 		if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
 			vma_pagesize = fault_granule;
 		else
-- 
2.33.1

[PATCH 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Ganapatrao Kulkarni]

In NV case, Shadow stage 2 page table is created using host hypervisor
page table configuration like page size, block size etc. Also, the shadow
stage 2 table uses block level mapping if the Guest Hypervisor IPA is
backed by the THP pages. However, this is resulting in illegal mapping of
NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
hypervisor are configured with different pagesize.

Adding fix to avoid block level mapping in stage 2 mapping if
max_map_size is smaller than the block size.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 6caa48da1b2e..3d4b53f153a1 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	 * backed by a THP and thus use block mapping if possible.
 	 */
 	if (vma_pagesize == PAGE_SIZE &&
-	    !(max_map_size == PAGE_SIZE || device)) {
+	    !(max_map_size < PMD_SIZE || device)) {
 		if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
 			vma_pagesize = fault_granule;
 		else
-- 
2.33.1


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

[PATCH 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Ganapatrao Kulkarni]

In NV case, Shadow stage 2 page table is created using host hypervisor
page table configuration like page size, block size etc. Also, the shadow
stage 2 table uses block level mapping if the Guest Hypervisor IPA is
backed by the THP pages. However, this is resulting in illegal mapping of
NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
hypervisor are configured with different pagesize.

Adding fix to avoid block level mapping in stage 2 mapping if
max_map_size is smaller than the block size.

Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
---
 arch/arm64/kvm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 6caa48da1b2e..3d4b53f153a1 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 	 * backed by a THP and thus use block mapping if possible.
 	 */
 	if (vma_pagesize == PAGE_SIZE &&
-	    !(max_map_size == PAGE_SIZE || device)) {
+	    !(max_map_size < PMD_SIZE || device)) {
 		if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
 			vma_pagesize = fault_granule;
 		else
-- 
2.33.1

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

Hi Marc,

Any review comments on this series?

On 24-08-2022 11:33 am, Ganapatrao Kulkarni wrote:
> This series contains 3 fixes which were found while testing
> ARM64 Nested Virtualization patch series.
> 
> First patch avoids the restart of hrtimer when timer interrupt is
> fired/forwarded to Guest-Hypervisor.
> 
> Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.
> 
> Third patch fixes the NestedVM boot hang seen when Guest Hypersior
> configured with 64K pagesize where as Host Hypervisor with 4K.
> 
> These patches are rebased on Nested Virtualization V6 patchset[1].
> 
> [1] https://www.spinics.net/lists/kvm/msg265656.html
> 
> D Scott Phillips (1):
>    KVM: arm64: nv: only emulate timers that have not yet fired
> 
> Ganapatrao Kulkarni (2):
>    KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.
>    KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than
>      block size.
> 
>   arch/arm64/kvm/arch_timer.c | 8 +++++++-
>   arch/arm64/kvm/mmu.c        | 2 +-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 

Thanks,
Ganapat

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

Hi Marc,

Any review comments on this series?

On 24-08-2022 11:33 am, Ganapatrao Kulkarni wrote:
> This series contains 3 fixes which were found while testing
> ARM64 Nested Virtualization patch series.
> 
> First patch avoids the restart of hrtimer when timer interrupt is
> fired/forwarded to Guest-Hypervisor.
> 
> Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.
> 
> Third patch fixes the NestedVM boot hang seen when Guest Hypersior
> configured with 64K pagesize where as Host Hypervisor with 4K.
> 
> These patches are rebased on Nested Virtualization V6 patchset[1].
> 
> [1] https://www.spinics.net/lists/kvm/msg265656.html
> 
> D Scott Phillips (1):
>    KVM: arm64: nv: only emulate timers that have not yet fired
> 
> Ganapatrao Kulkarni (2):
>    KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.
>    KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than
>      block size.
> 
>   arch/arm64/kvm/arch_timer.c | 8 +++++++-
>   arch/arm64/kvm/mmu.c        | 2 +-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

Hi Marc,

Any review comments on this series?

On 24-08-2022 11:33 am, Ganapatrao Kulkarni wrote:
> This series contains 3 fixes which were found while testing
> ARM64 Nested Virtualization patch series.
> 
> First patch avoids the restart of hrtimer when timer interrupt is
> fired/forwarded to Guest-Hypervisor.
> 
> Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.
> 
> Third patch fixes the NestedVM boot hang seen when Guest Hypersior
> configured with 64K pagesize where as Host Hypervisor with 4K.
> 
> These patches are rebased on Nested Virtualization V6 patchset[1].
> 
> [1] https://www.spinics.net/lists/kvm/msg265656.html
> 
> D Scott Phillips (1):
>    KVM: arm64: nv: only emulate timers that have not yet fired
> 
> Ganapatrao Kulkarni (2):
>    KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.
>    KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than
>      block size.
> 
>   arch/arm64/kvm/arch_timer.c | 8 +++++++-
>   arch/arm64/kvm/mmu.c        | 2 +-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 

Thanks,
Ganapat

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On Mon, 10 Oct 2022 06:56:31 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>
> Hi Marc,
> 
> Any review comments on this series?

Not yet. So far, the NV stuff is put on ice until I can source some
actual HW to make the development less painful.

	M.

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On Mon, 10 Oct 2022 06:56:31 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>
> Hi Marc,
> 
> Any review comments on this series?

Not yet. So far, the NV stuff is put on ice until I can source some
actual HW to make the development less painful.

	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/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On Mon, 10 Oct 2022 06:56:31 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>
> Hi Marc,
> 
> Any review comments on this series?

Not yet. So far, the NV stuff is put on ice until I can source some
actual HW to make the development less painful.

	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 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Marc Zyngier]

On Wed, 24 Aug 2022 07:03:02 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> From: D Scott Phillips <scott@os.amperecomputing.com>
> 
> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
> timer is being emulated.
> 
> While the CPU is executing in L1 context, the L2 timers are emulated using
> host hrtimer. When the delta of cval and current time reaches zero, the
> vtimer interrupt is fired/forwarded to L2, however the emulation function
> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
> set to now, triggering hrtimer to fire immediately and resulting in a
> continuous trigger of hrtimer and endless looping in the timer emulation.
> 
> Adding a fix to avoid restarting of the hrtimer if the interrupt is
> already fired.
> 
> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/arch_timer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 2371796b1ab5..27a6ec46803a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>  		return;
>  	}
>  
> -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
> +	if (!ctx->irq.level)
> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>  }
>  
>  static void timer_save_state(struct arch_timer_context *ctx)

I think this is a regression introduced by bee038a67487 ("KVM:
arm/arm64: Rework the timer code to use a timer_map"), and you can see
it because the comment in this function doesn't make much sense
anymore.

Does the following work for you, mostly restoring the original code?

Thanks,

	M.

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index ad2a5df88810..4945c5b96f05 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
 	 * scheduled for the future.  If the timer cannot fire at all,
 	 * then we also don't need a soft timer.
 	 */
-	if (!kvm_timer_irq_can_fire(ctx)) {
+	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {
 		soft_timer_cancel(&ctx->hrtimer);
 		return;
 	}

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

Re: [PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Marc Zyngier]

On Wed, 24 Aug 2022 07:03:02 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> From: D Scott Phillips <scott@os.amperecomputing.com>
> 
> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
> timer is being emulated.
> 
> While the CPU is executing in L1 context, the L2 timers are emulated using
> host hrtimer. When the delta of cval and current time reaches zero, the
> vtimer interrupt is fired/forwarded to L2, however the emulation function
> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
> set to now, triggering hrtimer to fire immediately and resulting in a
> continuous trigger of hrtimer and endless looping in the timer emulation.
> 
> Adding a fix to avoid restarting of the hrtimer if the interrupt is
> already fired.
> 
> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/arch_timer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 2371796b1ab5..27a6ec46803a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>  		return;
>  	}
>  
> -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
> +	if (!ctx->irq.level)
> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>  }
>  
>  static void timer_save_state(struct arch_timer_context *ctx)

I think this is a regression introduced by bee038a67487 ("KVM:
arm/arm64: Rework the timer code to use a timer_map"), and you can see
it because the comment in this function doesn't make much sense
anymore.

Does the following work for you, mostly restoring the original code?

Thanks,

	M.

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index ad2a5df88810..4945c5b96f05 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
 	 * scheduled for the future.  If the timer cannot fire at all,
 	 * then we also don't need a soft timer.
 	 */
-	if (!kvm_timer_irq_can_fire(ctx)) {
+	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {
 		soft_timer_cancel(&ctx->hrtimer);
 		return;
 	}

-- 
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 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Marc Zyngier]

On Wed, 24 Aug 2022 07:03:02 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> From: D Scott Phillips <scott@os.amperecomputing.com>
> 
> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
> timer is being emulated.
> 
> While the CPU is executing in L1 context, the L2 timers are emulated using
> host hrtimer. When the delta of cval and current time reaches zero, the
> vtimer interrupt is fired/forwarded to L2, however the emulation function
> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
> set to now, triggering hrtimer to fire immediately and resulting in a
> continuous trigger of hrtimer and endless looping in the timer emulation.
> 
> Adding a fix to avoid restarting of the hrtimer if the interrupt is
> already fired.
> 
> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/arch_timer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 2371796b1ab5..27a6ec46803a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>  		return;
>  	}
>  
> -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
> +	if (!ctx->irq.level)
> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>  }
>  
>  static void timer_save_state(struct arch_timer_context *ctx)

I think this is a regression introduced by bee038a67487 ("KVM:
arm/arm64: Rework the timer code to use a timer_map"), and you can see
it because the comment in this function doesn't make much sense
anymore.

Does the following work for you, mostly restoring the original code?

Thanks,

	M.

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index ad2a5df88810..4945c5b96f05 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
 	 * scheduled for the future.  If the timer cannot fire at all,
 	 * then we also don't need a soft timer.
 	 */
-	if (!kvm_timer_irq_can_fire(ctx)) {
+	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {
 		soft_timer_cancel(&ctx->hrtimer);
 		return;
 	}

-- 
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 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Marc Zyngier]

On Wed, 24 Aug 2022 07:03:03 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
> loaded timer.
> 
> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
> This results in the drop of interrupt from Guest-Hypervisor, where as
> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
> are stuck in Idle thread and rcu soft lockups are seen.
> 
> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
> trap handler is emulating the ISTATUS bit.
> 
> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/arch_timer.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 27a6ec46803a..0b32d943d2d5 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>  			      struct arch_timer_context *timer,
>  			      enum kvm_arch_timer_regs treg);
>  static bool kvm_arch_timer_get_input_level(int vintid);
> +static u64 read_timer_ctl(struct arch_timer_context *timer);
>  
>  static struct irq_ops arch_timer_irq_ops = {
>  	.get_input_level = kvm_arch_timer_get_input_level,
> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
>  		return HRTIMER_RESTART;
>  	}
>  
> +	/* Timer emulated, emulate ISTATUS also */
> +	timer_set_ctl(ctx, read_timer_ctl(ctx));

Why should we do that for non-NV2 configurations?

>  	kvm_timer_update_irq(vcpu, true, ctx);
>  	return HRTIMER_NORESTART;
>  }
> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>  	trace_kvm_timer_emulate(ctx, should_fire);
>  
>  	if (should_fire != ctx->irq.level) {
> +		/* Timer emulated, emulate ISTATUS also */
> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
>  		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
>  		return;
>  	}

I'm not overly keen on this. Yes, we can set the status bit there. But
conversely, the bit will not get cleared when the guest reprograms the
timer, and will take a full exit/entry cycle for it to appear.

Ergo, the architecture is buggy as memory (the VNCR page) cannot be
used to emulate something as dynamic as a timer.

It is only with FEAT_ECV that we can solve this correctly by trapping
the counter/timer accesses and emulate them for the guest hypervisor.
I'd rather we add support for that, as I expect all the FEAT_NV2
implementations to have it (and hopefully FEAT_FGT as well).

Thanks,

	M.

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

Re: [PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Marc Zyngier]

On Wed, 24 Aug 2022 07:03:03 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
> loaded timer.
> 
> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
> This results in the drop of interrupt from Guest-Hypervisor, where as
> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
> are stuck in Idle thread and rcu soft lockups are seen.
> 
> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
> trap handler is emulating the ISTATUS bit.
> 
> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/arch_timer.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 27a6ec46803a..0b32d943d2d5 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>  			      struct arch_timer_context *timer,
>  			      enum kvm_arch_timer_regs treg);
>  static bool kvm_arch_timer_get_input_level(int vintid);
> +static u64 read_timer_ctl(struct arch_timer_context *timer);
>  
>  static struct irq_ops arch_timer_irq_ops = {
>  	.get_input_level = kvm_arch_timer_get_input_level,
> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
>  		return HRTIMER_RESTART;
>  	}
>  
> +	/* Timer emulated, emulate ISTATUS also */
> +	timer_set_ctl(ctx, read_timer_ctl(ctx));

Why should we do that for non-NV2 configurations?

>  	kvm_timer_update_irq(vcpu, true, ctx);
>  	return HRTIMER_NORESTART;
>  }
> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>  	trace_kvm_timer_emulate(ctx, should_fire);
>  
>  	if (should_fire != ctx->irq.level) {
> +		/* Timer emulated, emulate ISTATUS also */
> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
>  		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
>  		return;
>  	}

I'm not overly keen on this. Yes, we can set the status bit there. But
conversely, the bit will not get cleared when the guest reprograms the
timer, and will take a full exit/entry cycle for it to appear.

Ergo, the architecture is buggy as memory (the VNCR page) cannot be
used to emulate something as dynamic as a timer.

It is only with FEAT_ECV that we can solve this correctly by trapping
the counter/timer accesses and emulate them for the guest hypervisor.
I'd rather we add support for that, as I expect all the FEAT_NV2
implementations to have it (and hopefully FEAT_FGT as well).

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 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Marc Zyngier]

On Wed, 24 Aug 2022 07:03:03 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
> loaded timer.
> 
> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
> This results in the drop of interrupt from Guest-Hypervisor, where as
> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
> are stuck in Idle thread and rcu soft lockups are seen.
> 
> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
> trap handler is emulating the ISTATUS bit.
> 
> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/arch_timer.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 27a6ec46803a..0b32d943d2d5 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>  			      struct arch_timer_context *timer,
>  			      enum kvm_arch_timer_regs treg);
>  static bool kvm_arch_timer_get_input_level(int vintid);
> +static u64 read_timer_ctl(struct arch_timer_context *timer);
>  
>  static struct irq_ops arch_timer_irq_ops = {
>  	.get_input_level = kvm_arch_timer_get_input_level,
> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
>  		return HRTIMER_RESTART;
>  	}
>  
> +	/* Timer emulated, emulate ISTATUS also */
> +	timer_set_ctl(ctx, read_timer_ctl(ctx));

Why should we do that for non-NV2 configurations?

>  	kvm_timer_update_irq(vcpu, true, ctx);
>  	return HRTIMER_NORESTART;
>  }
> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>  	trace_kvm_timer_emulate(ctx, should_fire);
>  
>  	if (should_fire != ctx->irq.level) {
> +		/* Timer emulated, emulate ISTATUS also */
> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
>  		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
>  		return;
>  	}

I'm not overly keen on this. Yes, we can set the status bit there. But
conversely, the bit will not get cleared when the guest reprograms the
timer, and will take a full exit/entry cycle for it to appear.

Ergo, the architecture is buggy as memory (the VNCR page) cannot be
used to emulate something as dynamic as a timer.

It is only with FEAT_ECV that we can solve this correctly by trapping
the counter/timer accesses and emulate them for the guest hypervisor.
I'd rather we add support for that, as I expect all the FEAT_NV2
implementations to have it (and hopefully FEAT_FGT as well).

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 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Marc Zyngier]

On Wed, 24 Aug 2022 07:03:04 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> In NV case, Shadow stage 2 page table is created using host hypervisor
> page table configuration like page size, block size etc. Also, the shadow
> stage 2 table uses block level mapping if the Guest Hypervisor IPA is
> backed by the THP pages. However, this is resulting in illegal mapping of
> NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
> hypervisor are configured with different pagesize.
> 
> Adding fix to avoid block level mapping in stage 2 mapping if
> max_map_size is smaller than the block size.
> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 6caa48da1b2e..3d4b53f153a1 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	 * backed by a THP and thus use block mapping if possible.
>  	 */
>  	if (vma_pagesize == PAGE_SIZE &&
> -	    !(max_map_size == PAGE_SIZE || device)) {
> +	    !(max_map_size < PMD_SIZE || device)) {
>  		if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
>  			vma_pagesize = fault_granule;
>  		else

That's quite a nice catch. I guess this was the main issue with
running 64kB L1 on a 4kB L0? Now, I'm not that fond of the fix itself,
and I think max_map_size should always represent something that is a
valid size *on the host*, specially when outside of NV-specific code.

How about something like this instead:

@@ -1346,6 +1346,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		 * table uses at least as big a mapping.
 		 */
 		max_map_size = min(kvm_s2_trans_size(nested), max_map_size);
+
+		if (max_map_size >= PMD_SIZE && max_map_size < PUD_SIZE)
+			max_map_size = PMD_SIZE;
+		else if (max_map_size >= PAGE_SIZE && max_map_size < PMD_SIZE)
+			max_map_size = PAGE_SIZE;
 	}
 
 	vma_pagesize = min(vma_pagesize, max_map_size);


Admittedly, this is a lot uglier than your fix. But it keep the nested
horror localised, and doesn't risk being reverted by accident by
people who would not take NV into account (can't blame them, really).

Can you please give it a go?

Thanks,

	M.

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

Re: [PATCH 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Marc Zyngier]

On Wed, 24 Aug 2022 07:03:04 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> In NV case, Shadow stage 2 page table is created using host hypervisor
> page table configuration like page size, block size etc. Also, the shadow
> stage 2 table uses block level mapping if the Guest Hypervisor IPA is
> backed by the THP pages. However, this is resulting in illegal mapping of
> NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
> hypervisor are configured with different pagesize.
> 
> Adding fix to avoid block level mapping in stage 2 mapping if
> max_map_size is smaller than the block size.
> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 6caa48da1b2e..3d4b53f153a1 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	 * backed by a THP and thus use block mapping if possible.
>  	 */
>  	if (vma_pagesize == PAGE_SIZE &&
> -	    !(max_map_size == PAGE_SIZE || device)) {
> +	    !(max_map_size < PMD_SIZE || device)) {
>  		if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
>  			vma_pagesize = fault_granule;
>  		else

That's quite a nice catch. I guess this was the main issue with
running 64kB L1 on a 4kB L0? Now, I'm not that fond of the fix itself,
and I think max_map_size should always represent something that is a
valid size *on the host*, specially when outside of NV-specific code.

How about something like this instead:

@@ -1346,6 +1346,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		 * table uses at least as big a mapping.
 		 */
 		max_map_size = min(kvm_s2_trans_size(nested), max_map_size);
+
+		if (max_map_size >= PMD_SIZE && max_map_size < PUD_SIZE)
+			max_map_size = PMD_SIZE;
+		else if (max_map_size >= PAGE_SIZE && max_map_size < PMD_SIZE)
+			max_map_size = PAGE_SIZE;
 	}
 
 	vma_pagesize = min(vma_pagesize, max_map_size);


Admittedly, this is a lot uglier than your fix. But it keep the nested
horror localised, and doesn't risk being reverted by accident by
people who would not take NV into account (can't blame them, really).

Can you please give it a go?

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/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Marc Zyngier]

On Wed, 24 Aug 2022 07:03:04 +0100,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> In NV case, Shadow stage 2 page table is created using host hypervisor
> page table configuration like page size, block size etc. Also, the shadow
> stage 2 table uses block level mapping if the Guest Hypervisor IPA is
> backed by the THP pages. However, this is resulting in illegal mapping of
> NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
> hypervisor are configured with different pagesize.
> 
> Adding fix to avoid block level mapping in stage 2 mapping if
> max_map_size is smaller than the block size.
> 
> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> ---
>  arch/arm64/kvm/mmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 6caa48da1b2e..3d4b53f153a1 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>  	 * backed by a THP and thus use block mapping if possible.
>  	 */
>  	if (vma_pagesize == PAGE_SIZE &&
> -	    !(max_map_size == PAGE_SIZE || device)) {
> +	    !(max_map_size < PMD_SIZE || device)) {
>  		if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
>  			vma_pagesize = fault_granule;
>  		else

That's quite a nice catch. I guess this was the main issue with
running 64kB L1 on a 4kB L0? Now, I'm not that fond of the fix itself,
and I think max_map_size should always represent something that is a
valid size *on the host*, specially when outside of NV-specific code.

How about something like this instead:

@@ -1346,6 +1346,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
 		 * table uses at least as big a mapping.
 		 */
 		max_map_size = min(kvm_s2_trans_size(nested), max_map_size);
+
+		if (max_map_size >= PMD_SIZE && max_map_size < PUD_SIZE)
+			max_map_size = PMD_SIZE;
+		else if (max_map_size >= PAGE_SIZE && max_map_size < PMD_SIZE)
+			max_map_size = PAGE_SIZE;
 	}
 
 	vma_pagesize = min(vma_pagesize, max_map_size);


Admittedly, this is a lot uglier than your fix. But it keep the nested
horror localised, and doesn't risk being reverted by accident by
people who would not take NV into account (can't blame them, really).

Can you please give it a go?

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 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Marc Zyngier]

On Thu, 29 Dec 2022 13:53:15 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Wed, 24 Aug 2022 07:03:03 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> > 
> > Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
> > enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
> > loaded timer.
> > 
> > For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
> > bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
> > This results in the drop of interrupt from Guest-Hypervisor, where as
> > Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
> > to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
> > are stuck in Idle thread and rcu soft lockups are seen.
> > 
> > This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
> > trap handler is emulating the ISTATUS bit.
> > 
> > Adding code to set/emulate the ISTATUS when the emulated timers are fired.
> > 
> > Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> > ---
> >  arch/arm64/kvm/arch_timer.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index 27a6ec46803a..0b32d943d2d5 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
> >  			      struct arch_timer_context *timer,
> >  			      enum kvm_arch_timer_regs treg);
> >  static bool kvm_arch_timer_get_input_level(int vintid);
> > +static u64 read_timer_ctl(struct arch_timer_context *timer);
> >  
> >  static struct irq_ops arch_timer_irq_ops = {
> >  	.get_input_level = kvm_arch_timer_get_input_level,
> > @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
> >  		return HRTIMER_RESTART;
> >  	}
> >  
> > +	/* Timer emulated, emulate ISTATUS also */
> > +	timer_set_ctl(ctx, read_timer_ctl(ctx));
> 
> Why should we do that for non-NV2 configurations?
> 
> >  	kvm_timer_update_irq(vcpu, true, ctx);
> >  	return HRTIMER_NORESTART;
> >  }
> > @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >  	trace_kvm_timer_emulate(ctx, should_fire);
> >  
> >  	if (should_fire != ctx->irq.level) {
> > +		/* Timer emulated, emulate ISTATUS also */
> > +		timer_set_ctl(ctx, read_timer_ctl(ctx));
> >  		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
> >  		return;
> >  	}
> 
> I'm not overly keen on this. Yes, we can set the status bit there. But
> conversely, the bit will not get cleared when the guest reprograms the
> timer, and will take a full exit/entry cycle for it to appear.
> 
> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
> used to emulate something as dynamic as a timer.
> 
> It is only with FEAT_ECV that we can solve this correctly by trapping
> the counter/timer accesses and emulate them for the guest hypervisor.
> I'd rather we add support for that, as I expect all the FEAT_NV2
> implementations to have it (and hopefully FEAT_FGT as well).

So I went ahead and implemented some very basic FEAT_ECV support to
correctly emulate the timers (trapping the CTL/CVAL accesses).

Performance dropped like a rock (~30% extra overhead) for L2
exit-heavy workloads that are terminated in userspace, such as virtio.
For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
as we save/restore the timer context, and this is enough to make
things visibly slower, even on a pretty fast machine.

I managed to get *some* performance back by satisfying CTL/CVAL reads
very early on the exit path (a pretty common theme with NV). Which
means we end-up needing something like what you have -- only a bit
more complete. I came up with the following:

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 4945c5b96f05..a198a6211e2a 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 {
 	int ret;
 
+	/*
+	 * Paper over NV2 brokenness by publishing the interrupt status
+	 * bit. This still results in a poor quality of emulation (guest
+	 * writes will have no effect until the next exit).
+	 *
+	 * But hey, it's fast, right?
+	 */
+	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
+	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
+		u32 ctl = timer_get_ctl(timer_ctx);
+
+		if (new_level)
+			ctl |= ARCH_TIMER_CTRL_IT_STAT;
+		else
+			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
+
+		timer_set_ctl(timer_ctx, ctl);
+	}
+
 	timer_ctx->irq.level = new_level;
 	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
 				   timer_ctx->irq.level);

which reports the interrupt state in all cases.

Does this work for you?

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 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Marc Zyngier]

On Thu, 29 Dec 2022 13:53:15 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Wed, 24 Aug 2022 07:03:03 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> > 
> > Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
> > enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
> > loaded timer.
> > 
> > For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
> > bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
> > This results in the drop of interrupt from Guest-Hypervisor, where as
> > Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
> > to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
> > are stuck in Idle thread and rcu soft lockups are seen.
> > 
> > This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
> > trap handler is emulating the ISTATUS bit.
> > 
> > Adding code to set/emulate the ISTATUS when the emulated timers are fired.
> > 
> > Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> > ---
> >  arch/arm64/kvm/arch_timer.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index 27a6ec46803a..0b32d943d2d5 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
> >  			      struct arch_timer_context *timer,
> >  			      enum kvm_arch_timer_regs treg);
> >  static bool kvm_arch_timer_get_input_level(int vintid);
> > +static u64 read_timer_ctl(struct arch_timer_context *timer);
> >  
> >  static struct irq_ops arch_timer_irq_ops = {
> >  	.get_input_level = kvm_arch_timer_get_input_level,
> > @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
> >  		return HRTIMER_RESTART;
> >  	}
> >  
> > +	/* Timer emulated, emulate ISTATUS also */
> > +	timer_set_ctl(ctx, read_timer_ctl(ctx));
> 
> Why should we do that for non-NV2 configurations?
> 
> >  	kvm_timer_update_irq(vcpu, true, ctx);
> >  	return HRTIMER_NORESTART;
> >  }
> > @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >  	trace_kvm_timer_emulate(ctx, should_fire);
> >  
> >  	if (should_fire != ctx->irq.level) {
> > +		/* Timer emulated, emulate ISTATUS also */
> > +		timer_set_ctl(ctx, read_timer_ctl(ctx));
> >  		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
> >  		return;
> >  	}
> 
> I'm not overly keen on this. Yes, we can set the status bit there. But
> conversely, the bit will not get cleared when the guest reprograms the
> timer, and will take a full exit/entry cycle for it to appear.
> 
> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
> used to emulate something as dynamic as a timer.
> 
> It is only with FEAT_ECV that we can solve this correctly by trapping
> the counter/timer accesses and emulate them for the guest hypervisor.
> I'd rather we add support for that, as I expect all the FEAT_NV2
> implementations to have it (and hopefully FEAT_FGT as well).

So I went ahead and implemented some very basic FEAT_ECV support to
correctly emulate the timers (trapping the CTL/CVAL accesses).

Performance dropped like a rock (~30% extra overhead) for L2
exit-heavy workloads that are terminated in userspace, such as virtio.
For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
as we save/restore the timer context, and this is enough to make
things visibly slower, even on a pretty fast machine.

I managed to get *some* performance back by satisfying CTL/CVAL reads
very early on the exit path (a pretty common theme with NV). Which
means we end-up needing something like what you have -- only a bit
more complete. I came up with the following:

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 4945c5b96f05..a198a6211e2a 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 {
 	int ret;
 
+	/*
+	 * Paper over NV2 brokenness by publishing the interrupt status
+	 * bit. This still results in a poor quality of emulation (guest
+	 * writes will have no effect until the next exit).
+	 *
+	 * But hey, it's fast, right?
+	 */
+	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
+	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
+		u32 ctl = timer_get_ctl(timer_ctx);
+
+		if (new_level)
+			ctl |= ARCH_TIMER_CTRL_IT_STAT;
+		else
+			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
+
+		timer_set_ctl(timer_ctx, ctl);
+	}
+
 	timer_ctx->irq.level = new_level;
 	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
 				   timer_ctx->irq.level);

which reports the interrupt state in all cases.

Does this work for you?

Thanks,

	M.

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

Re: [PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Marc Zyngier]

On Thu, 29 Dec 2022 13:53:15 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Wed, 24 Aug 2022 07:03:03 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> > 
> > Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
> > enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
> > loaded timer.
> > 
> > For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
> > bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
> > This results in the drop of interrupt from Guest-Hypervisor, where as
> > Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
> > to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
> > are stuck in Idle thread and rcu soft lockups are seen.
> > 
> > This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
> > trap handler is emulating the ISTATUS bit.
> > 
> > Adding code to set/emulate the ISTATUS when the emulated timers are fired.
> > 
> > Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> > ---
> >  arch/arm64/kvm/arch_timer.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index 27a6ec46803a..0b32d943d2d5 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
> >  			      struct arch_timer_context *timer,
> >  			      enum kvm_arch_timer_regs treg);
> >  static bool kvm_arch_timer_get_input_level(int vintid);
> > +static u64 read_timer_ctl(struct arch_timer_context *timer);
> >  
> >  static struct irq_ops arch_timer_irq_ops = {
> >  	.get_input_level = kvm_arch_timer_get_input_level,
> > @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
> >  		return HRTIMER_RESTART;
> >  	}
> >  
> > +	/* Timer emulated, emulate ISTATUS also */
> > +	timer_set_ctl(ctx, read_timer_ctl(ctx));
> 
> Why should we do that for non-NV2 configurations?
> 
> >  	kvm_timer_update_irq(vcpu, true, ctx);
> >  	return HRTIMER_NORESTART;
> >  }
> > @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >  	trace_kvm_timer_emulate(ctx, should_fire);
> >  
> >  	if (should_fire != ctx->irq.level) {
> > +		/* Timer emulated, emulate ISTATUS also */
> > +		timer_set_ctl(ctx, read_timer_ctl(ctx));
> >  		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
> >  		return;
> >  	}
> 
> I'm not overly keen on this. Yes, we can set the status bit there. But
> conversely, the bit will not get cleared when the guest reprograms the
> timer, and will take a full exit/entry cycle for it to appear.
> 
> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
> used to emulate something as dynamic as a timer.
> 
> It is only with FEAT_ECV that we can solve this correctly by trapping
> the counter/timer accesses and emulate them for the guest hypervisor.
> I'd rather we add support for that, as I expect all the FEAT_NV2
> implementations to have it (and hopefully FEAT_FGT as well).

So I went ahead and implemented some very basic FEAT_ECV support to
correctly emulate the timers (trapping the CTL/CVAL accesses).

Performance dropped like a rock (~30% extra overhead) for L2
exit-heavy workloads that are terminated in userspace, such as virtio.
For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
as we save/restore the timer context, and this is enough to make
things visibly slower, even on a pretty fast machine.

I managed to get *some* performance back by satisfying CTL/CVAL reads
very early on the exit path (a pretty common theme with NV). Which
means we end-up needing something like what you have -- only a bit
more complete. I came up with the following:

diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 4945c5b96f05..a198a6211e2a 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
 {
 	int ret;
 
+	/*
+	 * Paper over NV2 brokenness by publishing the interrupt status
+	 * bit. This still results in a poor quality of emulation (guest
+	 * writes will have no effect until the next exit).
+	 *
+	 * But hey, it's fast, right?
+	 */
+	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
+	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
+		u32 ctl = timer_get_ctl(timer_ctx);
+
+		if (new_level)
+			ctl |= ARCH_TIMER_CTRL_IT_STAT;
+		else
+			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
+
+		timer_set_ctl(timer_ctx, ctl);
+	}
+
 	timer_ctx->irq.level = new_level;
 	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
 				   timer_ctx->irq.level);

which reports the interrupt state in all cases.

Does this work for you?

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 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Ganapatrao Kulkarni]

On 02-01-2023 05:16 pm, Marc Zyngier wrote:
> On Thu, 29 Dec 2022 13:53:15 +0000,
> Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Wed, 24 Aug 2022 07:03:03 +0100,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
>>> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
>>> loaded timer.
>>>
>>> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
>>> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
>>> This results in the drop of interrupt from Guest-Hypervisor, where as
>>> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
>>> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
>>> are stuck in Idle thread and rcu soft lockups are seen.
>>>
>>> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
>>> trap handler is emulating the ISTATUS bit.
>>>
>>> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   arch/arm64/kvm/arch_timer.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>> index 27a6ec46803a..0b32d943d2d5 100644
>>> --- a/arch/arm64/kvm/arch_timer.c
>>> +++ b/arch/arm64/kvm/arch_timer.c
>>> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>>>   			      struct arch_timer_context *timer,
>>>   			      enum kvm_arch_timer_regs treg);
>>>   static bool kvm_arch_timer_get_input_level(int vintid);
>>> +static u64 read_timer_ctl(struct arch_timer_context *timer);
>>>   
>>>   static struct irq_ops arch_timer_irq_ops = {
>>>   	.get_input_level = kvm_arch_timer_get_input_level,
>>> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
>>>   		return HRTIMER_RESTART;
>>>   	}
>>>   
>>> +	/* Timer emulated, emulate ISTATUS also */
>>> +	timer_set_ctl(ctx, read_timer_ctl(ctx));
>>
>> Why should we do that for non-NV2 configurations?
>>
>>>   	kvm_timer_update_irq(vcpu, true, ctx);
>>>   	return HRTIMER_NORESTART;
>>>   }
>>> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>   	trace_kvm_timer_emulate(ctx, should_fire);
>>>   
>>>   	if (should_fire != ctx->irq.level) {
>>> +		/* Timer emulated, emulate ISTATUS also */
>>> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
>>>   		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
>>>   		return;
>>>   	}
>>
>> I'm not overly keen on this. Yes, we can set the status bit there. But
>> conversely, the bit will not get cleared when the guest reprograms the
>> timer, and will take a full exit/entry cycle for it to appear.
>>
>> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
>> used to emulate something as dynamic as a timer.
>>
>> It is only with FEAT_ECV that we can solve this correctly by trapping
>> the counter/timer accesses and emulate them for the guest hypervisor.
>> I'd rather we add support for that, as I expect all the FEAT_NV2
>> implementations to have it (and hopefully FEAT_FGT as well).
> 
> So I went ahead and implemented some very basic FEAT_ECV support to
> correctly emulate the timers (trapping the CTL/CVAL accesses).
> 
> Performance dropped like a rock (~30% extra overhead) for L2
> exit-heavy workloads that are terminated in userspace, such as virtio.
> For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
> as we save/restore the timer context, and this is enough to make
> things visibly slower, even on a pretty fast machine.
> 
> I managed to get *some* performance back by satisfying CTL/CVAL reads
> very early on the exit path (a pretty common theme with NV). Which
> means we end-up needing something like what you have -- only a bit
> more complete. I came up with the following:
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 4945c5b96f05..a198a6211e2a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>   {
>   	int ret;
>   
> +	/*
> +	 * Paper over NV2 brokenness by publishing the interrupt status
> +	 * bit. This still results in a poor quality of emulation (guest
> +	 * writes will have no effect until the next exit).
> +	 *
> +	 * But hey, it's fast, right?
> +	 */
> +	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
> +	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
> +		u32 ctl = timer_get_ctl(timer_ctx);
> +
> +		if (new_level)
> +			ctl |= ARCH_TIMER_CTRL_IT_STAT;
> +		else
> +			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
> +
> +		timer_set_ctl(timer_ctx, ctl);
> +	}
> +
>   	timer_ctx->irq.level = new_level;
>   	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
>   				   timer_ctx->irq.level);
> 
> which reports the interrupt state in all cases.
> 
> Does this work for you?

Thanks Marc for the patch. I will try this and update at the earliest.

> 
> Thanks,
> 
> 	M.
> 

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

Re: [PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Ganapatrao Kulkarni]

On 02-01-2023 05:16 pm, Marc Zyngier wrote:
> On Thu, 29 Dec 2022 13:53:15 +0000,
> Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Wed, 24 Aug 2022 07:03:03 +0100,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
>>> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
>>> loaded timer.
>>>
>>> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
>>> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
>>> This results in the drop of interrupt from Guest-Hypervisor, where as
>>> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
>>> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
>>> are stuck in Idle thread and rcu soft lockups are seen.
>>>
>>> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
>>> trap handler is emulating the ISTATUS bit.
>>>
>>> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   arch/arm64/kvm/arch_timer.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>> index 27a6ec46803a..0b32d943d2d5 100644
>>> --- a/arch/arm64/kvm/arch_timer.c
>>> +++ b/arch/arm64/kvm/arch_timer.c
>>> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>>>   			      struct arch_timer_context *timer,
>>>   			      enum kvm_arch_timer_regs treg);
>>>   static bool kvm_arch_timer_get_input_level(int vintid);
>>> +static u64 read_timer_ctl(struct arch_timer_context *timer);
>>>   
>>>   static struct irq_ops arch_timer_irq_ops = {
>>>   	.get_input_level = kvm_arch_timer_get_input_level,
>>> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
>>>   		return HRTIMER_RESTART;
>>>   	}
>>>   
>>> +	/* Timer emulated, emulate ISTATUS also */
>>> +	timer_set_ctl(ctx, read_timer_ctl(ctx));
>>
>> Why should we do that for non-NV2 configurations?
>>
>>>   	kvm_timer_update_irq(vcpu, true, ctx);
>>>   	return HRTIMER_NORESTART;
>>>   }
>>> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>   	trace_kvm_timer_emulate(ctx, should_fire);
>>>   
>>>   	if (should_fire != ctx->irq.level) {
>>> +		/* Timer emulated, emulate ISTATUS also */
>>> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
>>>   		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
>>>   		return;
>>>   	}
>>
>> I'm not overly keen on this. Yes, we can set the status bit there. But
>> conversely, the bit will not get cleared when the guest reprograms the
>> timer, and will take a full exit/entry cycle for it to appear.
>>
>> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
>> used to emulate something as dynamic as a timer.
>>
>> It is only with FEAT_ECV that we can solve this correctly by trapping
>> the counter/timer accesses and emulate them for the guest hypervisor.
>> I'd rather we add support for that, as I expect all the FEAT_NV2
>> implementations to have it (and hopefully FEAT_FGT as well).
> 
> So I went ahead and implemented some very basic FEAT_ECV support to
> correctly emulate the timers (trapping the CTL/CVAL accesses).
> 
> Performance dropped like a rock (~30% extra overhead) for L2
> exit-heavy workloads that are terminated in userspace, such as virtio.
> For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
> as we save/restore the timer context, and this is enough to make
> things visibly slower, even on a pretty fast machine.
> 
> I managed to get *some* performance back by satisfying CTL/CVAL reads
> very early on the exit path (a pretty common theme with NV). Which
> means we end-up needing something like what you have -- only a bit
> more complete. I came up with the following:
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 4945c5b96f05..a198a6211e2a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>   {
>   	int ret;
>   
> +	/*
> +	 * Paper over NV2 brokenness by publishing the interrupt status
> +	 * bit. This still results in a poor quality of emulation (guest
> +	 * writes will have no effect until the next exit).
> +	 *
> +	 * But hey, it's fast, right?
> +	 */
> +	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
> +	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
> +		u32 ctl = timer_get_ctl(timer_ctx);
> +
> +		if (new_level)
> +			ctl |= ARCH_TIMER_CTRL_IT_STAT;
> +		else
> +			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
> +
> +		timer_set_ctl(timer_ctx, ctl);
> +	}
> +
>   	timer_ctx->irq.level = new_level;
>   	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
>   				   timer_ctx->irq.level);
> 
> which reports the interrupt state in all cases.
> 
> Does this work for you?

Thanks Marc for the patch. I will try this and update at the earliest.

> 
> Thanks,
> 
> 	M.
> 

Thanks,
Ganapat

Re: [PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Ganapatrao Kulkarni]

On 02-01-2023 05:16 pm, Marc Zyngier wrote:
> On Thu, 29 Dec 2022 13:53:15 +0000,
> Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Wed, 24 Aug 2022 07:03:03 +0100,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
>>> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
>>> loaded timer.
>>>
>>> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
>>> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
>>> This results in the drop of interrupt from Guest-Hypervisor, where as
>>> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
>>> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
>>> are stuck in Idle thread and rcu soft lockups are seen.
>>>
>>> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
>>> trap handler is emulating the ISTATUS bit.
>>>
>>> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   arch/arm64/kvm/arch_timer.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>> index 27a6ec46803a..0b32d943d2d5 100644
>>> --- a/arch/arm64/kvm/arch_timer.c
>>> +++ b/arch/arm64/kvm/arch_timer.c
>>> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>>>   			      struct arch_timer_context *timer,
>>>   			      enum kvm_arch_timer_regs treg);
>>>   static bool kvm_arch_timer_get_input_level(int vintid);
>>> +static u64 read_timer_ctl(struct arch_timer_context *timer);
>>>   
>>>   static struct irq_ops arch_timer_irq_ops = {
>>>   	.get_input_level = kvm_arch_timer_get_input_level,
>>> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
>>>   		return HRTIMER_RESTART;
>>>   	}
>>>   
>>> +	/* Timer emulated, emulate ISTATUS also */
>>> +	timer_set_ctl(ctx, read_timer_ctl(ctx));
>>
>> Why should we do that for non-NV2 configurations?
>>
>>>   	kvm_timer_update_irq(vcpu, true, ctx);
>>>   	return HRTIMER_NORESTART;
>>>   }
>>> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>   	trace_kvm_timer_emulate(ctx, should_fire);
>>>   
>>>   	if (should_fire != ctx->irq.level) {
>>> +		/* Timer emulated, emulate ISTATUS also */
>>> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
>>>   		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
>>>   		return;
>>>   	}
>>
>> I'm not overly keen on this. Yes, we can set the status bit there. But
>> conversely, the bit will not get cleared when the guest reprograms the
>> timer, and will take a full exit/entry cycle for it to appear.
>>
>> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
>> used to emulate something as dynamic as a timer.
>>
>> It is only with FEAT_ECV that we can solve this correctly by trapping
>> the counter/timer accesses and emulate them for the guest hypervisor.
>> I'd rather we add support for that, as I expect all the FEAT_NV2
>> implementations to have it (and hopefully FEAT_FGT as well).
> 
> So I went ahead and implemented some very basic FEAT_ECV support to
> correctly emulate the timers (trapping the CTL/CVAL accesses).
> 
> Performance dropped like a rock (~30% extra overhead) for L2
> exit-heavy workloads that are terminated in userspace, such as virtio.
> For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
> as we save/restore the timer context, and this is enough to make
> things visibly slower, even on a pretty fast machine.
> 
> I managed to get *some* performance back by satisfying CTL/CVAL reads
> very early on the exit path (a pretty common theme with NV). Which
> means we end-up needing something like what you have -- only a bit
> more complete. I came up with the following:
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 4945c5b96f05..a198a6211e2a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>   {
>   	int ret;
>   
> +	/*
> +	 * Paper over NV2 brokenness by publishing the interrupt status
> +	 * bit. This still results in a poor quality of emulation (guest
> +	 * writes will have no effect until the next exit).
> +	 *
> +	 * But hey, it's fast, right?
> +	 */
> +	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
> +	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
> +		u32 ctl = timer_get_ctl(timer_ctx);
> +
> +		if (new_level)
> +			ctl |= ARCH_TIMER_CTRL_IT_STAT;
> +		else
> +			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
> +
> +		timer_set_ctl(timer_ctx, ctl);
> +	}
> +
>   	timer_ctx->irq.level = new_level;
>   	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
>   				   timer_ctx->irq.level);
> 
> which reports the interrupt state in all cases.
> 
> Does this work for you?

Thanks Marc for the patch. I will try this and update at the earliest.

> 
> Thanks,
> 
> 	M.
> 

Thanks,
Ganapat

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

Re: [PATCH 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Ganapatrao Kulkarni]

On 29-12-2022 11:12 pm, Marc Zyngier wrote:
> On Wed, 24 Aug 2022 07:03:04 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> In NV case, Shadow stage 2 page table is created using host hypervisor
>> page table configuration like page size, block size etc. Also, the shadow
>> stage 2 table uses block level mapping if the Guest Hypervisor IPA is
>> backed by the THP pages. However, this is resulting in illegal mapping of
>> NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
>> hypervisor are configured with different pagesize.
>>
>> Adding fix to avoid block level mapping in stage 2 mapping if
>> max_map_size is smaller than the block size.
>>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>> ---
>>   arch/arm64/kvm/mmu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index 6caa48da1b2e..3d4b53f153a1 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>   	 * backed by a THP and thus use block mapping if possible.
>>   	 */
>>   	if (vma_pagesize == PAGE_SIZE &&
>> -	    !(max_map_size == PAGE_SIZE || device)) {
>> +	    !(max_map_size < PMD_SIZE || device)) {
>>   		if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
>>   			vma_pagesize = fault_granule;
>>   		else
> 
> That's quite a nice catch. I guess this was the main issue with
> running 64kB L1 on a 4kB L0? Now, I'm not that fond of the fix itself,
> and I think max_map_size should always represent something that is a
> valid size *on the host*, specially when outside of NV-specific code.
> 

Thanks Marc, yes this patch was to fix the issue seen with L1 64K and L0 
4K page size.

> How about something like this instead:
> 
> @@ -1346,6 +1346,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>   		 * table uses at least as big a mapping.
>   		 */
>   		max_map_size = min(kvm_s2_trans_size(nested), max_map_size);
> +
> +		if (max_map_size >= PMD_SIZE && max_map_size < PUD_SIZE)
> +			max_map_size = PMD_SIZE;
> +		else if (max_map_size >= PAGE_SIZE && max_map_size < PMD_SIZE)
> +			max_map_size = PAGE_SIZE;
>   	}
>   
>   	vma_pagesize = min(vma_pagesize, max_map_size);
> 
> 
> Admittedly, this is a lot uglier than your fix. But it keep the nested
> horror localised, and doesn't risk being reverted by accident by
> people who would not take NV into account (can't blame them, really).
> 
> Can you please give it a go?

Sure, I will try this and update at the earliest.
> 
> Thanks,
> 
> 	M.
> 

Thanks,
Ganapat

Re: [PATCH 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Ganapatrao Kulkarni]

On 29-12-2022 11:12 pm, Marc Zyngier wrote:
> On Wed, 24 Aug 2022 07:03:04 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> In NV case, Shadow stage 2 page table is created using host hypervisor
>> page table configuration like page size, block size etc. Also, the shadow
>> stage 2 table uses block level mapping if the Guest Hypervisor IPA is
>> backed by the THP pages. However, this is resulting in illegal mapping of
>> NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
>> hypervisor are configured with different pagesize.
>>
>> Adding fix to avoid block level mapping in stage 2 mapping if
>> max_map_size is smaller than the block size.
>>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>> ---
>>   arch/arm64/kvm/mmu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index 6caa48da1b2e..3d4b53f153a1 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>   	 * backed by a THP and thus use block mapping if possible.
>>   	 */
>>   	if (vma_pagesize == PAGE_SIZE &&
>> -	    !(max_map_size == PAGE_SIZE || device)) {
>> +	    !(max_map_size < PMD_SIZE || device)) {
>>   		if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
>>   			vma_pagesize = fault_granule;
>>   		else
> 
> That's quite a nice catch. I guess this was the main issue with
> running 64kB L1 on a 4kB L0? Now, I'm not that fond of the fix itself,
> and I think max_map_size should always represent something that is a
> valid size *on the host*, specially when outside of NV-specific code.
> 

Thanks Marc, yes this patch was to fix the issue seen with L1 64K and L0 
4K page size.

> How about something like this instead:
> 
> @@ -1346,6 +1346,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>   		 * table uses at least as big a mapping.
>   		 */
>   		max_map_size = min(kvm_s2_trans_size(nested), max_map_size);
> +
> +		if (max_map_size >= PMD_SIZE && max_map_size < PUD_SIZE)
> +			max_map_size = PMD_SIZE;
> +		else if (max_map_size >= PAGE_SIZE && max_map_size < PMD_SIZE)
> +			max_map_size = PAGE_SIZE;
>   	}
>   
>   	vma_pagesize = min(vma_pagesize, max_map_size);
> 
> 
> Admittedly, this is a lot uglier than your fix. But it keep the nested
> horror localised, and doesn't risk being reverted by accident by
> people who would not take NV into account (can't blame them, really).
> 
> Can you please give it a go?

Sure, I will try this and update at the earliest.
> 
> Thanks,
> 
> 	M.
> 

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

Re: [PATCH 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Ganapatrao Kulkarni]

On 29-12-2022 11:12 pm, Marc Zyngier wrote:
> On Wed, 24 Aug 2022 07:03:04 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> In NV case, Shadow stage 2 page table is created using host hypervisor
>> page table configuration like page size, block size etc. Also, the shadow
>> stage 2 table uses block level mapping if the Guest Hypervisor IPA is
>> backed by the THP pages. However, this is resulting in illegal mapping of
>> NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
>> hypervisor are configured with different pagesize.
>>
>> Adding fix to avoid block level mapping in stage 2 mapping if
>> max_map_size is smaller than the block size.
>>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>> ---
>>   arch/arm64/kvm/mmu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index 6caa48da1b2e..3d4b53f153a1 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>   	 * backed by a THP and thus use block mapping if possible.
>>   	 */
>>   	if (vma_pagesize == PAGE_SIZE &&
>> -	    !(max_map_size == PAGE_SIZE || device)) {
>> +	    !(max_map_size < PMD_SIZE || device)) {
>>   		if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
>>   			vma_pagesize = fault_granule;
>>   		else
> 
> That's quite a nice catch. I guess this was the main issue with
> running 64kB L1 on a 4kB L0? Now, I'm not that fond of the fix itself,
> and I think max_map_size should always represent something that is a
> valid size *on the host*, specially when outside of NV-specific code.
> 

Thanks Marc, yes this patch was to fix the issue seen with L1 64K and L0 
4K page size.

> How about something like this instead:
> 
> @@ -1346,6 +1346,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>   		 * table uses at least as big a mapping.
>   		 */
>   		max_map_size = min(kvm_s2_trans_size(nested), max_map_size);
> +
> +		if (max_map_size >= PMD_SIZE && max_map_size < PUD_SIZE)
> +			max_map_size = PMD_SIZE;
> +		else if (max_map_size >= PAGE_SIZE && max_map_size < PMD_SIZE)
> +			max_map_size = PAGE_SIZE;
>   	}
>   
>   	vma_pagesize = min(vma_pagesize, max_map_size);
> 
> 
> Admittedly, this is a lot uglier than your fix. But it keep the nested
> horror localised, and doesn't risk being reverted by accident by
> people who would not take NV into account (can't blame them, really).
> 
> Can you please give it a go?

Sure, I will try this and update at the earliest.
> 
> Thanks,
> 
> 	M.
> 

Thanks,
Ganapat

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

Re: [PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Ganapatrao Kulkarni]

Hi Marc,

On 29-12-2022 06:30 pm, Marc Zyngier wrote:
> On Wed, 24 Aug 2022 07:03:02 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> From: D Scott Phillips <scott@os.amperecomputing.com>
>>
>> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
>> timer is being emulated.
>>
>> While the CPU is executing in L1 context, the L2 timers are emulated using
>> host hrtimer. When the delta of cval and current time reaches zero, the
>> vtimer interrupt is fired/forwarded to L2, however the emulation function
>> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
>> set to now, triggering hrtimer to fire immediately and resulting in a
>> continuous trigger of hrtimer and endless looping in the timer emulation.
>>
>> Adding a fix to avoid restarting of the hrtimer if the interrupt is
>> already fired.
>>
>> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>> ---
>>   arch/arm64/kvm/arch_timer.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>> index 2371796b1ab5..27a6ec46803a 100644
>> --- a/arch/arm64/kvm/arch_timer.c
>> +++ b/arch/arm64/kvm/arch_timer.c
>> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>   		return;
>>   	}
>>   
>> -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>> +	if (!ctx->irq.level)
>> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>>   }
>>   
>>   static void timer_save_state(struct arch_timer_context *ctx)
> 
> I think this is a regression introduced by bee038a67487 ("KVM:
> arm/arm64: Rework the timer code to use a timer_map"), and you can see
> it because the comment in this function doesn't make much sense
> anymore.

OK, check was removed while rework in bee038a67487.
> 
> Does the following work for you, mostly restoring the original code?

Below diff too works and avoids the unnecessary soft timer restarts with 
zero delta.
> 
> Thanks,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index ad2a5df88810..4945c5b96f05 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
>   	 * scheduled for the future.  If the timer cannot fire at all,
>   	 * then we also don't need a soft timer.
>   	 */
> -	if (!kvm_timer_irq_can_fire(ctx)) {
> +	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {

Now, aligns to comment.
>   		soft_timer_cancel(&ctx->hrtimer);
>   		return;
>   	}
> 

Shall I resend this patch as regression fix of bee038a67487?

Thanks,
Ganapat

Re: [PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Ganapatrao Kulkarni]

Hi Marc,

On 29-12-2022 06:30 pm, Marc Zyngier wrote:
> On Wed, 24 Aug 2022 07:03:02 +0100,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> From: D Scott Phillips <scott@os.amperecomputing.com>
>>
>> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
>> timer is being emulated.
>>
>> While the CPU is executing in L1 context, the L2 timers are emulated using
>> host hrtimer. When the delta of cval and current time reaches zero, the
>> vtimer interrupt is fired/forwarded to L2, however the emulation function
>> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
>> set to now, triggering hrtimer to fire immediately and resulting in a
>> continuous trigger of hrtimer and endless looping in the timer emulation.
>>
>> Adding a fix to avoid restarting of the hrtimer if the interrupt is
>> already fired.
>>
>> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>> ---
>>   arch/arm64/kvm/arch_timer.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>> index 2371796b1ab5..27a6ec46803a 100644
>> --- a/arch/arm64/kvm/arch_timer.c
>> +++ b/arch/arm64/kvm/arch_timer.c
>> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>   		return;
>>   	}
>>   
>> -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>> +	if (!ctx->irq.level)
>> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>>   }
>>   
>>   static void timer_save_state(struct arch_timer_context *ctx)
> 
> I think this is a regression introduced by bee038a67487 ("KVM:
> arm/arm64: Rework the timer code to use a timer_map"), and you can see
> it because the comment in this function doesn't make much sense
> anymore.

OK, check was removed while rework in bee038a67487.
> 
> Does the following work for you, mostly restoring the original code?

Below diff too works and avoids the unnecessary soft timer restarts with 
zero delta.
> 
> Thanks,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index ad2a5df88810..4945c5b96f05 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
>   	 * scheduled for the future.  If the timer cannot fire at all,
>   	 * then we also don't need a soft timer.
>   	 */
> -	if (!kvm_timer_irq_can_fire(ctx)) {
> +	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {

Now, aligns to comment.
>   		soft_timer_cancel(&ctx->hrtimer);
>   		return;
>   	}
> 

Shall I resend this patch as regression fix of bee038a67487?

Thanks,
Ganapat

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

Re: [PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Marc Zyngier]

On Mon, 09 Jan 2023 12:25:13 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> 
> Hi Marc,
> 
> On 29-12-2022 06:30 pm, Marc Zyngier wrote:
> > On Wed, 24 Aug 2022 07:03:02 +0100,
> > Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> >> 
> >> From: D Scott Phillips <scott@os.amperecomputing.com>
> >> 
> >> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
> >> timer is being emulated.
> >> 
> >> While the CPU is executing in L1 context, the L2 timers are emulated using
> >> host hrtimer. When the delta of cval and current time reaches zero, the
> >> vtimer interrupt is fired/forwarded to L2, however the emulation function
> >> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
> >> set to now, triggering hrtimer to fire immediately and resulting in a
> >> continuous trigger of hrtimer and endless looping in the timer emulation.
> >> 
> >> Adding a fix to avoid restarting of the hrtimer if the interrupt is
> >> already fired.
> >> 
> >> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> >> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> >> ---
> >>   arch/arm64/kvm/arch_timer.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> >> index 2371796b1ab5..27a6ec46803a 100644
> >> --- a/arch/arm64/kvm/arch_timer.c
> >> +++ b/arch/arm64/kvm/arch_timer.c
> >> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >>   		return;
> >>   	}
> >>   -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
> >> +	if (!ctx->irq.level)
> >> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
> >>   }
> >>     static void timer_save_state(struct arch_timer_context *ctx)
> > 
> > I think this is a regression introduced by bee038a67487 ("KVM:
> > arm/arm64: Rework the timer code to use a timer_map"), and you can see
> > it because the comment in this function doesn't make much sense
> > anymore.
> 
> OK, check was removed while rework in bee038a67487.
> > 
> > Does the following work for you, mostly restoring the original code?
> 
> Below diff too works and avoids the unnecessary soft timer restarts
> with zero delta.
> > 
> > Thanks,
> > 
> > 	M.
> > 
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index ad2a5df88810..4945c5b96f05 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >   	 * scheduled for the future.  If the timer cannot fire at all,
> >   	 * then we also don't need a soft timer.
> >   	 */
> > -	if (!kvm_timer_irq_can_fire(ctx)) {
> > +	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {
> 
> Now, aligns to comment.
> >   		soft_timer_cancel(&ctx->hrtimer);
> >   		return;
> >   	}
> > 
> 
> Shall I resend this patch as regression fix of bee038a67487?

I already have a patch written for this at [1], also getting rid of
the soft_timer_cancel() call in the process (as it doesn't make much
sense either).

Please give it a go if you have a chance (though the whole branch
might be of interest to you...).

Thanks,

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/nv-6.2-WIP&id=effdcfa175c374a1740f60642d221ad2e930c978

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

Re: [PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Marc Zyngier]

On Mon, 09 Jan 2023 12:25:13 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> 
> Hi Marc,
> 
> On 29-12-2022 06:30 pm, Marc Zyngier wrote:
> > On Wed, 24 Aug 2022 07:03:02 +0100,
> > Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> >> 
> >> From: D Scott Phillips <scott@os.amperecomputing.com>
> >> 
> >> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
> >> timer is being emulated.
> >> 
> >> While the CPU is executing in L1 context, the L2 timers are emulated using
> >> host hrtimer. When the delta of cval and current time reaches zero, the
> >> vtimer interrupt is fired/forwarded to L2, however the emulation function
> >> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
> >> set to now, triggering hrtimer to fire immediately and resulting in a
> >> continuous trigger of hrtimer and endless looping in the timer emulation.
> >> 
> >> Adding a fix to avoid restarting of the hrtimer if the interrupt is
> >> already fired.
> >> 
> >> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
> >> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> >> ---
> >>   arch/arm64/kvm/arch_timer.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> >> index 2371796b1ab5..27a6ec46803a 100644
> >> --- a/arch/arm64/kvm/arch_timer.c
> >> +++ b/arch/arm64/kvm/arch_timer.c
> >> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >>   		return;
> >>   	}
> >>   -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
> >> +	if (!ctx->irq.level)
> >> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
> >>   }
> >>     static void timer_save_state(struct arch_timer_context *ctx)
> > 
> > I think this is a regression introduced by bee038a67487 ("KVM:
> > arm/arm64: Rework the timer code to use a timer_map"), and you can see
> > it because the comment in this function doesn't make much sense
> > anymore.
> 
> OK, check was removed while rework in bee038a67487.
> > 
> > Does the following work for you, mostly restoring the original code?
> 
> Below diff too works and avoids the unnecessary soft timer restarts
> with zero delta.
> > 
> > Thanks,
> > 
> > 	M.
> > 
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index ad2a5df88810..4945c5b96f05 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >   	 * scheduled for the future.  If the timer cannot fire at all,
> >   	 * then we also don't need a soft timer.
> >   	 */
> > -	if (!kvm_timer_irq_can_fire(ctx)) {
> > +	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {
> 
> Now, aligns to comment.
> >   		soft_timer_cancel(&ctx->hrtimer);
> >   		return;
> >   	}
> > 
> 
> Shall I resend this patch as regression fix of bee038a67487?

I already have a patch written for this at [1], also getting rid of
the soft_timer_cancel() call in the process (as it doesn't make much
sense either).

Please give it a go if you have a chance (though the whole branch
might be of interest to you...).

Thanks,

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/nv-6.2-WIP&id=effdcfa175c374a1740f60642d221ad2e930c978

-- 
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 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Ganapatrao Kulkarni]

On 03-01-2023 09:56 am, Ganapatrao Kulkarni wrote:
> 
> 
> On 29-12-2022 11:12 pm, Marc Zyngier wrote:
>> On Wed, 24 Aug 2022 07:03:04 +0100,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> In NV case, Shadow stage 2 page table is created using host hypervisor
>>> page table configuration like page size, block size etc. Also, the 
>>> shadow
>>> stage 2 table uses block level mapping if the Guest Hypervisor IPA is
>>> backed by the THP pages. However, this is resulting in illegal 
>>> mapping of
>>> NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
>>> hypervisor are configured with different pagesize.
>>>
>>> Adding fix to avoid block level mapping in stage 2 mapping if
>>> max_map_size is smaller than the block size.
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   arch/arm64/kvm/mmu.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>> index 6caa48da1b2e..3d4b53f153a1 100644
>>> --- a/arch/arm64/kvm/mmu.c
>>> +++ b/arch/arm64/kvm/mmu.c
>>> @@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu 
>>> *vcpu, phys_addr_t fault_ipa,
>>>        * backed by a THP and thus use block mapping if possible.
>>>        */
>>>       if (vma_pagesize == PAGE_SIZE &&
>>> -        !(max_map_size == PAGE_SIZE || device)) {
>>> +        !(max_map_size < PMD_SIZE || device)) {
>>>           if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
>>>               vma_pagesize = fault_granule;
>>>           else
>>
>> That's quite a nice catch. I guess this was the main issue with
>> running 64kB L1 on a 4kB L0? Now, I'm not that fond of the fix itself,
>> and I think max_map_size should always represent something that is a
>> valid size *on the host*, specially when outside of NV-specific code.
>>
> 
> Thanks Marc, yes this patch was to fix the issue seen with L1 64K and L0 
> 4K page size.
> 
>> How about something like this instead:
>>
>> @@ -1346,6 +1346,11 @@ static int user_mem_abort(struct kvm_vcpu 
>> *vcpu, phys_addr_t fault_ipa,
>>            * table uses at least as big a mapping.
>>            */
>>           max_map_size = min(kvm_s2_trans_size(nested), max_map_size);
>> +

Would be good to add brief comment about the changes.
>> +        if (max_map_size >= PMD_SIZE && max_map_size < PUD_SIZE)
>> +            max_map_size = PMD_SIZE;
>> +        else if (max_map_size >= PAGE_SIZE && max_map_size < PMD_SIZE)
>> +            max_map_size = PAGE_SIZE;
>>       }
>>       vma_pagesize = min(vma_pagesize, max_map_size);
>>
>>
>> Admittedly, this is a lot uglier than your fix. But it keep the nested
>> horror localised, and doesn't risk being reverted by accident by
>> people who would not take NV into account (can't blame them, really).

I agree, it makes sense to keep the changes specific to NV under nested 
scope.

>>
>> Can you please give it a go?

This diff works as well.
> 
> Sure, I will try this and update at the earliest.
>>
>> Thanks,
>>
>>     M.
>>
> 
> Thanks,
> Ganapat

Thanks,
Ganapat

Re: [PATCH 3/3] KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than block size.

[Ganapatrao Kulkarni]

On 03-01-2023 09:56 am, Ganapatrao Kulkarni wrote:
> 
> 
> On 29-12-2022 11:12 pm, Marc Zyngier wrote:
>> On Wed, 24 Aug 2022 07:03:04 +0100,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> In NV case, Shadow stage 2 page table is created using host hypervisor
>>> page table configuration like page size, block size etc. Also, the 
>>> shadow
>>> stage 2 table uses block level mapping if the Guest Hypervisor IPA is
>>> backed by the THP pages. However, this is resulting in illegal 
>>> mapping of
>>> NestedVM IPA to Host Hypervisor PA, when Guest Hypervisor and Host
>>> hypervisor are configured with different pagesize.
>>>
>>> Adding fix to avoid block level mapping in stage 2 mapping if
>>> max_map_size is smaller than the block size.
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   arch/arm64/kvm/mmu.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>> index 6caa48da1b2e..3d4b53f153a1 100644
>>> --- a/arch/arm64/kvm/mmu.c
>>> +++ b/arch/arm64/kvm/mmu.c
>>> @@ -1304,7 +1304,7 @@ static int user_mem_abort(struct kvm_vcpu 
>>> *vcpu, phys_addr_t fault_ipa,
>>>        * backed by a THP and thus use block mapping if possible.
>>>        */
>>>       if (vma_pagesize == PAGE_SIZE &&
>>> -        !(max_map_size == PAGE_SIZE || device)) {
>>> +        !(max_map_size < PMD_SIZE || device)) {
>>>           if (fault_status == FSC_PERM && fault_granule > PAGE_SIZE)
>>>               vma_pagesize = fault_granule;
>>>           else
>>
>> That's quite a nice catch. I guess this was the main issue with
>> running 64kB L1 on a 4kB L0? Now, I'm not that fond of the fix itself,
>> and I think max_map_size should always represent something that is a
>> valid size *on the host*, specially when outside of NV-specific code.
>>
> 
> Thanks Marc, yes this patch was to fix the issue seen with L1 64K and L0 
> 4K page size.
> 
>> How about something like this instead:
>>
>> @@ -1346,6 +1346,11 @@ static int user_mem_abort(struct kvm_vcpu 
>> *vcpu, phys_addr_t fault_ipa,
>>            * table uses at least as big a mapping.
>>            */
>>           max_map_size = min(kvm_s2_trans_size(nested), max_map_size);
>> +

Would be good to add brief comment about the changes.
>> +        if (max_map_size >= PMD_SIZE && max_map_size < PUD_SIZE)
>> +            max_map_size = PMD_SIZE;
>> +        else if (max_map_size >= PAGE_SIZE && max_map_size < PMD_SIZE)
>> +            max_map_size = PAGE_SIZE;
>>       }
>>       vma_pagesize = min(vma_pagesize, max_map_size);
>>
>>
>> Admittedly, this is a lot uglier than your fix. But it keep the nested
>> horror localised, and doesn't risk being reverted by accident by
>> people who would not take NV into account (can't blame them, really).

I agree, it makes sense to keep the changes specific to NV under nested 
scope.

>>
>> Can you please give it a go?

This diff works as well.
> 
> Sure, I will try this and update at the earliest.
>>
>> Thanks,
>>
>>     M.
>>
> 
> Thanks,
> Ganapat

Thanks,
Ganapat

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

Re: [PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Ganapatrao Kulkarni]

On 09-01-2023 07:14 pm, Marc Zyngier wrote:
> On Mon, 09 Jan 2023 12:25:13 +0000,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>>
>> Hi Marc,
>>
>> On 29-12-2022 06:30 pm, Marc Zyngier wrote:
>>> On Wed, 24 Aug 2022 07:03:02 +0100,
>>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>>
>>>> From: D Scott Phillips <scott@os.amperecomputing.com>
>>>>
>>>> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
>>>> timer is being emulated.
>>>>
>>>> While the CPU is executing in L1 context, the L2 timers are emulated using
>>>> host hrtimer. When the delta of cval and current time reaches zero, the
>>>> vtimer interrupt is fired/forwarded to L2, however the emulation function
>>>> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
>>>> set to now, triggering hrtimer to fire immediately and resulting in a
>>>> continuous trigger of hrtimer and endless looping in the timer emulation.
>>>>
>>>> Adding a fix to avoid restarting of the hrtimer if the interrupt is
>>>> already fired.
>>>>
>>>> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
>>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>>> ---
>>>>    arch/arm64/kvm/arch_timer.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>>> index 2371796b1ab5..27a6ec46803a 100644
>>>> --- a/arch/arm64/kvm/arch_timer.c
>>>> +++ b/arch/arm64/kvm/arch_timer.c
>>>> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>>    		return;
>>>>    	}
>>>>    -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>>>> +	if (!ctx->irq.level)
>>>> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>>>>    }
>>>>      static void timer_save_state(struct arch_timer_context *ctx)
>>>
>>> I think this is a regression introduced by bee038a67487 ("KVM:
>>> arm/arm64: Rework the timer code to use a timer_map"), and you can see
>>> it because the comment in this function doesn't make much sense
>>> anymore.
>>
>> OK, check was removed while rework in bee038a67487.
>>>
>>> Does the following work for you, mostly restoring the original code?
>>
>> Below diff too works and avoids the unnecessary soft timer restarts
>> with zero delta.
>>>
>>> Thanks,
>>>
>>> 	M.
>>>
>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>> index ad2a5df88810..4945c5b96f05 100644
>>> --- a/arch/arm64/kvm/arch_timer.c
>>> +++ b/arch/arm64/kvm/arch_timer.c
>>> @@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>    	 * scheduled for the future.  If the timer cannot fire at all,
>>>    	 * then we also don't need a soft timer.
>>>    	 */
>>> -	if (!kvm_timer_irq_can_fire(ctx)) {
>>> +	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {
>>
>> Now, aligns to comment.
>>>    		soft_timer_cancel(&ctx->hrtimer);
>>>    		return;
>>>    	}
>>>
>>
>> Shall I resend this patch as regression fix of bee038a67487?
> 
> I already have a patch written for this at [1], also getting rid of
> the soft_timer_cancel() call in the process (as it doesn't make much
> sense either).

OK, thanks.
> 
> Please give it a go if you have a chance (though the whole branch
> might be of interest to you...).

Sure, I will try this branch.
> 
> Thanks,
> 
> 	M.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/nv-6.2-WIP&id=effdcfa175c374a1740f60642d221ad2e930c978
> 

Thanks,
Ganapat

Re: [PATCH 1/3] KVM: arm64: nv: only emulate timers that have not yet fired

[Ganapatrao Kulkarni]

On 09-01-2023 07:14 pm, Marc Zyngier wrote:
> On Mon, 09 Jan 2023 12:25:13 +0000,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>>
>> Hi Marc,
>>
>> On 29-12-2022 06:30 pm, Marc Zyngier wrote:
>>> On Wed, 24 Aug 2022 07:03:02 +0100,
>>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>>
>>>> From: D Scott Phillips <scott@os.amperecomputing.com>
>>>>
>>>> The timer emulation logic goes into an infinite loop when the NestedVM(L2)
>>>> timer is being emulated.
>>>>
>>>> While the CPU is executing in L1 context, the L2 timers are emulated using
>>>> host hrtimer. When the delta of cval and current time reaches zero, the
>>>> vtimer interrupt is fired/forwarded to L2, however the emulation function
>>>> in Host-Hypervisor(L0) is still restarting the hrtimer with an expiry time
>>>> set to now, triggering hrtimer to fire immediately and resulting in a
>>>> continuous trigger of hrtimer and endless looping in the timer emulation.
>>>>
>>>> Adding a fix to avoid restarting of the hrtimer if the interrupt is
>>>> already fired.
>>>>
>>>> Signed-off-by: D Scott Phillips <scott@os.amperecomputing.com>
>>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>>> ---
>>>>    arch/arm64/kvm/arch_timer.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>>> index 2371796b1ab5..27a6ec46803a 100644
>>>> --- a/arch/arm64/kvm/arch_timer.c
>>>> +++ b/arch/arm64/kvm/arch_timer.c
>>>> @@ -472,7 +472,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>>    		return;
>>>>    	}
>>>>    -	soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>>>> +	if (!ctx->irq.level)
>>>> +		soft_timer_start(&ctx->hrtimer, kvm_timer_compute_delta(ctx));
>>>>    }
>>>>      static void timer_save_state(struct arch_timer_context *ctx)
>>>
>>> I think this is a regression introduced by bee038a67487 ("KVM:
>>> arm/arm64: Rework the timer code to use a timer_map"), and you can see
>>> it because the comment in this function doesn't make much sense
>>> anymore.
>>
>> OK, check was removed while rework in bee038a67487.
>>>
>>> Does the following work for you, mostly restoring the original code?
>>
>> Below diff too works and avoids the unnecessary soft timer restarts
>> with zero delta.
>>>
>>> Thanks,
>>>
>>> 	M.
>>>
>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>> index ad2a5df88810..4945c5b96f05 100644
>>> --- a/arch/arm64/kvm/arch_timer.c
>>> +++ b/arch/arm64/kvm/arch_timer.c
>>> @@ -480,7 +480,7 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>    	 * scheduled for the future.  If the timer cannot fire at all,
>>>    	 * then we also don't need a soft timer.
>>>    	 */
>>> -	if (!kvm_timer_irq_can_fire(ctx)) {
>>> +	if (should_fire || !kvm_timer_irq_can_fire(ctx)) {
>>
>> Now, aligns to comment.
>>>    		soft_timer_cancel(&ctx->hrtimer);
>>>    		return;
>>>    	}
>>>
>>
>> Shall I resend this patch as regression fix of bee038a67487?
> 
> I already have a patch written for this at [1], also getting rid of
> the soft_timer_cancel() call in the process (as it doesn't make much
> sense either).

OK, thanks.
> 
> Please give it a go if you have a chance (though the whole branch
> might be of interest to you...).

Sure, I will try this branch.
> 
> Thanks,
> 
> 	M.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=kvm-arm64/nv-6.2-WIP&id=effdcfa175c374a1740f60642d221ad2e930c978
> 

Thanks,
Ganapat

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

Re: [PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Ganapatrao Kulkarni]

On 02-01-2023 05:16 pm, Marc Zyngier wrote:
> On Thu, 29 Dec 2022 13:53:15 +0000,
> Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Wed, 24 Aug 2022 07:03:03 +0100,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
>>> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
>>> loaded timer.
>>>
>>> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
>>> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
>>> This results in the drop of interrupt from Guest-Hypervisor, where as
>>> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
>>> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
>>> are stuck in Idle thread and rcu soft lockups are seen.
>>>
>>> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
>>> trap handler is emulating the ISTATUS bit.
>>>
>>> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   arch/arm64/kvm/arch_timer.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>> index 27a6ec46803a..0b32d943d2d5 100644
>>> --- a/arch/arm64/kvm/arch_timer.c
>>> +++ b/arch/arm64/kvm/arch_timer.c
>>> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>>>   			      struct arch_timer_context *timer,
>>>   			      enum kvm_arch_timer_regs treg);
>>>   static bool kvm_arch_timer_get_input_level(int vintid);
>>> +static u64 read_timer_ctl(struct arch_timer_context *timer);
>>>   
>>>   static struct irq_ops arch_timer_irq_ops = {
>>>   	.get_input_level = kvm_arch_timer_get_input_level,
>>> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
>>>   		return HRTIMER_RESTART;
>>>   	}
>>>   
>>> +	/* Timer emulated, emulate ISTATUS also */
>>> +	timer_set_ctl(ctx, read_timer_ctl(ctx));
>>
>> Why should we do that for non-NV2 configurations?
>>
>>>   	kvm_timer_update_irq(vcpu, true, ctx);
>>>   	return HRTIMER_NORESTART;
>>>   }
>>> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>   	trace_kvm_timer_emulate(ctx, should_fire);
>>>   
>>>   	if (should_fire != ctx->irq.level) {
>>> +		/* Timer emulated, emulate ISTATUS also */
>>> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
>>>   		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
>>>   		return;
>>>   	}
>>
>> I'm not overly keen on this. Yes, we can set the status bit there. But
>> conversely, the bit will not get cleared when the guest reprograms the
>> timer, and will take a full exit/entry cycle for it to appear.
>>
>> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
>> used to emulate something as dynamic as a timer.
>>
>> It is only with FEAT_ECV that we can solve this correctly by trapping
>> the counter/timer accesses and emulate them for the guest hypervisor.
>> I'd rather we add support for that, as I expect all the FEAT_NV2
>> implementations to have it (and hopefully FEAT_FGT as well).
> 
> So I went ahead and implemented some very basic FEAT_ECV support to
> correctly emulate the timers (trapping the CTL/CVAL accesses).
> 
> Performance dropped like a rock (~30% extra overhead) for L2
> exit-heavy workloads that are terminated in userspace, such as virtio.
> For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
> as we save/restore the timer context, and this is enough to make
> things visibly slower, even on a pretty fast machine.
> 
> I managed to get *some* performance back by satisfying CTL/CVAL reads
> very early on the exit path (a pretty common theme with NV). Which
> means we end-up needing something like what you have -- only a bit
> more complete. I came up with the following:

Yes it is more appropriate, this moves ISTATUS update to single place.
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 4945c5b96f05..a198a6211e2a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>   {
>   	int ret;
>   
> +	/*
> +	 * Paper over NV2 brokenness by publishing the interrupt status
> +	 * bit. This still results in a poor quality of emulation (guest
> +	 * writes will have no effect until the next exit).
> +	 *
> +	 * But hey, it's fast, right?
> +	 */
> +	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
> +	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
> +		u32 ctl = timer_get_ctl(timer_ctx);
> +
> +		if (new_level)
> +			ctl |= ARCH_TIMER_CTRL_IT_STAT;
> +		else
> +			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
> +
> +		timer_set_ctl(timer_ctx, ctl);
> +	}
> +
>   	timer_ctx->irq.level = new_level;
>   	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
>   				   timer_ctx->irq.level);
> 
> which reports the interrupt state in all cases.
> 
> Does this work for you?

This works.
Are you going to pull this diff/patch in to your 6.2-nv tree? or you 
want me to send an updated patch?

> 
> Thanks,
> 
> 	M.
> 

Thanks,
Ganapat


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

Re: [PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Ganapatrao Kulkarni]

On 02-01-2023 05:16 pm, Marc Zyngier wrote:
> On Thu, 29 Dec 2022 13:53:15 +0000,
> Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Wed, 24 Aug 2022 07:03:03 +0100,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
>>> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
>>> loaded timer.
>>>
>>> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
>>> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
>>> This results in the drop of interrupt from Guest-Hypervisor, where as
>>> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
>>> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
>>> are stuck in Idle thread and rcu soft lockups are seen.
>>>
>>> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
>>> trap handler is emulating the ISTATUS bit.
>>>
>>> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
>>>
>>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
>>> ---
>>>   arch/arm64/kvm/arch_timer.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
>>> index 27a6ec46803a..0b32d943d2d5 100644
>>> --- a/arch/arm64/kvm/arch_timer.c
>>> +++ b/arch/arm64/kvm/arch_timer.c
>>> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
>>>   			      struct arch_timer_context *timer,
>>>   			      enum kvm_arch_timer_regs treg);
>>>   static bool kvm_arch_timer_get_input_level(int vintid);
>>> +static u64 read_timer_ctl(struct arch_timer_context *timer);
>>>   
>>>   static struct irq_ops arch_timer_irq_ops = {
>>>   	.get_input_level = kvm_arch_timer_get_input_level,
>>> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
>>>   		return HRTIMER_RESTART;
>>>   	}
>>>   
>>> +	/* Timer emulated, emulate ISTATUS also */
>>> +	timer_set_ctl(ctx, read_timer_ctl(ctx));
>>
>> Why should we do that for non-NV2 configurations?
>>
>>>   	kvm_timer_update_irq(vcpu, true, ctx);
>>>   	return HRTIMER_NORESTART;
>>>   }
>>> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
>>>   	trace_kvm_timer_emulate(ctx, should_fire);
>>>   
>>>   	if (should_fire != ctx->irq.level) {
>>> +		/* Timer emulated, emulate ISTATUS also */
>>> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
>>>   		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
>>>   		return;
>>>   	}
>>
>> I'm not overly keen on this. Yes, we can set the status bit there. But
>> conversely, the bit will not get cleared when the guest reprograms the
>> timer, and will take a full exit/entry cycle for it to appear.
>>
>> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
>> used to emulate something as dynamic as a timer.
>>
>> It is only with FEAT_ECV that we can solve this correctly by trapping
>> the counter/timer accesses and emulate them for the guest hypervisor.
>> I'd rather we add support for that, as I expect all the FEAT_NV2
>> implementations to have it (and hopefully FEAT_FGT as well).
> 
> So I went ahead and implemented some very basic FEAT_ECV support to
> correctly emulate the timers (trapping the CTL/CVAL accesses).
> 
> Performance dropped like a rock (~30% extra overhead) for L2
> exit-heavy workloads that are terminated in userspace, such as virtio.
> For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
> as we save/restore the timer context, and this is enough to make
> things visibly slower, even on a pretty fast machine.
> 
> I managed to get *some* performance back by satisfying CTL/CVAL reads
> very early on the exit path (a pretty common theme with NV). Which
> means we end-up needing something like what you have -- only a bit
> more complete. I came up with the following:

Yes it is more appropriate, this moves ISTATUS update to single place.
> 
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 4945c5b96f05..a198a6211e2a 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
>   {
>   	int ret;
>   
> +	/*
> +	 * Paper over NV2 brokenness by publishing the interrupt status
> +	 * bit. This still results in a poor quality of emulation (guest
> +	 * writes will have no effect until the next exit).
> +	 *
> +	 * But hey, it's fast, right?
> +	 */
> +	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
> +	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
> +		u32 ctl = timer_get_ctl(timer_ctx);
> +
> +		if (new_level)
> +			ctl |= ARCH_TIMER_CTRL_IT_STAT;
> +		else
> +			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
> +
> +		timer_set_ctl(timer_ctx, ctl);
> +	}
> +
>   	timer_ctx->irq.level = new_level;
>   	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
>   				   timer_ctx->irq.level);
> 
> which reports the interrupt state in all cases.
> 
> Does this work for you?

This works.
Are you going to pull this diff/patch in to your 6.2-nv tree? or you 
want me to send an updated patch?

> 
> Thanks,
> 
> 	M.
> 

Thanks,
Ganapat

Re: [PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Marc Zyngier]

On Tue, 10 Jan 2023 08:41:44 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> 
> On 02-01-2023 05:16 pm, Marc Zyngier wrote:
> > On Thu, 29 Dec 2022 13:53:15 +0000,
> > Marc Zyngier <maz@kernel.org> wrote:
> >> 
> >> On Wed, 24 Aug 2022 07:03:03 +0100,
> >> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> >>> 
> >>> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
> >>> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
> >>> loaded timer.
> >>> 
> >>> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
> >>> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
> >>> This results in the drop of interrupt from Guest-Hypervisor, where as
> >>> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
> >>> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
> >>> are stuck in Idle thread and rcu soft lockups are seen.
> >>> 
> >>> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
> >>> trap handler is emulating the ISTATUS bit.
> >>> 
> >>> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
> >>> 
> >>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> >>> ---
> >>>   arch/arm64/kvm/arch_timer.c | 5 +++++
> >>>   1 file changed, 5 insertions(+)
> >>> 
> >>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> >>> index 27a6ec46803a..0b32d943d2d5 100644
> >>> --- a/arch/arm64/kvm/arch_timer.c
> >>> +++ b/arch/arm64/kvm/arch_timer.c
> >>> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
> >>>   			      struct arch_timer_context *timer,
> >>>   			      enum kvm_arch_timer_regs treg);
> >>>   static bool kvm_arch_timer_get_input_level(int vintid);
> >>> +static u64 read_timer_ctl(struct arch_timer_context *timer);
> >>>     static struct irq_ops arch_timer_irq_ops = {
> >>>   	.get_input_level = kvm_arch_timer_get_input_level,
> >>> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
> >>>   		return HRTIMER_RESTART;
> >>>   	}
> >>>   +	/* Timer emulated, emulate ISTATUS also */
> >>> +	timer_set_ctl(ctx, read_timer_ctl(ctx));
> >> 
> >> Why should we do that for non-NV2 configurations?
> >> 
> >>>   	kvm_timer_update_irq(vcpu, true, ctx);
> >>>   	return HRTIMER_NORESTART;
> >>>   }
> >>> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >>>   	trace_kvm_timer_emulate(ctx, should_fire);
> >>>     	if (should_fire != ctx->irq.level) {
> >>> +		/* Timer emulated, emulate ISTATUS also */
> >>> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
> >>>   		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
> >>>   		return;
> >>>   	}
> >> 
> >> I'm not overly keen on this. Yes, we can set the status bit there. But
> >> conversely, the bit will not get cleared when the guest reprograms the
> >> timer, and will take a full exit/entry cycle for it to appear.
> >> 
> >> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
> >> used to emulate something as dynamic as a timer.
> >> 
> >> It is only with FEAT_ECV that we can solve this correctly by trapping
> >> the counter/timer accesses and emulate them for the guest hypervisor.
> >> I'd rather we add support for that, as I expect all the FEAT_NV2
> >> implementations to have it (and hopefully FEAT_FGT as well).
> > 
> > So I went ahead and implemented some very basic FEAT_ECV support to
> > correctly emulate the timers (trapping the CTL/CVAL accesses).
> > 
> > Performance dropped like a rock (~30% extra overhead) for L2
> > exit-heavy workloads that are terminated in userspace, such as virtio.
> > For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
> > as we save/restore the timer context, and this is enough to make
> > things visibly slower, even on a pretty fast machine.
> > 
> > I managed to get *some* performance back by satisfying CTL/CVAL reads
> > very early on the exit path (a pretty common theme with NV). Which
> > means we end-up needing something like what you have -- only a bit
> > more complete. I came up with the following:
> 
> Yes it is more appropriate, this moves ISTATUS update to single place.
> > 
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index 4945c5b96f05..a198a6211e2a 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> >   {
> >   	int ret;
> >   +	/*
> > +	 * Paper over NV2 brokenness by publishing the interrupt status
> > +	 * bit. This still results in a poor quality of emulation (guest
> > +	 * writes will have no effect until the next exit).
> > +	 *
> > +	 * But hey, it's fast, right?
> > +	 */
> > +	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
> > +	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
> > +		u32 ctl = timer_get_ctl(timer_ctx);
> > +
> > +		if (new_level)
> > +			ctl |= ARCH_TIMER_CTRL_IT_STAT;
> > +		else
> > +			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
> > +
> > +		timer_set_ctl(timer_ctx, ctl);
> > +	}
> > +
> >   	timer_ctx->irq.level = new_level;
> >   	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
> >   				   timer_ctx->irq.level);
> > 
> > which reports the interrupt state in all cases.
> > 
> > Does this work for you?
> 
> This works.
> Are you going to pull this diff/patch in to your 6.2-nv tree? or you
> want me to send an updated patch?

I already have this in the patch titled:

KVM: arm64: nv: Publish emulated timer interrupt state in the in-memory state

and the result gets used by:

KVM: arm64: nv: Accelerate EL0 timer read accesses when FEAT_ECV is on

(not pasting the SHA1s as I'm still fixing a few nits here and there,
and the commit IDs will change).

Thanks,

	M.

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

Re: [PATCH 2/3] KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.

[Marc Zyngier]

On Tue, 10 Jan 2023 08:41:44 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> 
> On 02-01-2023 05:16 pm, Marc Zyngier wrote:
> > On Thu, 29 Dec 2022 13:53:15 +0000,
> > Marc Zyngier <maz@kernel.org> wrote:
> >> 
> >> On Wed, 24 Aug 2022 07:03:03 +0100,
> >> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> >>> 
> >>> Guest-Hypervisor forwards the timer interrupt to Guest-Guest, if it is
> >>> enabled, unmasked and ISTATUS bit of register CNTV_CTL_EL0 is set for a
> >>> loaded timer.
> >>> 
> >>> For NV2 implementation, the Host-Hypervisor is not emulating the ISTATUS
> >>> bit while forwarding the Emulated Vtimer Interrupt to Guest-Hypervisor.
> >>> This results in the drop of interrupt from Guest-Hypervisor, where as
> >>> Host Hypervisor marked it as an active interrupt and expecting Guest-Guest
> >>> to consume and acknowledge. Due to this, some of the Guest-Guest vCPUs
> >>> are stuck in Idle thread and rcu soft lockups are seen.
> >>> 
> >>> This issue is not seen with NV1 case since the register CNTV_CTL_EL0 read
> >>> trap handler is emulating the ISTATUS bit.
> >>> 
> >>> Adding code to set/emulate the ISTATUS when the emulated timers are fired.
> >>> 
> >>> Signed-off-by: Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com>
> >>> ---
> >>>   arch/arm64/kvm/arch_timer.c | 5 +++++
> >>>   1 file changed, 5 insertions(+)
> >>> 
> >>> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> >>> index 27a6ec46803a..0b32d943d2d5 100644
> >>> --- a/arch/arm64/kvm/arch_timer.c
> >>> +++ b/arch/arm64/kvm/arch_timer.c
> >>> @@ -63,6 +63,7 @@ static u64 kvm_arm_timer_read(struct kvm_vcpu *vcpu,
> >>>   			      struct arch_timer_context *timer,
> >>>   			      enum kvm_arch_timer_regs treg);
> >>>   static bool kvm_arch_timer_get_input_level(int vintid);
> >>> +static u64 read_timer_ctl(struct arch_timer_context *timer);
> >>>     static struct irq_ops arch_timer_irq_ops = {
> >>>   	.get_input_level = kvm_arch_timer_get_input_level,
> >>> @@ -356,6 +357,8 @@ static enum hrtimer_restart kvm_hrtimer_expire(struct hrtimer *hrt)
> >>>   		return HRTIMER_RESTART;
> >>>   	}
> >>>   +	/* Timer emulated, emulate ISTATUS also */
> >>> +	timer_set_ctl(ctx, read_timer_ctl(ctx));
> >> 
> >> Why should we do that for non-NV2 configurations?
> >> 
> >>>   	kvm_timer_update_irq(vcpu, true, ctx);
> >>>   	return HRTIMER_NORESTART;
> >>>   }
> >>> @@ -458,6 +461,8 @@ static void timer_emulate(struct arch_timer_context *ctx)
> >>>   	trace_kvm_timer_emulate(ctx, should_fire);
> >>>     	if (should_fire != ctx->irq.level) {
> >>> +		/* Timer emulated, emulate ISTATUS also */
> >>> +		timer_set_ctl(ctx, read_timer_ctl(ctx));
> >>>   		kvm_timer_update_irq(ctx->vcpu, should_fire, ctx);
> >>>   		return;
> >>>   	}
> >> 
> >> I'm not overly keen on this. Yes, we can set the status bit there. But
> >> conversely, the bit will not get cleared when the guest reprograms the
> >> timer, and will take a full exit/entry cycle for it to appear.
> >> 
> >> Ergo, the architecture is buggy as memory (the VNCR page) cannot be
> >> used to emulate something as dynamic as a timer.
> >> 
> >> It is only with FEAT_ECV that we can solve this correctly by trapping
> >> the counter/timer accesses and emulate them for the guest hypervisor.
> >> I'd rather we add support for that, as I expect all the FEAT_NV2
> >> implementations to have it (and hopefully FEAT_FGT as well).
> > 
> > So I went ahead and implemented some very basic FEAT_ECV support to
> > correctly emulate the timers (trapping the CTL/CVAL accesses).
> > 
> > Performance dropped like a rock (~30% extra overhead) for L2
> > exit-heavy workloads that are terminated in userspace, such as virtio.
> > For those workloads, vcpu_{load,put}() in L1 now generate extra traps,
> > as we save/restore the timer context, and this is enough to make
> > things visibly slower, even on a pretty fast machine.
> > 
> > I managed to get *some* performance back by satisfying CTL/CVAL reads
> > very early on the exit path (a pretty common theme with NV). Which
> > means we end-up needing something like what you have -- only a bit
> > more complete. I came up with the following:
> 
> Yes it is more appropriate, this moves ISTATUS update to single place.
> > 
> > diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> > index 4945c5b96f05..a198a6211e2a 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -450,6 +450,25 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
> >   {
> >   	int ret;
> >   +	/*
> > +	 * Paper over NV2 brokenness by publishing the interrupt status
> > +	 * bit. This still results in a poor quality of emulation (guest
> > +	 * writes will have no effect until the next exit).
> > +	 *
> > +	 * But hey, it's fast, right?
> > +	 */
> > +	if (vcpu_has_nv2(vcpu) && is_hyp_ctxt(vcpu) &&
> > +	    (timer_ctx == vcpu_vtimer(vcpu) || timer_ctx == vcpu_ptimer(vcpu))) {
> > +		u32 ctl = timer_get_ctl(timer_ctx);
> > +
> > +		if (new_level)
> > +			ctl |= ARCH_TIMER_CTRL_IT_STAT;
> > +		else
> > +			ctl &= ~ARCH_TIMER_CTRL_IT_STAT;
> > +
> > +		timer_set_ctl(timer_ctx, ctl);
> > +	}
> > +
> >   	timer_ctx->irq.level = new_level;
> >   	trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
> >   				   timer_ctx->irq.level);
> > 
> > which reports the interrupt state in all cases.
> > 
> > Does this work for you?
> 
> This works.
> Are you going to pull this diff/patch in to your 6.2-nv tree? or you
> want me to send an updated patch?

I already have this in the patch titled:

KVM: arm64: nv: Publish emulated timer interrupt state in the in-memory state

and the result gets used by:

KVM: arm64: nv: Accelerate EL0 timer read accesses when FEAT_ECV is on

(not pasting the SHA1s as I'm still fixing a few nits here and there,
and the commit IDs will change).

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/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

Hi Marc,

On 24-08-2022 11:33 am, Ganapatrao Kulkarni wrote:
> This series contains 3 fixes which were found while testing
> ARM64 Nested Virtualization patch series.
> 
> First patch avoids the restart of hrtimer when timer interrupt is
> fired/forwarded to Guest-Hypervisor.
> 
> Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.
> 
> Third patch fixes the NestedVM boot hang seen when Guest Hypersior
> configured with 64K pagesize where as Host Hypervisor with 4K.
> 
> These patches are rebased on Nested Virtualization V6 patchset[1].

If I boot a Guest Hypervisor with more cores and then booting of a 
NestedVM with equal number of cores or booting multiple 
NestedVMs(simultaneously) with lower number of cores is resulting in 
very slow booting and some time RCU soft-lockup of a NestedVM. This I 
have debugged and turned out to be due to many SGI are getting asserted 
to all vCPUs of a Guest-Hypervisor when Guest-Hypervisor KVM code 
prepares NestedVM for WFI wakeup/return.

When Guest Hypervisor prepares NestedVM while returning/resuming from 
WFI, it is loading guest-context,  vGIC and timer contexts etc.
The function gic_poke_irq (called from irq_set_irqchip_state with 
spinlock held) writes to register GICD_ISACTIVER in Guest-Hypervisor's 
KVM code resulting in mem-abort trap to Host Hypervisor. Host Hypervisor 
as part of handling the guest mem abort, function io_mem_abort is called 
  in turn vgic_mmio_write_sactive, which prepares every vCPU of Guest 
Hypervisor by calling SGI. The number of SGI/IPI calls goes 
exponentially high when more and more cores are used to boot Guest 
Hypervisor.

Code trace:
At Guest-hypervisor: 
kvm_timer_vcpu_load->kvm_timer_vcpu_load_gic->set_timer_irq_phys_active->
irq_set_irqchip_state->gic_poke_irq

At Host-Hypervisor: io_mem_abort-> 
kvm_io_bus_write->__kvm_io_bus_write->dispatch_mmio_write->
vgic_mmio_write_sactive->vgic_access_active_prepare->
kvm_kick_many_cpus->smp_call_function_many

I am currently working around this with "nohlt" kernel param to 
NestedVM. Any suggestions to handle/fix this case/issue and avoid the 
slowness of booting of NestedVM with more cores?

Note: Guest-Hypervisor and NestedVM are using default kernel installed 
using Fedora 36 iso.

> 
> [1] https://www.spinics.net/lists/kvm/msg265656.html
> 
> D Scott Phillips (1):
>    KVM: arm64: nv: only emulate timers that have not yet fired
> 
> Ganapatrao Kulkarni (2):
>    KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.
>    KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than
>      block size.
> 
>   arch/arm64/kvm/arch_timer.c | 8 +++++++-
>   arch/arm64/kvm/mmu.c        | 2 +-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 

Thanks,
Ganapat

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

Hi Marc,

On 24-08-2022 11:33 am, Ganapatrao Kulkarni wrote:
> This series contains 3 fixes which were found while testing
> ARM64 Nested Virtualization patch series.
> 
> First patch avoids the restart of hrtimer when timer interrupt is
> fired/forwarded to Guest-Hypervisor.
> 
> Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.
> 
> Third patch fixes the NestedVM boot hang seen when Guest Hypersior
> configured with 64K pagesize where as Host Hypervisor with 4K.
> 
> These patches are rebased on Nested Virtualization V6 patchset[1].

If I boot a Guest Hypervisor with more cores and then booting of a 
NestedVM with equal number of cores or booting multiple 
NestedVMs(simultaneously) with lower number of cores is resulting in 
very slow booting and some time RCU soft-lockup of a NestedVM. This I 
have debugged and turned out to be due to many SGI are getting asserted 
to all vCPUs of a Guest-Hypervisor when Guest-Hypervisor KVM code 
prepares NestedVM for WFI wakeup/return.

When Guest Hypervisor prepares NestedVM while returning/resuming from 
WFI, it is loading guest-context,  vGIC and timer contexts etc.
The function gic_poke_irq (called from irq_set_irqchip_state with 
spinlock held) writes to register GICD_ISACTIVER in Guest-Hypervisor's 
KVM code resulting in mem-abort trap to Host Hypervisor. Host Hypervisor 
as part of handling the guest mem abort, function io_mem_abort is called 
  in turn vgic_mmio_write_sactive, which prepares every vCPU of Guest 
Hypervisor by calling SGI. The number of SGI/IPI calls goes 
exponentially high when more and more cores are used to boot Guest 
Hypervisor.

Code trace:
At Guest-hypervisor: 
kvm_timer_vcpu_load->kvm_timer_vcpu_load_gic->set_timer_irq_phys_active->
irq_set_irqchip_state->gic_poke_irq

At Host-Hypervisor: io_mem_abort-> 
kvm_io_bus_write->__kvm_io_bus_write->dispatch_mmio_write->
vgic_mmio_write_sactive->vgic_access_active_prepare->
kvm_kick_many_cpus->smp_call_function_many

I am currently working around this with "nohlt" kernel param to 
NestedVM. Any suggestions to handle/fix this case/issue and avoid the 
slowness of booting of NestedVM with more cores?

Note: Guest-Hypervisor and NestedVM are using default kernel installed 
using Fedora 36 iso.

> 
> [1] https://www.spinics.net/lists/kvm/msg265656.html
> 
> D Scott Phillips (1):
>    KVM: arm64: nv: only emulate timers that have not yet fired
> 
> Ganapatrao Kulkarni (2):
>    KVM: arm64: nv: Emulate ISTATUS when emulated timers are fired.
>    KVM: arm64: nv: Avoid block mapping if max_map_size is smaller than
>      block size.
> 
>   arch/arm64/kvm/arch_timer.c | 8 +++++++-
>   arch/arm64/kvm/mmu.c        | 2 +-
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 

Thanks,
Ganapat

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

Hi Ganapatrao,

On Tue, 10 Jan 2023 12:17:20 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> 
> Hi Marc,
> 
> On 24-08-2022 11:33 am, Ganapatrao Kulkarni wrote:
> > This series contains 3 fixes which were found while testing
> > ARM64 Nested Virtualization patch series.
> > 
> > First patch avoids the restart of hrtimer when timer interrupt is
> > fired/forwarded to Guest-Hypervisor.
> > 
> > Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.
> > 
> > Third patch fixes the NestedVM boot hang seen when Guest Hypersior
> > configured with 64K pagesize where as Host Hypervisor with 4K.
> > 
> > These patches are rebased on Nested Virtualization V6 patchset[1].
> 
> If I boot a Guest Hypervisor with more cores and then booting of a
> NestedVM with equal number of cores or booting multiple
> NestedVMs(simultaneously) with lower number of cores is resulting in
> very slow booting and some time RCU soft-lockup of a NestedVM. This I
> have debugged and turned out to be due to many SGI are getting
> asserted to all vCPUs of a Guest-Hypervisor when Guest-Hypervisor KVM
> code prepares NestedVM for WFI wakeup/return.
> 
> When Guest Hypervisor prepares NestedVM while returning/resuming from
> WFI, it is loading guest-context,  vGIC and timer contexts etc.
> The function gic_poke_irq (called from irq_set_irqchip_state with
> spinlock held) writes to register GICD_ISACTIVER in Guest-Hypervisor's
> KVM code resulting in mem-abort trap to Host Hypervisor. Host
> Hypervisor as part of handling the guest mem abort, function
> io_mem_abort is called  in turn vgic_mmio_write_sactive, which
> prepares every vCPU of Guest Hypervisor by calling SGI. The number of
> SGI/IPI calls goes exponentially high when more and more cores are
> used to boot Guest Hypervisor.

This really isn't surprising. NV combined with oversubscribing is
bound to be absolutely terrible. The write to GICD_ISACTIVER is
only symptomatic of the interrupt amplification problem that already
exists without NV (any IPI in a guest is likely to result in at least
one IPI in the host).

Short of having direct injection of interrupts for *all* interrupts,
you end-up with this sort of emulation that relies on being able to
synchronise all CPUs. Is it bad? Yes. Very.

> 
> Code trace:
> At Guest-hypervisor:
> kvm_timer_vcpu_load->kvm_timer_vcpu_load_gic->set_timer_irq_phys_active->
> irq_set_irqchip_state->gic_poke_irq
> 
> At Host-Hypervisor: io_mem_abort->
> kvm_io_bus_write->__kvm_io_bus_write->dispatch_mmio_write->
> vgic_mmio_write_sactive->vgic_access_active_prepare->
> kvm_kick_many_cpus->smp_call_function_many
> 
> I am currently working around this with "nohlt" kernel param to
> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
> slowness of booting of NestedVM with more cores?

At the moment, I'm focussing on correctness rather than performance.

Maybe we can restrict the conditions in which we perform this
synchronisation, but that's pretty low on my radar at the moment. Once
things are in a state that can be merged and that works correctly, we
can look into it.

Thanks,

	M.

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

Hi Ganapatrao,

On Tue, 10 Jan 2023 12:17:20 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> 
> Hi Marc,
> 
> On 24-08-2022 11:33 am, Ganapatrao Kulkarni wrote:
> > This series contains 3 fixes which were found while testing
> > ARM64 Nested Virtualization patch series.
> > 
> > First patch avoids the restart of hrtimer when timer interrupt is
> > fired/forwarded to Guest-Hypervisor.
> > 
> > Second patch fixes the vtimer interrupt drop from the Guest-Hypervisor.
> > 
> > Third patch fixes the NestedVM boot hang seen when Guest Hypersior
> > configured with 64K pagesize where as Host Hypervisor with 4K.
> > 
> > These patches are rebased on Nested Virtualization V6 patchset[1].
> 
> If I boot a Guest Hypervisor with more cores and then booting of a
> NestedVM with equal number of cores or booting multiple
> NestedVMs(simultaneously) with lower number of cores is resulting in
> very slow booting and some time RCU soft-lockup of a NestedVM. This I
> have debugged and turned out to be due to many SGI are getting
> asserted to all vCPUs of a Guest-Hypervisor when Guest-Hypervisor KVM
> code prepares NestedVM for WFI wakeup/return.
> 
> When Guest Hypervisor prepares NestedVM while returning/resuming from
> WFI, it is loading guest-context,  vGIC and timer contexts etc.
> The function gic_poke_irq (called from irq_set_irqchip_state with
> spinlock held) writes to register GICD_ISACTIVER in Guest-Hypervisor's
> KVM code resulting in mem-abort trap to Host Hypervisor. Host
> Hypervisor as part of handling the guest mem abort, function
> io_mem_abort is called  in turn vgic_mmio_write_sactive, which
> prepares every vCPU of Guest Hypervisor by calling SGI. The number of
> SGI/IPI calls goes exponentially high when more and more cores are
> used to boot Guest Hypervisor.

This really isn't surprising. NV combined with oversubscribing is
bound to be absolutely terrible. The write to GICD_ISACTIVER is
only symptomatic of the interrupt amplification problem that already
exists without NV (any IPI in a guest is likely to result in at least
one IPI in the host).

Short of having direct injection of interrupts for *all* interrupts,
you end-up with this sort of emulation that relies on being able to
synchronise all CPUs. Is it bad? Yes. Very.

> 
> Code trace:
> At Guest-hypervisor:
> kvm_timer_vcpu_load->kvm_timer_vcpu_load_gic->set_timer_irq_phys_active->
> irq_set_irqchip_state->gic_poke_irq
> 
> At Host-Hypervisor: io_mem_abort->
> kvm_io_bus_write->__kvm_io_bus_write->dispatch_mmio_write->
> vgic_mmio_write_sactive->vgic_access_active_prepare->
> kvm_kick_many_cpus->smp_call_function_many
> 
> I am currently working around this with "nohlt" kernel param to
> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
> slowness of booting of NestedVM with more cores?

At the moment, I'm focussing on correctness rather than performance.

Maybe we can restrict the conditions in which we perform this
synchronisation, but that's pretty low on my radar at the moment. Once
things are in a state that can be merged and that works correctly, we
can look into it.

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/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On Tue, 10 Jan 2023 12:17:20 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> I am currently working around this with "nohlt" kernel param to
> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
> slowness of booting of NestedVM with more cores?
> 
> Note: Guest-Hypervisor and NestedVM are using default kernel installed
> using Fedora 36 iso.

Despite what I said earlier, I have a vague idea here, thanks to the
interesting call traces that you provided (this is really awesome work
BTW, given how hard it is to trace things across 3 different kernels).

We can slightly limit the impact of the prepare/finish sequence if the
guest hypervisor only accesses the active registers for SGIs/PPIs on
the vcpu that owns them, forbidding any cross-CPU-to-redistributor
access.

Something along these lines, which is only boot-tested. Let me know
how this fares for you.

Thanks,

	M.

diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
index b32d434c1d4a..1cca45be5335 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio.c
@@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
  * active state can be overwritten when the VCPU's state is synced coming back
  * from the guest.
  *
- * For shared interrupts as well as GICv3 private interrupts, we have to
- * stop all the VCPUs because interrupts can be migrated while we don't hold
- * the IRQ locks and we don't want to be chasing moving targets.
+ * For shared interrupts as well as GICv3 private interrupts accessed from the
+ * non-owning CPU, we have to stop all the VCPUs because interrupts can be
+ * migrated while we don't hold the IRQ locks and we don't want to be chasing
+ * moving targets.
  *
  * For GICv2 private interrupts we don't have to do anything because
  * userspace accesses to the VGIC state already require all VCPUs to be
@@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
  */
 static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
 {
-	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
+	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
+	     vcpu == kvm_get_running_vcpu()) ||
 	    intid >= VGIC_NR_PRIVATE_IRQS)
 		kvm_arm_halt_guest(vcpu->kvm);
 }
@@ -492,7 +494,8 @@ static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
 /* See vgic_access_active_prepare */
 static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
 {
-	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
+	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
+	     vcpu == kvm_get_running_vcpu()) ||
 	    intid >= VGIC_NR_PRIVATE_IRQS)
 		kvm_arm_resume_guest(vcpu->kvm);
 }

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On Tue, 10 Jan 2023 12:17:20 +0000,
Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> 
> I am currently working around this with "nohlt" kernel param to
> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
> slowness of booting of NestedVM with more cores?
> 
> Note: Guest-Hypervisor and NestedVM are using default kernel installed
> using Fedora 36 iso.

Despite what I said earlier, I have a vague idea here, thanks to the
interesting call traces that you provided (this is really awesome work
BTW, given how hard it is to trace things across 3 different kernels).

We can slightly limit the impact of the prepare/finish sequence if the
guest hypervisor only accesses the active registers for SGIs/PPIs on
the vcpu that owns them, forbidding any cross-CPU-to-redistributor
access.

Something along these lines, which is only boot-tested. Let me know
how this fares for you.

Thanks,

	M.

diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
index b32d434c1d4a..1cca45be5335 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio.c
@@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
  * active state can be overwritten when the VCPU's state is synced coming back
  * from the guest.
  *
- * For shared interrupts as well as GICv3 private interrupts, we have to
- * stop all the VCPUs because interrupts can be migrated while we don't hold
- * the IRQ locks and we don't want to be chasing moving targets.
+ * For shared interrupts as well as GICv3 private interrupts accessed from the
+ * non-owning CPU, we have to stop all the VCPUs because interrupts can be
+ * migrated while we don't hold the IRQ locks and we don't want to be chasing
+ * moving targets.
  *
  * For GICv2 private interrupts we don't have to do anything because
  * userspace accesses to the VGIC state already require all VCPUs to be
@@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
  */
 static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
 {
-	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
+	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
+	     vcpu == kvm_get_running_vcpu()) ||
 	    intid >= VGIC_NR_PRIVATE_IRQS)
 		kvm_arm_halt_guest(vcpu->kvm);
 }
@@ -492,7 +494,8 @@ static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
 /* See vgic_access_active_prepare */
 static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
 {
-	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
+	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
+	     vcpu == kvm_get_running_vcpu()) ||
 	    intid >= VGIC_NR_PRIVATE_IRQS)
 		kvm_arm_resume_guest(vcpu->kvm);
 }

-- 
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/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On Tue, 10 Jan 2023 21:54:39 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Tue, 10 Jan 2023 12:17:20 +0000,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> > 
> > I am currently working around this with "nohlt" kernel param to
> > NestedVM. Any suggestions to handle/fix this case/issue and avoid the
> > slowness of booting of NestedVM with more cores?
> > 
> > Note: Guest-Hypervisor and NestedVM are using default kernel installed
> > using Fedora 36 iso.
> 
> Despite what I said earlier, I have a vague idea here, thanks to the
> interesting call traces that you provided (this is really awesome work
> BTW, given how hard it is to trace things across 3 different kernels).
> 
> We can slightly limit the impact of the prepare/finish sequence if the
> guest hypervisor only accesses the active registers for SGIs/PPIs on
> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
> access.
> 
> Something along these lines, which is only boot-tested. Let me know
> how this fares for you.
> 
> Thanks,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index b32d434c1d4a..1cca45be5335 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>   * active state can be overwritten when the VCPU's state is synced coming back
>   * from the guest.
>   *
> - * For shared interrupts as well as GICv3 private interrupts, we have to
> - * stop all the VCPUs because interrupts can be migrated while we don't hold
> - * the IRQ locks and we don't want to be chasing moving targets.
> + * For shared interrupts as well as GICv3 private interrupts accessed from the
> + * non-owning CPU, we have to stop all the VCPUs because interrupts can be
> + * migrated while we don't hold the IRQ locks and we don't want to be chasing
> + * moving targets.
>   *
>   * For GICv2 private interrupts we don't have to do anything because
>   * userspace accesses to the VGIC state already require all VCPUs to be
> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>   */
>  static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>  {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

This should obviously be

+	     vcpu != kvm_get_running_vcpu()) ||

>  	    intid >= VGIC_NR_PRIVATE_IRQS)
>  		kvm_arm_halt_guest(vcpu->kvm);
>  }
> @@ -492,7 +494,8 @@ static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>  /* See vgic_access_active_prepare */
>  static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
>  {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

Same here.

>  	    intid >= VGIC_NR_PRIVATE_IRQS)
>  		kvm_arm_resume_guest(vcpu->kvm);
>  }

Thanks,

	M.

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On Tue, 10 Jan 2023 21:54:39 +0000,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Tue, 10 Jan 2023 12:17:20 +0000,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
> > 
> > I am currently working around this with "nohlt" kernel param to
> > NestedVM. Any suggestions to handle/fix this case/issue and avoid the
> > slowness of booting of NestedVM with more cores?
> > 
> > Note: Guest-Hypervisor and NestedVM are using default kernel installed
> > using Fedora 36 iso.
> 
> Despite what I said earlier, I have a vague idea here, thanks to the
> interesting call traces that you provided (this is really awesome work
> BTW, given how hard it is to trace things across 3 different kernels).
> 
> We can slightly limit the impact of the prepare/finish sequence if the
> guest hypervisor only accesses the active registers for SGIs/PPIs on
> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
> access.
> 
> Something along these lines, which is only boot-tested. Let me know
> how this fares for you.
> 
> Thanks,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index b32d434c1d4a..1cca45be5335 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>   * active state can be overwritten when the VCPU's state is synced coming back
>   * from the guest.
>   *
> - * For shared interrupts as well as GICv3 private interrupts, we have to
> - * stop all the VCPUs because interrupts can be migrated while we don't hold
> - * the IRQ locks and we don't want to be chasing moving targets.
> + * For shared interrupts as well as GICv3 private interrupts accessed from the
> + * non-owning CPU, we have to stop all the VCPUs because interrupts can be
> + * migrated while we don't hold the IRQ locks and we don't want to be chasing
> + * moving targets.
>   *
>   * For GICv2 private interrupts we don't have to do anything because
>   * userspace accesses to the VGIC state already require all VCPUs to be
> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>   */
>  static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>  {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

This should obviously be

+	     vcpu != kvm_get_running_vcpu()) ||

>  	    intid >= VGIC_NR_PRIVATE_IRQS)
>  		kvm_arm_halt_guest(vcpu->kvm);
>  }
> @@ -492,7 +494,8 @@ static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>  /* See vgic_access_active_prepare */
>  static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
>  {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

Same here.

>  	    intid >= VGIC_NR_PRIVATE_IRQS)
>  		kvm_arm_resume_guest(vcpu->kvm);
>  }

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/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

On 11-01-2023 03:24 am, Marc Zyngier wrote:
> On Tue, 10 Jan 2023 12:17:20 +0000,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> I am currently working around this with "nohlt" kernel param to
>> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
>> slowness of booting of NestedVM with more cores?
>>
>> Note: Guest-Hypervisor and NestedVM are using default kernel installed
>> using Fedora 36 iso.
> 
> Despite what I said earlier, I have a vague idea here, thanks to the
> interesting call traces that you provided (this is really awesome work
> BTW, given how hard it is to trace things across 3 different kernels).
> 
> We can slightly limit the impact of the prepare/finish sequence if the
> guest hypervisor only accesses the active registers for SGIs/PPIs on
> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
> access.
> 
> Something along these lines, which is only boot-tested. Let me know
> how this fares for you.
> 
> Thanks,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index b32d434c1d4a..1cca45be5335 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>    * active state can be overwritten when the VCPU's state is synced coming back
>    * from the guest.
>    *
> - * For shared interrupts as well as GICv3 private interrupts, we have to
> - * stop all the VCPUs because interrupts can be migrated while we don't hold
> - * the IRQ locks and we don't want to be chasing moving targets.
> + * For shared interrupts as well as GICv3 private interrupts accessed from the
> + * non-owning CPU, we have to stop all the VCPUs because interrupts can be
> + * migrated while we don't hold the IRQ locks and we don't want to be chasing
> + * moving targets.
>    *
>    * For GICv2 private interrupts we don't have to do anything because
>    * userspace accesses to the VGIC state already require all VCPUs to be
> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>    */
>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>   {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

Thanks Marc for the patch!

I think, you mean not equal to?
+           vcpu != kvm_get_running_vcpu()) ||

With the change to not-equal, the issue is fixed and I could see the 
NestedVM booting is pretty fast with higher number of cores as well.

>   	    intid >= VGIC_NR_PRIVATE_IRQS)
>   		kvm_arm_halt_guest(vcpu->kvm);
>   }
> @@ -492,7 +494,8 @@ static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>   /* See vgic_access_active_prepare */
>   static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
>   {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

Same, not equal to.
>   	    intid >= VGIC_NR_PRIVATE_IRQS)
>   		kvm_arm_resume_guest(vcpu->kvm);
>   }
> 


Thanks,
Ganapat

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

On 11-01-2023 03:24 am, Marc Zyngier wrote:
> On Tue, 10 Jan 2023 12:17:20 +0000,
> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>
>> I am currently working around this with "nohlt" kernel param to
>> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
>> slowness of booting of NestedVM with more cores?
>>
>> Note: Guest-Hypervisor and NestedVM are using default kernel installed
>> using Fedora 36 iso.
> 
> Despite what I said earlier, I have a vague idea here, thanks to the
> interesting call traces that you provided (this is really awesome work
> BTW, given how hard it is to trace things across 3 different kernels).
> 
> We can slightly limit the impact of the prepare/finish sequence if the
> guest hypervisor only accesses the active registers for SGIs/PPIs on
> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
> access.
> 
> Something along these lines, which is only boot-tested. Let me know
> how this fares for you.
> 
> Thanks,
> 
> 	M.
> 
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index b32d434c1d4a..1cca45be5335 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>    * active state can be overwritten when the VCPU's state is synced coming back
>    * from the guest.
>    *
> - * For shared interrupts as well as GICv3 private interrupts, we have to
> - * stop all the VCPUs because interrupts can be migrated while we don't hold
> - * the IRQ locks and we don't want to be chasing moving targets.
> + * For shared interrupts as well as GICv3 private interrupts accessed from the
> + * non-owning CPU, we have to stop all the VCPUs because interrupts can be
> + * migrated while we don't hold the IRQ locks and we don't want to be chasing
> + * moving targets.
>    *
>    * For GICv2 private interrupts we don't have to do anything because
>    * userspace accesses to the VGIC state already require all VCPUs to be
> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu *vcpu,
>    */
>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>   {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

Thanks Marc for the patch!

I think, you mean not equal to?
+           vcpu != kvm_get_running_vcpu()) ||

With the change to not-equal, the issue is fixed and I could see the 
NestedVM booting is pretty fast with higher number of cores as well.

>   	    intid >= VGIC_NR_PRIVATE_IRQS)
>   		kvm_arm_halt_guest(vcpu->kvm);
>   }
> @@ -492,7 +494,8 @@ static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 intid)
>   /* See vgic_access_active_prepare */
>   static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
>   {
> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
> +	     vcpu == kvm_get_running_vcpu()) ||

Same, not equal to.
>   	    intid >= VGIC_NR_PRIVATE_IRQS)
>   		kvm_arm_resume_guest(vcpu->kvm);
>   }
> 


Thanks,
Ganapat

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

On 11-01-2023 02:16 pm, Ganapatrao Kulkarni wrote:
> 
> 
> On 11-01-2023 03:24 am, Marc Zyngier wrote:
>> On Tue, 10 Jan 2023 12:17:20 +0000,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> I am currently working around this with "nohlt" kernel param to
>>> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
>>> slowness of booting of NestedVM with more cores?
>>>
>>> Note: Guest-Hypervisor and NestedVM are using default kernel installed
>>> using Fedora 36 iso.
>>
>> Despite what I said earlier, I have a vague idea here, thanks to the
>> interesting call traces that you provided (this is really awesome work
>> BTW, given how hard it is to trace things across 3 different kernels).
>>
>> We can slightly limit the impact of the prepare/finish sequence if the
>> guest hypervisor only accesses the active registers for SGIs/PPIs on
>> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
>> access.
>>
>> Something along these lines, which is only boot-tested. Let me know
>> how this fares for you.
>>
>> Thanks,
>>
>>     M.
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c 
>> b/arch/arm64/kvm/vgic/vgic-mmio.c
>> index b32d434c1d4a..1cca45be5335 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
>> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>> *vcpu,
>>    * active state can be overwritten when the VCPU's state is synced 
>> coming back
>>    * from the guest.
>>    *
>> - * For shared interrupts as well as GICv3 private interrupts, we have to
>> - * stop all the VCPUs because interrupts can be migrated while we 
>> don't hold
>> - * the IRQ locks and we don't want to be chasing moving targets.
>> + * For shared interrupts as well as GICv3 private interrupts accessed 
>> from the
>> + * non-owning CPU, we have to stop all the VCPUs because interrupts 
>> can be
>> + * migrated while we don't hold the IRQ locks and we don't want to be 
>> chasing
>> + * moving targets.
>>    *
>>    * For GICv2 private interrupts we don't have to do anything because
>>    * userspace accesses to the VGIC state already require all VCPUs to be
>> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>> *vcpu,
>>    */
>>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 
>> intid)
>>   {
>> -    if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
>> +    if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
>> +         vcpu == kvm_get_running_vcpu()) ||
> 
> Thanks Marc for the patch!
> 
> I think, you mean not equal to?
Sorry, I did not see your follow up email.

> +           vcpu != kvm_get_running_vcpu()) ||
> 
> With the change to not-equal, the issue is fixed and I could see the 
> NestedVM booting is pretty fast with higher number of cores as well.
> 
>>           intid >= VGIC_NR_PRIVATE_IRQS)
>>           kvm_arm_halt_guest(vcpu->kvm);
>>   }
>> @@ -492,7 +494,8 @@ static void vgic_access_active_prepare(struct 
>> kvm_vcpu *vcpu, u32 intid)
>>   /* See vgic_access_active_prepare */
>>   static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
>>   {
>> -    if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
>> +    if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
>> +         vcpu == kvm_get_running_vcpu()) ||
> 
> Same, not equal to.
>>           intid >= VGIC_NR_PRIVATE_IRQS)
>>           kvm_arm_resume_guest(vcpu->kvm);
>>   }
>>
> 
> 
> Thanks,
> Ganapat

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

On 11-01-2023 02:16 pm, Ganapatrao Kulkarni wrote:
> 
> 
> On 11-01-2023 03:24 am, Marc Zyngier wrote:
>> On Tue, 10 Jan 2023 12:17:20 +0000,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>
>>> I am currently working around this with "nohlt" kernel param to
>>> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
>>> slowness of booting of NestedVM with more cores?
>>>
>>> Note: Guest-Hypervisor and NestedVM are using default kernel installed
>>> using Fedora 36 iso.
>>
>> Despite what I said earlier, I have a vague idea here, thanks to the
>> interesting call traces that you provided (this is really awesome work
>> BTW, given how hard it is to trace things across 3 different kernels).
>>
>> We can slightly limit the impact of the prepare/finish sequence if the
>> guest hypervisor only accesses the active registers for SGIs/PPIs on
>> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
>> access.
>>
>> Something along these lines, which is only boot-tested. Let me know
>> how this fares for you.
>>
>> Thanks,
>>
>>     M.
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c 
>> b/arch/arm64/kvm/vgic/vgic-mmio.c
>> index b32d434c1d4a..1cca45be5335 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
>> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>> *vcpu,
>>    * active state can be overwritten when the VCPU's state is synced 
>> coming back
>>    * from the guest.
>>    *
>> - * For shared interrupts as well as GICv3 private interrupts, we have to
>> - * stop all the VCPUs because interrupts can be migrated while we 
>> don't hold
>> - * the IRQ locks and we don't want to be chasing moving targets.
>> + * For shared interrupts as well as GICv3 private interrupts accessed 
>> from the
>> + * non-owning CPU, we have to stop all the VCPUs because interrupts 
>> can be
>> + * migrated while we don't hold the IRQ locks and we don't want to be 
>> chasing
>> + * moving targets.
>>    *
>>    * For GICv2 private interrupts we don't have to do anything because
>>    * userspace accesses to the VGIC state already require all VCPUs to be
>> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>> *vcpu,
>>    */
>>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 
>> intid)
>>   {
>> -    if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
>> +    if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
>> +         vcpu == kvm_get_running_vcpu()) ||
> 
> Thanks Marc for the patch!
> 
> I think, you mean not equal to?
Sorry, I did not see your follow up email.

> +           vcpu != kvm_get_running_vcpu()) ||
> 
> With the change to not-equal, the issue is fixed and I could see the 
> NestedVM booting is pretty fast with higher number of cores as well.
> 
>>           intid >= VGIC_NR_PRIVATE_IRQS)
>>           kvm_arm_halt_guest(vcpu->kvm);
>>   }
>> @@ -492,7 +494,8 @@ static void vgic_access_active_prepare(struct 
>> kvm_vcpu *vcpu, u32 intid)
>>   /* See vgic_access_active_prepare */
>>   static void vgic_access_active_finish(struct kvm_vcpu *vcpu, u32 intid)
>>   {
>> -    if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
>> +    if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
>> +         vcpu == kvm_get_running_vcpu()) ||
> 
> Same, not equal to.
>>           intid >= VGIC_NR_PRIVATE_IRQS)
>>           kvm_arm_resume_guest(vcpu->kvm);
>>   }
>>
> 
> 
> Thanks,
> Ganapat

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On 2023-01-11 08:46, Ganapatrao Kulkarni wrote:
> On 11-01-2023 03:24 am, Marc Zyngier wrote:
>> On Tue, 10 Jan 2023 12:17:20 +0000,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>> 
>>> I am currently working around this with "nohlt" kernel param to
>>> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
>>> slowness of booting of NestedVM with more cores?
>>> 
>>> Note: Guest-Hypervisor and NestedVM are using default kernel 
>>> installed
>>> using Fedora 36 iso.
>> 
>> Despite what I said earlier, I have a vague idea here, thanks to the
>> interesting call traces that you provided (this is really awesome work
>> BTW, given how hard it is to trace things across 3 different kernels).
>> 
>> We can slightly limit the impact of the prepare/finish sequence if the
>> guest hypervisor only accesses the active registers for SGIs/PPIs on
>> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
>> access.
>> 
>> Something along these lines, which is only boot-tested. Let me know
>> how this fares for you.
>> 
>> Thanks,
>> 
>> 	M.
>> 
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c 
>> b/arch/arm64/kvm/vgic/vgic-mmio.c
>> index b32d434c1d4a..1cca45be5335 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
>> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>> *vcpu,
>>    * active state can be overwritten when the VCPU's state is synced 
>> coming back
>>    * from the guest.
>>    *
>> - * For shared interrupts as well as GICv3 private interrupts, we have 
>> to
>> - * stop all the VCPUs because interrupts can be migrated while we 
>> don't hold
>> - * the IRQ locks and we don't want to be chasing moving targets.
>> + * For shared interrupts as well as GICv3 private interrupts accessed 
>> from the
>> + * non-owning CPU, we have to stop all the VCPUs because interrupts 
>> can be
>> + * migrated while we don't hold the IRQ locks and we don't want to be 
>> chasing
>> + * moving targets.
>>    *
>>    * For GICv2 private interrupts we don't have to do anything because
>>    * userspace accesses to the VGIC state already require all VCPUs to 
>> be
>> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>> *vcpu,
>>    */
>>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 
>> intid)
>>   {
>> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
>> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
>> +	     vcpu == kvm_get_running_vcpu()) ||
> 
> Thanks Marc for the patch!
> 
> I think, you mean not equal to?
> +           vcpu != kvm_get_running_vcpu()) ||

Yeah, exactly. I woke up this morning realising this patch was
*almost* right. Don't write patches like this after a long day
at work...

> With the change to not-equal, the issue is fixed and I could see the
> NestedVM booting is pretty fast with higher number of cores as well.

Good, thanks for testing it. I'll roll up an actual patch for that
and stick it in the monster queue.

Cheers,

        M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On 2023-01-11 08:46, Ganapatrao Kulkarni wrote:
> On 11-01-2023 03:24 am, Marc Zyngier wrote:
>> On Tue, 10 Jan 2023 12:17:20 +0000,
>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>> 
>>> I am currently working around this with "nohlt" kernel param to
>>> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
>>> slowness of booting of NestedVM with more cores?
>>> 
>>> Note: Guest-Hypervisor and NestedVM are using default kernel 
>>> installed
>>> using Fedora 36 iso.
>> 
>> Despite what I said earlier, I have a vague idea here, thanks to the
>> interesting call traces that you provided (this is really awesome work
>> BTW, given how hard it is to trace things across 3 different kernels).
>> 
>> We can slightly limit the impact of the prepare/finish sequence if the
>> guest hypervisor only accesses the active registers for SGIs/PPIs on
>> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
>> access.
>> 
>> Something along these lines, which is only boot-tested. Let me know
>> how this fares for you.
>> 
>> Thanks,
>> 
>> 	M.
>> 
>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c 
>> b/arch/arm64/kvm/vgic/vgic-mmio.c
>> index b32d434c1d4a..1cca45be5335 100644
>> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
>> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
>> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>> *vcpu,
>>    * active state can be overwritten when the VCPU's state is synced 
>> coming back
>>    * from the guest.
>>    *
>> - * For shared interrupts as well as GICv3 private interrupts, we have 
>> to
>> - * stop all the VCPUs because interrupts can be migrated while we 
>> don't hold
>> - * the IRQ locks and we don't want to be chasing moving targets.
>> + * For shared interrupts as well as GICv3 private interrupts accessed 
>> from the
>> + * non-owning CPU, we have to stop all the VCPUs because interrupts 
>> can be
>> + * migrated while we don't hold the IRQ locks and we don't want to be 
>> chasing
>> + * moving targets.
>>    *
>>    * For GICv2 private interrupts we don't have to do anything because
>>    * userspace accesses to the VGIC state already require all VCPUs to 
>> be
>> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>> *vcpu,
>>    */
>>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 
>> intid)
>>   {
>> -	if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
>> +	if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
>> +	     vcpu == kvm_get_running_vcpu()) ||
> 
> Thanks Marc for the patch!
> 
> I think, you mean not equal to?
> +           vcpu != kvm_get_running_vcpu()) ||

Yeah, exactly. I woke up this morning realising this patch was
*almost* right. Don't write patches like this after a long day
at work...

> With the change to not-equal, the issue is fixed and I could see the
> NestedVM booting is pretty fast with higher number of cores as well.

Good, thanks for testing it. I'll roll up an actual patch for that
and stick it in the monster queue.

Cheers,

        M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

On 11-01-2023 05:09 pm, Marc Zyngier wrote:
> On 2023-01-11 08:46, Ganapatrao Kulkarni wrote:
>> On 11-01-2023 03:24 am, Marc Zyngier wrote:
>>> On Tue, 10 Jan 2023 12:17:20 +0000,
>>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>>
>>>> I am currently working around this with "nohlt" kernel param to
>>>> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
>>>> slowness of booting of NestedVM with more cores?
>>>>
>>>> Note: Guest-Hypervisor and NestedVM are using default kernel installed
>>>> using Fedora 36 iso.
>>>
>>> Despite what I said earlier, I have a vague idea here, thanks to the
>>> interesting call traces that you provided (this is really awesome work
>>> BTW, given how hard it is to trace things across 3 different kernels).
>>>
>>> We can slightly limit the impact of the prepare/finish sequence if the
>>> guest hypervisor only accesses the active registers for SGIs/PPIs on
>>> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
>>> access.
>>>
>>> Something along these lines, which is only boot-tested. Let me know
>>> how this fares for you.
>>>
>>> Thanks,
>>>
>>>     M.
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c 
>>> b/arch/arm64/kvm/vgic/vgic-mmio.c
>>> index b32d434c1d4a..1cca45be5335 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
>>> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>>> *vcpu,
>>>    * active state can be overwritten when the VCPU's state is synced 
>>> coming back
>>>    * from the guest.
>>>    *
>>> - * For shared interrupts as well as GICv3 private interrupts, we 
>>> have to
>>> - * stop all the VCPUs because interrupts can be migrated while we 
>>> don't hold
>>> - * the IRQ locks and we don't want to be chasing moving targets.
>>> + * For shared interrupts as well as GICv3 private interrupts 
>>> accessed from the
>>> + * non-owning CPU, we have to stop all the VCPUs because interrupts 
>>> can be
>>> + * migrated while we don't hold the IRQ locks and we don't want to 
>>> be chasing
>>> + * moving targets.
>>>    *
>>>    * For GICv2 private interrupts we don't have to do anything because
>>>    * userspace accesses to the VGIC state already require all VCPUs 
>>> to be
>>> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>>> *vcpu,
>>>    */
>>>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 
>>> intid)
>>>   {
>>> -    if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
>>> +    if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
>>> +         vcpu == kvm_get_running_vcpu()) ||
>>
>> Thanks Marc for the patch!
>>
>> I think, you mean not equal to?
>> +           vcpu != kvm_get_running_vcpu()) ||
> 
> Yeah, exactly. I woke up this morning realising this patch was
> *almost* right. Don't write patches like this after a long day
> at work...
> 
>> With the change to not-equal, the issue is fixed and I could see the
>> NestedVM booting is pretty fast with higher number of cores as well.
> 
> Good, thanks for testing it. I'll roll up an actual patch for that
> and stick it in the monster queue.

Thanks, Please pull patch 3/3 also to nv-6.2 tree along with this patch. 
I will move my setup to nv-6.2 once these patches are in.

> 
> Cheers,
> 
>         M.


Thanks,
Ganapat

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Ganapatrao Kulkarni]

On 11-01-2023 05:09 pm, Marc Zyngier wrote:
> On 2023-01-11 08:46, Ganapatrao Kulkarni wrote:
>> On 11-01-2023 03:24 am, Marc Zyngier wrote:
>>> On Tue, 10 Jan 2023 12:17:20 +0000,
>>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>>
>>>> I am currently working around this with "nohlt" kernel param to
>>>> NestedVM. Any suggestions to handle/fix this case/issue and avoid the
>>>> slowness of booting of NestedVM with more cores?
>>>>
>>>> Note: Guest-Hypervisor and NestedVM are using default kernel installed
>>>> using Fedora 36 iso.
>>>
>>> Despite what I said earlier, I have a vague idea here, thanks to the
>>> interesting call traces that you provided (this is really awesome work
>>> BTW, given how hard it is to trace things across 3 different kernels).
>>>
>>> We can slightly limit the impact of the prepare/finish sequence if the
>>> guest hypervisor only accesses the active registers for SGIs/PPIs on
>>> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
>>> access.
>>>
>>> Something along these lines, which is only boot-tested. Let me know
>>> how this fares for you.
>>>
>>> Thanks,
>>>
>>>     M.
>>>
>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c 
>>> b/arch/arm64/kvm/vgic/vgic-mmio.c
>>> index b32d434c1d4a..1cca45be5335 100644
>>> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
>>> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>>> *vcpu,
>>>    * active state can be overwritten when the VCPU's state is synced 
>>> coming back
>>>    * from the guest.
>>>    *
>>> - * For shared interrupts as well as GICv3 private interrupts, we 
>>> have to
>>> - * stop all the VCPUs because interrupts can be migrated while we 
>>> don't hold
>>> - * the IRQ locks and we don't want to be chasing moving targets.
>>> + * For shared interrupts as well as GICv3 private interrupts 
>>> accessed from the
>>> + * non-owning CPU, we have to stop all the VCPUs because interrupts 
>>> can be
>>> + * migrated while we don't hold the IRQ locks and we don't want to 
>>> be chasing
>>> + * moving targets.
>>>    *
>>>    * For GICv2 private interrupts we don't have to do anything because
>>>    * userspace accesses to the VGIC state already require all VCPUs 
>>> to be
>>> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>>> *vcpu,
>>>    */
>>>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 
>>> intid)
>>>   {
>>> -    if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 ||
>>> +    if ((vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 &&
>>> +         vcpu == kvm_get_running_vcpu()) ||
>>
>> Thanks Marc for the patch!
>>
>> I think, you mean not equal to?
>> +           vcpu != kvm_get_running_vcpu()) ||
> 
> Yeah, exactly. I woke up this morning realising this patch was
> *almost* right. Don't write patches like this after a long day
> at work...
> 
>> With the change to not-equal, the issue is fixed and I could see the
>> NestedVM booting is pretty fast with higher number of cores as well.
> 
> Good, thanks for testing it. I'll roll up an actual patch for that
> and stick it in the monster queue.

Thanks, Please pull patch 3/3 also to nv-6.2 tree along with this patch. 
I will move my setup to nv-6.2 once these patches are in.

> 
> Cheers,
> 
>         M.


Thanks,
Ganapat

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On 2023-01-11 12:46, Ganapatrao Kulkarni wrote:
> On 11-01-2023 05:09 pm, Marc Zyngier wrote:
>> On 2023-01-11 08:46, Ganapatrao Kulkarni wrote:
>>> On 11-01-2023 03:24 am, Marc Zyngier wrote:
>>>> On Tue, 10 Jan 2023 12:17:20 +0000,
>>>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>>> 
>>>>> I am currently working around this with "nohlt" kernel param to
>>>>> NestedVM. Any suggestions to handle/fix this case/issue and avoid 
>>>>> the
>>>>> slowness of booting of NestedVM with more cores?
>>>>> 
>>>>> Note: Guest-Hypervisor and NestedVM are using default kernel 
>>>>> installed
>>>>> using Fedora 36 iso.
>>>> 
>>>> Despite what I said earlier, I have a vague idea here, thanks to the
>>>> interesting call traces that you provided (this is really awesome 
>>>> work
>>>> BTW, given how hard it is to trace things across 3 different 
>>>> kernels).
>>>> 
>>>> We can slightly limit the impact of the prepare/finish sequence if 
>>>> the
>>>> guest hypervisor only accesses the active registers for SGIs/PPIs on
>>>> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
>>>> access.
>>>> 
>>>> Something along these lines, which is only boot-tested. Let me know
>>>> how this fares for you.
>>>> 
>>>> Thanks,
>>>> 
>>>>     M.
>>>> 
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c 
>>>> b/arch/arm64/kvm/vgic/vgic-mmio.c
>>>> index b32d434c1d4a..1cca45be5335 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
>>>> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>>>> *vcpu,
>>>>    * active state can be overwritten when the VCPU's state is synced 
>>>> coming back
>>>>    * from the guest.
>>>>    *
>>>> - * For shared interrupts as well as GICv3 private interrupts, we 
>>>> have to
>>>> - * stop all the VCPUs because interrupts can be migrated while we 
>>>> don't hold
>>>> - * the IRQ locks and we don't want to be chasing moving targets.
>>>> + * For shared interrupts as well as GICv3 private interrupts 
>>>> accessed from the
>>>> + * non-owning CPU, we have to stop all the VCPUs because interrupts 
>>>> can be
>>>> + * migrated while we don't hold the IRQ locks and we don't want to 
>>>> be chasing
>>>> + * moving targets.
>>>>    *
>>>>    * For GICv2 private interrupts we don't have to do anything 
>>>> because
>>>>    * userspace accesses to the VGIC state already require all VCPUs 
>>>> to be
>>>> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>>>> *vcpu,
>>>>    */
>>>>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 
>>>> intid)
>>>>   {
>>>> -    if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 
>>>> ||
>>>> +    if ((vcpu->kvm->arch.vgic.vgic_model == 
>>>> KVM_DEV_TYPE_ARM_VGIC_V3 &&
>>>> +         vcpu == kvm_get_running_vcpu()) ||
>>> 
>>> Thanks Marc for the patch!
>>> 
>>> I think, you mean not equal to?
>>> +           vcpu != kvm_get_running_vcpu()) ||
>> 
>> Yeah, exactly. I woke up this morning realising this patch was
>> *almost* right. Don't write patches like this after a long day
>> at work...
>> 
>>> With the change to not-equal, the issue is fixed and I could see the
>>> NestedVM booting is pretty fast with higher number of cores as well.
>> 
>> Good, thanks for testing it. I'll roll up an actual patch for that
>> and stick it in the monster queue.
> 
> Thanks, Please pull patch 3/3 also to nv-6.2 tree along with this
> patch. I will move my setup to nv-6.2 once these patches are in.

3/3 should already be in the branch, merged with the shadow
S2 fault handling.

         M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH 0/3] KVM: arm64: nv: Fixes for Nested Virtualization issues

[Marc Zyngier]

On 2023-01-11 12:46, Ganapatrao Kulkarni wrote:
> On 11-01-2023 05:09 pm, Marc Zyngier wrote:
>> On 2023-01-11 08:46, Ganapatrao Kulkarni wrote:
>>> On 11-01-2023 03:24 am, Marc Zyngier wrote:
>>>> On Tue, 10 Jan 2023 12:17:20 +0000,
>>>> Ganapatrao Kulkarni <gankulkarni@os.amperecomputing.com> wrote:
>>>>> 
>>>>> I am currently working around this with "nohlt" kernel param to
>>>>> NestedVM. Any suggestions to handle/fix this case/issue and avoid 
>>>>> the
>>>>> slowness of booting of NestedVM with more cores?
>>>>> 
>>>>> Note: Guest-Hypervisor and NestedVM are using default kernel 
>>>>> installed
>>>>> using Fedora 36 iso.
>>>> 
>>>> Despite what I said earlier, I have a vague idea here, thanks to the
>>>> interesting call traces that you provided (this is really awesome 
>>>> work
>>>> BTW, given how hard it is to trace things across 3 different 
>>>> kernels).
>>>> 
>>>> We can slightly limit the impact of the prepare/finish sequence if 
>>>> the
>>>> guest hypervisor only accesses the active registers for SGIs/PPIs on
>>>> the vcpu that owns them, forbidding any cross-CPU-to-redistributor
>>>> access.
>>>> 
>>>> Something along these lines, which is only boot-tested. Let me know
>>>> how this fares for you.
>>>> 
>>>> Thanks,
>>>> 
>>>>     M.
>>>> 
>>>> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c 
>>>> b/arch/arm64/kvm/vgic/vgic-mmio.c
>>>> index b32d434c1d4a..1cca45be5335 100644
>>>> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
>>>> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
>>>> @@ -473,9 +473,10 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>>>> *vcpu,
>>>>    * active state can be overwritten when the VCPU's state is synced 
>>>> coming back
>>>>    * from the guest.
>>>>    *
>>>> - * For shared interrupts as well as GICv3 private interrupts, we 
>>>> have to
>>>> - * stop all the VCPUs because interrupts can be migrated while we 
>>>> don't hold
>>>> - * the IRQ locks and we don't want to be chasing moving targets.
>>>> + * For shared interrupts as well as GICv3 private interrupts 
>>>> accessed from the
>>>> + * non-owning CPU, we have to stop all the VCPUs because interrupts 
>>>> can be
>>>> + * migrated while we don't hold the IRQ locks and we don't want to 
>>>> be chasing
>>>> + * moving targets.
>>>>    *
>>>>    * For GICv2 private interrupts we don't have to do anything 
>>>> because
>>>>    * userspace accesses to the VGIC state already require all VCPUs 
>>>> to be
>>>> @@ -484,7 +485,8 @@ int vgic_uaccess_write_cpending(struct kvm_vcpu 
>>>> *vcpu,
>>>>    */
>>>>   static void vgic_access_active_prepare(struct kvm_vcpu *vcpu, u32 
>>>> intid)
>>>>   {
>>>> -    if (vcpu->kvm->arch.vgic.vgic_model == KVM_DEV_TYPE_ARM_VGIC_V3 
>>>> ||
>>>> +    if ((vcpu->kvm->arch.vgic.vgic_model == 
>>>> KVM_DEV_TYPE_ARM_VGIC_V3 &&
>>>> +         vcpu == kvm_get_running_vcpu()) ||
>>> 
>>> Thanks Marc for the patch!
>>> 
>>> I think, you mean not equal to?
>>> +           vcpu != kvm_get_running_vcpu()) ||
>> 
>> Yeah, exactly. I woke up this morning realising this patch was
>> *almost* right. Don't write patches like this after a long day
>> at work...
>> 
>>> With the change to not-equal, the issue is fixed and I could see the
>>> NestedVM booting is pretty fast with higher number of cores as well.
>> 
>> Good, thanks for testing it. I'll roll up an actual patch for that
>> and stick it in the monster queue.
> 
> Thanks, Please pull patch 3/3 also to nv-6.2 tree along with this
> patch. I will move my setup to nv-6.2 once these patches are in.

3/3 should already be in the branch, merged with the shadow
S2 fault handling.

         M.
-- 
Jazz is not dead. It just smells funny...