* [PATCH] KVM: arm64: Keep need_db always true in vgic_v4_put() when emulating WFI
@ 2023-07-07 8:55 chenxiang
2023-07-07 17:16 ` Oliver Upton
0 siblings, 1 reply; 3+ messages in thread
From: chenxiang @ 2023-07-07 8:55 UTC (permalink / raw)
To: maz, oliver.upton, james.morse; +Cc: kvmarm, kvm, linuxarm, Xiang Chen
From: Xiang Chen <chenxiang66@hisilicon.com>
When enabled GICv4.1 on Kunpeng 920 platform with 6.4 kernel (preemptible
kernel), running a vm with 128 vcpus on 64 pcpu, sometimes vm is not booted
successfully, and we find there is a situation that doorbell interrupt will
not occur event if there is a pending interrupt.
In function kvm_vcpu_wfi(), the parameter of need_db is true for function
vgic_v4_put() which wants to tell GICv4 that we need doorbells to be
signalled if there is a pending interrupt.
But if there is a preemption before kvm_vcpu_halt (after preempt_enable()),
it will change the value of vpe->resident, which will make vgic_v4_put()
is called with need_db = false When calling function schedule().
Then after calling schedule(), doorbell interrupt will not occur even
if there is a pending interrupt.
We need to keep need_db always true for emulate Wait-For-Interrupt
behavior, so set need_db true in vgic_v4_put() if it is in the process of
emulating WFI.
kvm_vcpu_wfi
preempt_disable
...
vgic_v4_put(vcpu, true)
vpe->resident = 0
need_db = 1
preempt_enable
------------------------------------------> preemption occur
kvm_sched_out
vgic_v4_put(vcpu, false)
vpe->resident == 0 -> return 0 (do nothing)
...
<------------------------------------------ back
kvm_sched_in
vgic_v4_load
vpe->resident = 1
...
------------------------------------------- Continue
kvm_vcpu_halt
set vcpu thread INTERRUPTIBLE
schedule() ->
kvm_sched_out
vgic_v4_put(vcpu, false)
need_db = 0 (vcpu thread is not scheduled and doorbell interrupt
will not signalled even if there is a pending interrupt)
vpe->resident = 0
Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
---
arch/arm64/kvm/vgic/vgic-v4.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
index c1c28fe..37152cf8 100644
--- a/arch/arm64/kvm/vgic/vgic-v4.c
+++ b/arch/arm64/kvm/vgic/vgic-v4.c
@@ -343,6 +343,9 @@ int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db)
if (!vgic_supports_direct_msis(vcpu->kvm) || !vpe->resident)
return 0;
+ if (vcpu->stat.generic.blocking == 1)
+ need_db = true;
+
return its_make_vpe_non_resident(vpe, need_db);
}
--
2.8.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: arm64: Keep need_db always true in vgic_v4_put() when emulating WFI
2023-07-07 8:55 [PATCH] KVM: arm64: Keep need_db always true in vgic_v4_put() when emulating WFI chenxiang
@ 2023-07-07 17:16 ` Oliver Upton
2023-07-10 2:04 ` chenxiang (M)
0 siblings, 1 reply; 3+ messages in thread
From: Oliver Upton @ 2023-07-07 17:16 UTC (permalink / raw)
To: chenxiang; +Cc: maz, james.morse, kvmarm, kvm, linuxarm
Hi Xiang,
Thanks for reporting this.
On Fri, Jul 07, 2023 at 04:55:45PM +0800, chenxiang wrote:
> When enabled GICv4.1 on Kunpeng 920 platform with 6.4 kernel (preemptible
> kernel), running a vm with 128 vcpus on 64 pcpu, sometimes vm is not booted
> successfully, and we find there is a situation that doorbell interrupt will
> not occur event if there is a pending interrupt.
Oh, that's no good.
> ---
> arch/arm64/kvm/vgic/vgic-v4.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
> index c1c28fe..37152cf8 100644
> --- a/arch/arm64/kvm/vgic/vgic-v4.c
> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
> @@ -343,6 +343,9 @@ int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db)
> if (!vgic_supports_direct_msis(vcpu->kvm) || !vpe->resident)
> return 0;
>
> + if (vcpu->stat.generic.blocking == 1)
> + need_db = true;
> +
As I understand it, the issue really comes from the fact that @need_db
is always false when called from vgic_v3_put(). I'd rather we not
override the argument, as that could have unintended consequences in the
future.
You could also use the helper we already have for determining if a vCPU
is blocking, which we could use as a hint for requesting a doorbell
interrupt on sched out.
Putting the two comments together, I arrived at the following (untested)
diff. I don't have a GICv4 system on hand, sadly. Mind taking it for a
spin?
--
Thanks,
Oliver
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 93a47a515c13..8c743643b122 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -756,7 +756,7 @@ void vgic_v3_put(struct kvm_vcpu *vcpu)
{
struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
- WARN_ON(vgic_v4_put(vcpu, false));
+ WARN_ON(vgic_v4_put(vcpu, kvm_vcpu_is_blocking(vcpu)));
vgic_v3_vmcr_sync(vcpu);
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: arm64: Keep need_db always true in vgic_v4_put() when emulating WFI
2023-07-07 17:16 ` Oliver Upton
@ 2023-07-10 2:04 ` chenxiang (M)
0 siblings, 0 replies; 3+ messages in thread
From: chenxiang (M) @ 2023-07-10 2:04 UTC (permalink / raw)
To: Oliver Upton; +Cc: maz, james.morse, kvmarm, kvm, linuxarm
Hi Oliver,
在 2023/7/8 星期六 1:16, Oliver Upton 写道:
> Hi Xiang,
>
> Thanks for reporting this.
>
> On Fri, Jul 07, 2023 at 04:55:45PM +0800, chenxiang wrote:
>> When enabled GICv4.1 on Kunpeng 920 platform with 6.4 kernel (preemptible
>> kernel), running a vm with 128 vcpus on 64 pcpu, sometimes vm is not booted
>> successfully, and we find there is a situation that doorbell interrupt will
>> not occur event if there is a pending interrupt.
> Oh, that's no good.
>
>> ---
>> arch/arm64/kvm/vgic/vgic-v4.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/vgic/vgic-v4.c b/arch/arm64/kvm/vgic/vgic-v4.c
>> index c1c28fe..37152cf8 100644
>> --- a/arch/arm64/kvm/vgic/vgic-v4.c
>> +++ b/arch/arm64/kvm/vgic/vgic-v4.c
>> @@ -343,6 +343,9 @@ int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db)
>> if (!vgic_supports_direct_msis(vcpu->kvm) || !vpe->resident)
>> return 0;
>>
>> + if (vcpu->stat.generic.blocking == 1)
>> + need_db = true;
>> +
> As I understand it, the issue really comes from the fact that @need_db
> is always false when called from vgic_v3_put(). I'd rather we not
> override the argument, as that could have unintended consequences in the
> future.
>
> You could also use the helper we already have for determining if a vCPU
> is blocking, which we could use as a hint for requesting a doorbell
> interrupt on sched out.
>
> Putting the two comments together, I arrived at the following (untested)
> diff. I don't have a GICv4 system on hand, sadly. Mind taking it for a
> spin?
Right, it is more suitable to use the helper we already have for
determining if a vCPU is blocking.
We have tested the following diff you provide, and it solves the issue,
so please feel free to add for the change:
Tested-by: Xiang Chen <chenxiang66@hisilicon.com>
>
> --
> Thanks,
> Oliver
>
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 93a47a515c13..8c743643b122 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -756,7 +756,7 @@ void vgic_v3_put(struct kvm_vcpu *vcpu)
> {
> struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>
> - WARN_ON(vgic_v4_put(vcpu, false));
> + WARN_ON(vgic_v4_put(vcpu, kvm_vcpu_is_blocking(vcpu)));
>
> vgic_v3_vmcr_sync(vcpu);
>
> .
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-10 2:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-07 8:55 [PATCH] KVM: arm64: Keep need_db always true in vgic_v4_put() when emulating WFI chenxiang
2023-07-07 17:16 ` Oliver Upton
2023-07-10 2:04 ` chenxiang (M)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.