kvm_arch_vcpu_runnable related improvements
Subsystems (4)
cover kvm_arch_vcpu_runnable related improvements
Only patch 5/5 fixes anything, and that's just a theoretical bug, but, IMHO, this series future-proofs and improves the maintainability of the code. See https://www.spinics.net/lists/kvm-arm/msg27254.html
for a TL;DR analysis that led to this series.
Thanks,
drew
Andrew Jones (5):
KVM: arm/arm64: tidy 'should sleep' conditions
KVM: arm/arm64: replace power_off with mp_state=STOPPED
KVM: arm/arm64: factor out common wfe/wfi emulation code
KVM: arm/arm64: improve kvm_arch_vcpu_runnable
KVM: arm/arm64: kvm_arch_vcpu_runnable: don't miss injected irqs
Documentation/virtual/kvm/api.txt | 10 +++--- arch/arm/include/asm/kvm_host.h | 8 +++-- arch/arm/kvm/handle_exit.c | 14 +++------ arch/arm64/include/asm/kvm_host.h | 8 +++-- arch/arm64/kvm/handle_exit.c | 14 +++------ virt/kvm/arm/arm.c | 64 +++++++++++++++++++++++++++------------ virt/kvm/arm/psci.c | 32 ++++++++------------ 7 files changed, 84 insertions(+), 66 deletions(-)
Signature · 1 line
2.13.5
PATCH 1/5 KVM: arm/arm64: tidy 'should sleep' conditions
Introduce vcpu_should_sleep() in order to tidy several places up that the compound 'power_off || pause' condition is used.
--- virt/kvm/arm/arm.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
virt/kvm/arm/arm.c +11 -7
@@ -403,17 +403,22 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,return 0 ; } +static bool vcpu_should_sleep (struct kvm_vcpu *vcpu) +{ +return vcpu->arch.power_off || vcpu->arch.pause; +} +/** * kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled -* @v: The VCPU pointer +* @vcpu: The VCPU pointer * * If the guest CPU is not waiting for interrupts or an interrupt line is * asserted, the CPU is by definition runnable. */ -int kvm_arch_vcpu_runnable (struct kvm_vcpu *v) +int kvm_arch_vcpu_runnable (struct kvm_vcpu *vcpu) { -return ((!!v->arch.irq_lines ||kvm_vgic_vcpu_pending_irq (v)) - && !v->arch.power_off && !v->arch.pause); +return (!!vcpu->arch.irq_lines ||kvm_vgic_vcpu_pending_irq (vcpu)) + && !vcpu_should_sleep (vcpu); }bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)
@@ -570,10 +575,9 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu) {struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu); -swait_event_interruptible (*wq, ((!vcpu->arch.power_off) && - (!vcpu->arch.pause))); +swait_event_interruptible (*wq, !vcpu_should_sleep (vcpu)); -if (vcpu->arch.power_off || vcpu->arch.pause) { +if (vcpu_should_sleep (vcpu)) {/* Awaken to handle a signal, request we sleep again later. */ kvm_make_request (KVM_REQ_SLEEP , vcpu); }
Signature · 1 line
2.13.5
Re: [PATCH 1/5] KVM: arm/arm64: tidy 'should sleep' conditions
Quoted 43 lines · Andrew Jones
Introduce
vcpu_should_sleep()in order to tidy several places up that the compound 'power_off|| pause' condition is used.Signed-off-by Andrew Jones <[email protected]>--- virt/kvm/arm/arm.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)virt/kvm/arm/arm.c +9
-4@@ -403,17 +403,22 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,return 0 ; } +static bool vcpu_should_sleep (struct kvm_vcpu *vcpu) +{ +return vcpu->arch.power_off || vcpu->arch.pause; +} +/** * kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled -* @v: The VCPU pointer +* @vcpu: The VCPU pointer * * If the guest CPU is not waiting for interrupts or an interrupt line is * asserted, the CPU is by definition runnable. */ -int kvm_arch_vcpu_runnable (struct kvm_vcpu *v) +int kvm_arch_vcpu_runnable (struct kvm_vcpu *vcpu) { -return ((!!v->arch.irq_lines ||kvm_vgic_vcpu_pending_irq (v)) - && !v->arch.power_off && !v->arch.pause); +return (!!vcpu->arch.irq_lines ||kvm_vgic_vcpu_pending_irq (vcpu)) + && !vcpu_should_sleep (vcpu); }bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)@@ -570,10 +575,9 @@ static void vcpu_req_sleep(struct kvm_vcpu *vcpu) {struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu);
virt/kvm/arm/arm.c static void vcpu_req_sleep(struct kvm_vcpu *vcpu) · line 583
Quoted 10 lines
virt/kvm/arm/arm.c +2
-3-swait_event_interruptible (*wq, ((!vcpu->arch.power_off) && - (!vcpu->arch.pause))); +swait_event_interruptible (*wq, !vcpu_should_sleep (vcpu)); -if (vcpu->arch.power_off || vcpu->arch.pause) { +if (vcpu_should_sleep (vcpu)) {/* Awaken to handle a signal, request we sleep again later. */ kvm_make_request (KVM_REQ_SLEEP , vcpu); }
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH 2/5 KVM: arm/arm64: replace power_off with mp_state=STOPPED
In reply to Andrew JonesThis prepares for more MP states to be used.
--- arch/arm/include/asm/kvm_host.h | 6 ++++-- arch/arm64/include/asm/kvm_host.h | 6 ++++-- virt/kvm/arm/arm.c | 26 ++++++++++++++------------ virt/kvm/arm/psci.c | 17 +++++------------ 4 files changed, 27 insertions(+), 28 deletions(-)
arch/arm/include/asm/kvm_host.h +4 -2
@@ -171,8 +171,8 @@ struct kvm_vcpu_arch {* here. */ -/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;/* Don't run the guest (internal implementation need) */ bool pause;
@@ -227,6 +227,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu __percpu **kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);
arch/arm64/include/asm/kvm_host.h +4 -2
@@ -255,8 +255,8 @@ struct kvm_vcpu_arch {u32 mdscr_el1; } guest_debug_preserved; -/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;/* Don't run the guest (internal implementation need) */ bool pause;
@@ -328,6 +328,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);
virt/kvm/arm/arm.c +14 -12
@@ -368,21 +368,23 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)kvm_timer_vcpu_put (vcpu); } -static void vcpu_power_off (struct kvm_vcpu *vcpu) +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu) { - vcpu->arch.power_off = true; + vcpu->arch.mp_state =KVM_MP_STATE_STOPPED ;kvm_make_request (KVM_REQ_SLEEP , vcpu);kvm_vcpu_kick (vcpu); } +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu) +{ + vcpu->arch.mp_state =KVM_MP_STATE_RUNNABLE ; +kvm_vcpu_kick (vcpu); +} +int kvm_arch_vcpu_ioctl_get_mpstate (struct kvm_vcpu *vcpu,struct kvm_mp_state *mp_state) { -if (vcpu->arch.power_off) - mp_state->mp_state =KVM_MP_STATE_STOPPED ; -else - mp_state->mp_state =KVM_MP_STATE_RUNNABLE ; - + mp_state->mp_state = vcpu->arch.mp_state;return 0 ; }
@@ -391,10 +393,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu, {switch (mp_state->mp_state) {case KVM_MP_STATE_RUNNABLE : - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);break ;case KVM_MP_STATE_STOPPED : -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);break ;default :return -EINVAL ;
@@ -405,7 +407,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,static bool vcpu_should_sleep (struct kvm_vcpu *vcpu) { -return vcpu->arch.power_off || vcpu->arch.pause; +return vcpu->arch.mp_state ==KVM_MP_STATE_STOPPED || vcpu->arch.pause; }/**
@@ -919,9 +921,9 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,* Handle the "start in power-off" case. */ if (test_bit (KVM_ARM_VCPU_POWER_OFF , vcpu->arch.features)) -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);else - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);return 0 ; }
virt/kvm/arm/psci.c +5 -12
@@ -64,16 +64,13 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)static void kvm_psci_vcpu_off (struct kvm_vcpu *vcpu) { - vcpu->arch.power_off = true; -kvm_make_request (KVM_REQ_SLEEP , vcpu); -kvm_vcpu_kick (vcpu); +kvm_arm_vcpu_power_off (vcpu); }static unsigned long kvm_psci_vcpu_on (struct kvm_vcpu *source_vcpu) {struct kvm *kvm = source_vcpu->kvm;struct kvm_vcpu *vcpu =NULL ; -struct swait_queue_head *wq;unsigned long cpu_id;unsigned long context_id;phys_addr_t target_pc;
@@ -90,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)*/ if (!vcpu)return PSCI_RET_INVALID_PARAMS ; -if (!vcpu->arch.power_off) { +if (vcpu->arch.mp_state !=KVM_MP_STATE_STOPPED ) {if (kvm_psci_version (source_vcpu) !=KVM_ARM_PSCI_0_1 )return PSCI_RET_ALREADY_ON ;else
@@ -118,11 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)* the general puspose registers are undefined upon CPU_ON. */ vcpu_set_reg (vcpu,0 , context_id); - vcpu->arch.power_off = false; -smp_mb ();/* Make sure the above is visible */ - - wq =kvm_arch_vcpu_wq (vcpu); -swake_up (wq); +kvm_arm_vcpu_power_on (vcpu);return PSCI_RET_SUCCESS ; }
@@ -156,7 +149,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu) mpidr =kvm_vcpu_get_mpidr_aff (tmp);if ((mpidr & target_affinity_mask) == target_affinity) { matching_cpus++; -if (!tmp->arch.power_off) +if (tmp->arch.mp_state !=KVM_MP_STATE_STOPPED )return PSCI_0_2_AFFINITY_LEVEL_ON ; } }
@@ -182,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)* re-initialized. */ kvm_for_each_vcpu (i, tmp, vcpu->kvm) - tmp->arch.power_off = true; + tmp->arch.mp_state =KVM_MP_STATE_STOPPED ;kvm_make_all_cpus_request (vcpu->kvm,KVM_REQ_SLEEP );memset (&vcpu->run->system_event,0 ,sizeof (vcpu->run->system_event));
Signature · 1 line
2.13.5
Re: [PATCH 2/5] KVM: arm/arm64: replace power_off with mp_state=STOPPED
Quoted 15 lines · Andrew Jones
This prepares for more MP states to be used.
Signed-off-by Andrew Jones <[email protected]>--- arch/arm/include/asm/kvm_host.h | 6 ++++-- arch/arm64/include/asm/kvm_host.h | 6 ++++-- virt/kvm/arm/arm.c | 26 ++++++++++++++------------ virt/kvm/arm/psci.c | 17 +++++------------ 4 files changed, 27 insertions(+), 28 deletions(-)arch/arm/include/asm/kvm_host.h +0
-0
arch/arm/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 175
Quoted 7 lines
arch/arm/include/asm/kvm_host.h +2
-2@@ -171,8 +171,8 @@ struct kvm_vcpu_arch {* here. */ -/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;
What is the rational for not using a struct kvm_mp_state directly here? It is the same thing (a u32), but I certainly find it more explicit than a bare u32.
Quoted 128 lines
arch/arm/include/asm/kvm_host.h +2
-0/* Don't run the guest (internal implementation need) */ bool pause;@@ -227,6 +227,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu __percpu **kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);arch/arm64/include/asm/kvm_host.h +4
-2@@ -255,8 +255,8 @@ struct kvm_vcpu_arch {u32 mdscr_el1; } guest_debug_preserved; -/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;/* Don't run the guest (internal implementation need) */ bool pause;@@ -328,6 +328,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);virt/kvm/arm/arm.c +14
-12@@ -368,21 +368,23 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)kvm_timer_vcpu_put (vcpu); } -static void vcpu_power_off (struct kvm_vcpu *vcpu) +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu) { - vcpu->arch.power_off = true; + vcpu->arch.mp_state =KVM_MP_STATE_STOPPED ;kvm_make_request (KVM_REQ_SLEEP , vcpu);kvm_vcpu_kick (vcpu); } +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu) +{ + vcpu->arch.mp_state =KVM_MP_STATE_RUNNABLE ; +kvm_vcpu_kick (vcpu); +} +int kvm_arch_vcpu_ioctl_get_mpstate (struct kvm_vcpu *vcpu,struct kvm_mp_state *mp_state) { -if (vcpu->arch.power_off) - mp_state->mp_state =KVM_MP_STATE_STOPPED ; -else - mp_state->mp_state =KVM_MP_STATE_RUNNABLE ; - + mp_state->mp_state = vcpu->arch.mp_state;return 0 ; }@@ -391,10 +393,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu, {switch (mp_state->mp_state) {case KVM_MP_STATE_RUNNABLE : - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);break ;case KVM_MP_STATE_STOPPED : -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);break ;default :return -EINVAL ;@@ -405,7 +407,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,static bool vcpu_should_sleep (struct kvm_vcpu *vcpu) { -return vcpu->arch.power_off || vcpu->arch.pause; +return vcpu->arch.mp_state ==KVM_MP_STATE_STOPPED || vcpu->arch.pause; }/** @@ -919,9 +921,9 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,* Handle the "start in power-off" case. */ if (test_bit (KVM_ARM_VCPU_POWER_OFF , vcpu->arch.features)) -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);else - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);return 0 ; }virt/kvm/arm/psci.c +1
-4@@ -64,16 +64,13 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)static void kvm_psci_vcpu_off (struct kvm_vcpu *vcpu) { - vcpu->arch.power_off = true; -kvm_make_request (KVM_REQ_SLEEP , vcpu); -kvm_vcpu_kick (vcpu); +kvm_arm_vcpu_power_off (vcpu); }static unsigned long kvm_psci_vcpu_on (struct kvm_vcpu *source_vcpu) {struct kvm *kvm = source_vcpu->kvm;struct kvm_vcpu *vcpu =NULL ; -struct swait_queue_head *wq;unsigned long cpu_id;unsigned long context_id;phys_addr_t target_pc;
virt/kvm/arm/psci.c static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) · line 90
Quoted 5 lines
virt/kvm/arm/psci.c +1
-1@@ -90,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)*/ if (!vcpu)return PSCI_RET_INVALID_PARAMS ; -if (!vcpu->arch.power_off) { +if (vcpu->arch.mp_state !=KVM_MP_STATE_STOPPED ) {
Would it make sense to have a new helper to wrap that condition (also used below)?
virt/kvm/arm/psci.c static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) · line 118
Quoted 13 lines
virt/kvm/arm/psci.c +1
-5if (kvm_psci_version (source_vcpu) !=KVM_ARM_PSCI_0_1 )return PSCI_RET_ALREADY_ON ;else @@ -118,11 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)* the general puspose registers are undefined upon CPU_ON. */ vcpu_set_reg (vcpu,0 , context_id); - vcpu->arch.power_off = false; -smp_mb ();/* Make sure the above is visible */ - - wq =kvm_arch_vcpu_wq (vcpu); -swake_up (wq); +kvm_arm_vcpu_power_on (vcpu);
What guarantees that the mp_state update is now visible to other agents? Or was the smp_mb() here superfluous? This should anyhow deserve a mention in the commit message.
Quoted 13 lines
virt/kvm/arm/psci.c +1
-1return PSCI_RET_SUCCESS ; }@@ -156,7 +149,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu) mpidr =kvm_vcpu_get_mpidr_aff (tmp);if ((mpidr & target_affinity_mask) == target_affinity) { matching_cpus++; -if (!tmp->arch.power_off) +if (tmp->arch.mp_state !=KVM_MP_STATE_STOPPED )return PSCI_0_2_AFFINITY_LEVEL_ON ; } }
virt/kvm/arm/psci.c static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type) · line 178
Quoted 5 lines
virt/kvm/arm/psci.c +1
-1@@ -182,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)* re-initialized. */ kvm_for_each_vcpu (i, tmp, vcpu->kvm) - tmp->arch.power_off = true; + tmp->arch.mp_state =KVM_MP_STATE_STOPPED ;
Ah, shame we can't get rid of this with a power_off...
virt/kvm/arm/psci.c static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type) · line 181
Quoted 4 lines
virt/kvm/arm/psci.c +0
-0kvm_make_all_cpus_request (vcpu->kvm,KVM_REQ_SLEEP );memset (&vcpu->run->system_event,0 ,sizeof (vcpu->run->system_event));
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH 2/5] KVM: arm/arm64: replace power_off with mp_state=STOPPED
Quoted 19 lines · Marc Zyngier
Quoted 18 lines · Andrew Jones
This prepares for more MP states to be used.
Signed-off-by Andrew Jones <[email protected]>--- arch/arm/include/asm/kvm_host.h | 6 ++++-- arch/arm64/include/asm/kvm_host.h | 6 ++++-- virt/kvm/arm/arm.c | 26 ++++++++++++++------------ virt/kvm/arm/psci.c | 17 +++++------------ 4 files changed, 27 insertions(+), 28 deletions(-)arch/arm/include/asm/kvm_host.h +0
-0@@ -171,8 +171,8 @@ struct kvm_vcpu_arch {* here. */
Quoted 8 lines
arch/arm/include/asm/kvm_host.h struct kvm_vcpu_arch { · line 175
Quoted 4 lines
arch/arm/include/asm/kvm_host.h +2
-2-/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;What is the rational for not using a struct
kvm_mp_statedirectly here? It is the same thing (a u32), but I certainly find it more explicit than a bare u32.
No good rationale, only inspiration from reading x86 code. I agree using the struct makes more sense, though, so I'll change it.
Quoted 129 lines
Quoted 129 lines
arch/arm/include/asm/kvm_host.h +2
-0/* Don't run the guest (internal implementation need) */ bool pause;@@ -227,6 +227,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu __percpu **kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);arch/arm64/include/asm/kvm_host.h +4
-2@@ -255,8 +255,8 @@ struct kvm_vcpu_arch {u32 mdscr_el1; } guest_debug_preserved; -/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;/* Don't run the guest (internal implementation need) */ bool pause;@@ -328,6 +328,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);virt/kvm/arm/arm.c +14
-12@@ -368,21 +368,23 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)kvm_timer_vcpu_put (vcpu); } -static void vcpu_power_off (struct kvm_vcpu *vcpu) +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu) { - vcpu->arch.power_off = true; + vcpu->arch.mp_state =KVM_MP_STATE_STOPPED ;kvm_make_request (KVM_REQ_SLEEP , vcpu);kvm_vcpu_kick (vcpu); } +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu) +{ + vcpu->arch.mp_state =KVM_MP_STATE_RUNNABLE ; +kvm_vcpu_kick (vcpu); +} +int kvm_arch_vcpu_ioctl_get_mpstate (struct kvm_vcpu *vcpu,struct kvm_mp_state *mp_state) { -if (vcpu->arch.power_off) - mp_state->mp_state =KVM_MP_STATE_STOPPED ; -else - mp_state->mp_state =KVM_MP_STATE_RUNNABLE ; - + mp_state->mp_state = vcpu->arch.mp_state;return 0 ; }@@ -391,10 +393,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu, {switch (mp_state->mp_state) {case KVM_MP_STATE_RUNNABLE : - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);break ;case KVM_MP_STATE_STOPPED : -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);break ;default :return -EINVAL ;@@ -405,7 +407,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,static bool vcpu_should_sleep (struct kvm_vcpu *vcpu) { -return vcpu->arch.power_off || vcpu->arch.pause; +return vcpu->arch.mp_state ==KVM_MP_STATE_STOPPED || vcpu->arch.pause; }/** @@ -919,9 +921,9 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,* Handle the "start in power-off" case. */ if (test_bit (KVM_ARM_VCPU_POWER_OFF , vcpu->arch.features)) -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);else - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);return 0 ; }virt/kvm/arm/psci.c +1
-4@@ -64,16 +64,13 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)static void kvm_psci_vcpu_off (struct kvm_vcpu *vcpu) { - vcpu->arch.power_off = true; -kvm_make_request (KVM_REQ_SLEEP , vcpu); -kvm_vcpu_kick (vcpu); +kvm_arm_vcpu_power_off (vcpu); }static unsigned long kvm_psci_vcpu_on (struct kvm_vcpu *source_vcpu) {struct kvm *kvm = source_vcpu->kvm;struct kvm_vcpu *vcpu =NULL ; -struct swait_queue_head *wq;unsigned long cpu_id;unsigned long context_id;phys_addr_t target_pc;@@ -90,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)
Quoted 8 lines
virt/kvm/arm/psci.c static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) · line 90
Quoted 5 lines
virt/kvm/arm/psci.c +1
-1*/ if (!vcpu)return PSCI_RET_INVALID_PARAMS ; -if (!vcpu->arch.power_off) { +if (vcpu->arch.mp_state !=KVM_MP_STATE_STOPPED ) {Would it make sense to have a new helper to wrap that condition (also used below)?
I'll see how it looks.
Quoted 10 lines
Quoted 10 lines
virt/kvm/arm/psci.c +0
-2if (kvm_psci_version (source_vcpu) !=KVM_ARM_PSCI_0_1 )return PSCI_RET_ALREADY_ON ;else @@ -118,11 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)* the general puspose registers are undefined upon CPU_ON. */ vcpu_set_reg (vcpu,0 , context_id); - vcpu->arch.power_off = false; -smp_mb ();/* Make sure the above is visible */
Quoted 8 lines
virt/kvm/arm/psci.c static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) · line 118
Quoted 4 lines
virt/kvm/arm/psci.c +1
-3- - wq =kvm_arch_vcpu_wq (vcpu); -swake_up (wq); +kvm_arm_vcpu_power_on (vcpu);What guarantees that the
mp_stateupdate is now visible to other agents? Or was thesmp_mb()here superfluous? This should anyhow deserve a mention in the commit message.
The smp_mb() was superfluous, as this has the same pattern as other uses of prepare_to_swait/swake_up. I'll update the commit message with more detail.
Quoted 22 lines
Quoted 14 lines
virt/kvm/arm/psci.c +1
-1return PSCI_RET_SUCCESS ; }@@ -156,7 +149,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu) mpidr =kvm_vcpu_get_mpidr_aff (tmp);if ((mpidr & target_affinity_mask) == target_affinity) { matching_cpus++; -if (!tmp->arch.power_off) +if (tmp->arch.mp_state !=KVM_MP_STATE_STOPPED )return PSCI_0_2_AFFINITY_LEVEL_ON ; } }virt/kvm/arm/psci.c static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type) · line 178
Quoted 5 lines
virt/kvm/arm/psci.c +1
-1@@ -182,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)* re-initialized. */ kvm_for_each_vcpu (i, tmp, vcpu->kvm) - tmp->arch.power_off = true; + tmp->arch.mp_state =KVM_MP_STATE_STOPPED ;Ah, shame we can't get rid of this with a
power_off...
Quoted 10 lines
virt/kvm/arm/psci.c static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type) · line 181
Quoted 4 lines
virt/kvm/arm/psci.c +0
-0kvm_make_all_cpus_request (vcpu->kvm,KVM_REQ_SLEEP );memset (&vcpu->run->system_event,0 ,sizeof (vcpu->run->system_event));Thanks,
M.Signature · 1 line
Jazz is not dead. It just smells funny...
Thanks for the review!
drew
Re: [PATCH 2/5] KVM: arm/arm64: replace power_off with mp_state=STOPPED
In reply to Andrew JonesQuoted 69 lines · Andrew Jones
This prepares for more MP states to be used.
Signed-off-by Andrew Jones <[email protected]>--- arch/arm/include/asm/kvm_host.h | 6 ++++-- arch/arm64/include/asm/kvm_host.h | 6 ++++-- virt/kvm/arm/arm.c | 26 ++++++++++++++------------ virt/kvm/arm/psci.c | 17 +++++------------ 4 files changed, 27 insertions(+), 28 deletions(-)arch/arm/include/asm/kvm_host.h +4
-2@@ -171,8 +171,8 @@ struct kvm_vcpu_arch {* here. */ -/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;/* Don't run the guest (internal implementation need) */ bool pause;@@ -227,6 +227,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu __percpu **kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);arch/arm64/include/asm/kvm_host.h +4
-2@@ -255,8 +255,8 @@ struct kvm_vcpu_arch {u32 mdscr_el1; } guest_debug_preserved; -/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;/* Don't run the guest (internal implementation need) */ bool pause;@@ -328,6 +328,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);virt/kvm/arm/arm.c +1
-1@@ -368,21 +368,23 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)kvm_timer_vcpu_put (vcpu); } -static void vcpu_power_off (struct kvm_vcpu *vcpu) +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu) {
virt/kvm/arm/arm.c void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) · line 381
Quoted 10 lines
virt/kvm/arm/arm.c +5
-1- vcpu->arch.power_off = true; + vcpu->arch.mp_state =KVM_MP_STATE_STOPPED ;kvm_make_request (KVM_REQ_SLEEP , vcpu);kvm_vcpu_kick (vcpu); } +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu) +{ + vcpu->arch.mp_state =KVM_MP_STATE_RUNNABLE ; +kvm_vcpu_kick (vcpu);
Given your rant about request-less kicks, shouldn't this be a kvm_vcpu_wake_up() ?
Quoted 113 lines
virt/kvm/arm/arm.c +8
-10+} +int kvm_arch_vcpu_ioctl_get_mpstate (struct kvm_vcpu *vcpu,struct kvm_mp_state *mp_state) { -if (vcpu->arch.power_off) - mp_state->mp_state =KVM_MP_STATE_STOPPED ; -else - mp_state->mp_state =KVM_MP_STATE_RUNNABLE ; - + mp_state->mp_state = vcpu->arch.mp_state;return 0 ; }@@ -391,10 +393,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu, {switch (mp_state->mp_state) {case KVM_MP_STATE_RUNNABLE : - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);break ;case KVM_MP_STATE_STOPPED : -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);break ;default :return -EINVAL ;@@ -405,7 +407,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,static bool vcpu_should_sleep (struct kvm_vcpu *vcpu) { -return vcpu->arch.power_off || vcpu->arch.pause; +return vcpu->arch.mp_state ==KVM_MP_STATE_STOPPED || vcpu->arch.pause; }/** @@ -919,9 +921,9 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,* Handle the "start in power-off" case. */ if (test_bit (KVM_ARM_VCPU_POWER_OFF , vcpu->arch.features)) -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);else - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);return 0 ; }virt/kvm/arm/psci.c +5
-12@@ -64,16 +64,13 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)static void kvm_psci_vcpu_off (struct kvm_vcpu *vcpu) { - vcpu->arch.power_off = true; -kvm_make_request (KVM_REQ_SLEEP , vcpu); -kvm_vcpu_kick (vcpu); +kvm_arm_vcpu_power_off (vcpu); }static unsigned long kvm_psci_vcpu_on (struct kvm_vcpu *source_vcpu) {struct kvm *kvm = source_vcpu->kvm;struct kvm_vcpu *vcpu =NULL ; -struct swait_queue_head *wq;unsigned long cpu_id;unsigned long context_id;phys_addr_t target_pc;@@ -90,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)*/ if (!vcpu)return PSCI_RET_INVALID_PARAMS ; -if (!vcpu->arch.power_off) { +if (vcpu->arch.mp_state !=KVM_MP_STATE_STOPPED ) {if (kvm_psci_version (source_vcpu) !=KVM_ARM_PSCI_0_1 )return PSCI_RET_ALREADY_ON ;else @@ -118,11 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)* the general puspose registers are undefined upon CPU_ON. */ vcpu_set_reg (vcpu,0 , context_id); - vcpu->arch.power_off = false; -smp_mb ();/* Make sure the above is visible */ - - wq =kvm_arch_vcpu_wq (vcpu); -swake_up (wq); +kvm_arm_vcpu_power_on (vcpu);return PSCI_RET_SUCCESS ; }@@ -156,7 +149,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu) mpidr =kvm_vcpu_get_mpidr_aff (tmp);if ((mpidr & target_affinity_mask) == target_affinity) { matching_cpus++; -if (!tmp->arch.power_off) +if (tmp->arch.mp_state !=KVM_MP_STATE_STOPPED )return PSCI_0_2_AFFINITY_LEVEL_ON ; } }@@ -182,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)* re-initialized. */ kvm_for_each_vcpu (i, tmp, vcpu->kvm) - tmp->arch.power_off = true; + tmp->arch.mp_state =KVM_MP_STATE_STOPPED ;kvm_make_all_cpus_request (vcpu->kvm,KVM_REQ_SLEEP );memset (&vcpu->run->system_event,0 ,sizeof (vcpu->run->system_event));Signature · 1 line
2.13.5
Otherwise looks good to me:
Re: [PATCH 2/5] KVM: arm/arm64: replace power_off with mp_state=STOPPED
Quoted 76 lines · Christoffer Dall
Quoted 75 lines · Andrew Jones
This prepares for more MP states to be used.
Signed-off-by Andrew Jones <[email protected]>--- arch/arm/include/asm/kvm_host.h | 6 ++++-- arch/arm64/include/asm/kvm_host.h | 6 ++++-- virt/kvm/arm/arm.c | 26 ++++++++++++++------------ virt/kvm/arm/psci.c | 17 +++++------------ 4 files changed, 27 insertions(+), 28 deletions(-)arch/arm/include/asm/kvm_host.h +4
-2@@ -171,8 +171,8 @@ struct kvm_vcpu_arch {* here. */ -/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;/* Don't run the guest (internal implementation need) */ bool pause;@@ -227,6 +227,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu __percpu **kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);arch/arm64/include/asm/kvm_host.h +4
-2@@ -255,8 +255,8 @@ struct kvm_vcpu_arch {u32 mdscr_el1; } guest_debug_preserved; -/* vcpu power-off state */ -bool power_off; +/* Current VCPU MP state */ +u32 mp_state;/* Don't run the guest (internal implementation need) */ bool pause;@@ -328,6 +328,8 @@ int kvm_test_age_hva(struct kvm *kvm, unsigned long hva);struct kvm_vcpu *kvm_arm_get_running_vcpu (void );struct kvm_vcpu *__percpu *kvm_get_running_vcpus (void ); +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu); +void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm);virt/kvm/arm/arm.c +2
-2@@ -368,21 +368,23 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)kvm_timer_vcpu_put (vcpu); } -static void vcpu_power_off (struct kvm_vcpu *vcpu) +void kvm_arm_vcpu_power_off (struct kvm_vcpu *vcpu) { - vcpu->arch.power_off = true; + vcpu->arch.mp_state =KVM_MP_STATE_STOPPED ;kvm_make_request (KVM_REQ_SLEEP , vcpu);kvm_vcpu_kick (vcpu); }
Quoted 7 lines
virt/kvm/arm/arm.c void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) · line 381
Quoted 4 lines
virt/kvm/arm/arm.c +4
-0+void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu) +{ + vcpu->arch.mp_state =KVM_MP_STATE_RUNNABLE ; +kvm_vcpu_kick (vcpu);Given your rant about request-less kicks, shouldn't this be a
kvm_vcpu_wake_up()?
Wow, people actually listen to my rants? :-) Yes, I agree that kvm_vcpu_wake_up() is more appropriate. I'll change that.
Quoted 110 lines
Quoted 110 lines
virt/kvm/arm/arm.c +8
-10+} +int kvm_arch_vcpu_ioctl_get_mpstate (struct kvm_vcpu *vcpu,struct kvm_mp_state *mp_state) { -if (vcpu->arch.power_off) - mp_state->mp_state =KVM_MP_STATE_STOPPED ; -else - mp_state->mp_state =KVM_MP_STATE_RUNNABLE ; - + mp_state->mp_state = vcpu->arch.mp_state;return 0 ; }@@ -391,10 +393,10 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu, {switch (mp_state->mp_state) {case KVM_MP_STATE_RUNNABLE : - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);break ;case KVM_MP_STATE_STOPPED : -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);break ;default :return -EINVAL ;@@ -405,7 +407,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,static bool vcpu_should_sleep (struct kvm_vcpu *vcpu) { -return vcpu->arch.power_off || vcpu->arch.pause; +return vcpu->arch.mp_state ==KVM_MP_STATE_STOPPED || vcpu->arch.pause; }/** @@ -919,9 +921,9 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu,* Handle the "start in power-off" case. */ if (test_bit (KVM_ARM_VCPU_POWER_OFF , vcpu->arch.features)) -vcpu_power_off (vcpu); +kvm_arm_vcpu_power_off (vcpu);else - vcpu->arch.power_off = false; +kvm_arm_vcpu_power_on (vcpu);return 0 ; }virt/kvm/arm/psci.c +5
-12@@ -64,16 +64,13 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)static void kvm_psci_vcpu_off (struct kvm_vcpu *vcpu) { - vcpu->arch.power_off = true; -kvm_make_request (KVM_REQ_SLEEP , vcpu); -kvm_vcpu_kick (vcpu); +kvm_arm_vcpu_power_off (vcpu); }static unsigned long kvm_psci_vcpu_on (struct kvm_vcpu *source_vcpu) {struct kvm *kvm = source_vcpu->kvm;struct kvm_vcpu *vcpu =NULL ; -struct swait_queue_head *wq;unsigned long cpu_id;unsigned long context_id;phys_addr_t target_pc;@@ -90,7 +87,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)*/ if (!vcpu)return PSCI_RET_INVALID_PARAMS ; -if (!vcpu->arch.power_off) { +if (vcpu->arch.mp_state !=KVM_MP_STATE_STOPPED ) {if (kvm_psci_version (source_vcpu) !=KVM_ARM_PSCI_0_1 )return PSCI_RET_ALREADY_ON ;else @@ -118,11 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu)* the general puspose registers are undefined upon CPU_ON. */ vcpu_set_reg (vcpu,0 , context_id); - vcpu->arch.power_off = false; -smp_mb ();/* Make sure the above is visible */ - - wq =kvm_arch_vcpu_wq (vcpu); -swake_up (wq); +kvm_arm_vcpu_power_on (vcpu);return PSCI_RET_SUCCESS ; }@@ -156,7 +149,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu) mpidr =kvm_vcpu_get_mpidr_aff (tmp);if ((mpidr & target_affinity_mask) == target_affinity) { matching_cpus++; -if (!tmp->arch.power_off) +if (tmp->arch.mp_state !=KVM_MP_STATE_STOPPED )return PSCI_0_2_AFFINITY_LEVEL_ON ; } }@@ -182,7 +175,7 @@ static void kvm_prepare_system_event(struct kvm_vcpu *vcpu, u32 type)* re-initialized. */ kvm_for_each_vcpu (i, tmp, vcpu->kvm) - tmp->arch.power_off = true; + tmp->arch.mp_state =KVM_MP_STATE_STOPPED ;kvm_make_all_cpus_request (vcpu->kvm,KVM_REQ_SLEEP );
Quoted 8 lines
Quoted 4 lines
virt/kvm/arm/psci.c +0
-0memset (&vcpu->run->system_event,0 ,sizeof (vcpu->run->system_event));Signature · 1 line
2.13.5Otherwise looks good to me:
Reviewed-by Christoffer Dall <[email protected]>
Thanks,
drew
PATCH 3/5 KVM: arm/arm64: factor out common wfe/wfi emulation code
In reply to Andrew JonesBefore we add more common code to the wfi emulation, let's first factor out everything common.
--- arch/arm/include/asm/kvm_host.h | 2 ++ arch/arm/kvm/handle_exit.c | 14 +++++--------- arch/arm64/include/asm/kvm_host.h | 2 ++ arch/arm64/kvm/handle_exit.c | 14 +++++--------- virt/kvm/arm/arm.c | 13 +++++++++++++ virt/kvm/arm/psci.c | 15 +++++++-------- 6 files changed, 34 insertions(+), 26 deletions(-)
arch/arm/include/asm/kvm_host.h +2 -0
@@ -231,6 +231,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu); +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);
arch/arm/kvm/handle_exit.c +5 -9
@@ -57,22 +57,18 @@ static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)* @run: the kvm_run structure pointer * * WFE: Yield the CPU and come back to this vcpu when the scheduler -* decides to. -* WFI: Simply call kvm_vcpu_block(), which will halt execution of -* world-switches and schedule other host processes until there is an -* incoming IRQ or FIQ to the VM. +* decides to. +* WFI: Halt execution of world-switches and schedule other host +* processes until there is an incoming IRQ or FIQ to the VM. */ static int kvm_handle_wfx (struct kvm_vcpu *vcpu,struct kvm_run *run) {if (kvm_vcpu_get_hsr (vcpu) &HSR_WFI_IS_WFE ) {trace_kvm_wfx (*vcpu_pc (vcpu), true); - vcpu->stat.wfe_exit_stat++; -kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +kvm_arm_emulate_wfe (vcpu); }else {trace_kvm_wfx (*vcpu_pc (vcpu), false); - vcpu->stat.wfi_exit_stat++; -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); +kvm_arm_emulate_wfi (vcpu); }kvm_skip_instr (vcpu,kvm_vcpu_trap_il_is32bit (vcpu));
arch/arm64/include/asm/kvm_host.h +2 -0
@@ -332,6 +332,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu); +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)
arch/arm64/kvm/handle_exit.c +5 -9
@@ -74,22 +74,18 @@ static int handle_no_fpsimd(struct kvm_vcpu *vcpu, struct kvm_run *run)* @vcpu: the vcpu pointer * * WFE: Yield the CPU and come back to this vcpu when the scheduler -* decides to. -* WFI: Simply call kvm_vcpu_block(), which will halt execution of -* world-switches and schedule other host processes until there is an -* incoming IRQ or FIQ to the VM. +* decides to. +* WFI: Halt execution of world-switches and schedule other host +* processes until there is an incoming IRQ or FIQ to the VM. */ static int kvm_handle_wfx (struct kvm_vcpu *vcpu,struct kvm_run *run) {if (kvm_vcpu_get_hsr (vcpu) & ESR_ELx_WFx_ISS_WFE) {trace_kvm_wfx_arm64 (*vcpu_pc (vcpu), true); - vcpu->stat.wfe_exit_stat++; -kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +kvm_arm_emulate_wfe (vcpu); }else {trace_kvm_wfx_arm64 (*vcpu_pc (vcpu), false); - vcpu->stat.wfi_exit_stat++; -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); +kvm_arm_emulate_wfi (vcpu); }kvm_skip_instr (vcpu,kvm_vcpu_trap_il_is32bit (vcpu));
virt/kvm/arm/arm.c +13 -0
@@ -573,6 +573,19 @@ void kvm_arm_resume_guest(struct kvm *kvm) } } +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu) +{ + vcpu->stat.wfe_exit_stat++; +kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +} + +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu) +{ + vcpu->stat.wfi_exit_stat++; +kvm_vcpu_block (vcpu); +kvm_clear_request (KVM_REQ_UNHALT , vcpu); +} +static void vcpu_req_sleep (struct kvm_vcpu *vcpu) {struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu);
virt/kvm/arm/psci.c +7 -8
@@ -50,20 +50,19 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)* This means for KVM the wakeup events are interrupts and * this is consistent with intended use of StateID as described * in section 5.4.1 of PSCI v0.2 specification (ARM DEN 0022A). -* -* Further, we also treat power-down request to be same as -* stand-by request as-per section 5.4.2 clause 3 of PSCI v0.2 -* specification (ARM DEN 0022A). This means all suspend states -* for KVM will preserve the register state. */ -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); - +kvm_arm_emulate_wfi (vcpu);return PSCI_RET_SUCCESS ; }static void kvm_psci_vcpu_off (struct kvm_vcpu *vcpu) { +/* +* We treat a power-off request the same as a stand-by request, +* as-per section 5.4.2 clause 3 of PSCI v0.2* specification +* (ARM DEN 0022A). This means all suspend states for KVM will +* preserve the register state. +*/ kvm_arm_vcpu_power_off (vcpu); }
Signature · 1 line
2.13.5
Re: [PATCH 3/5] KVM: arm/arm64: factor out common wfe/wfi emulation code
Quoted 150 lines · Andrew Jones
Before we add more common code to the wfi emulation, let's first factor out everything common.
Signed-off-by Andrew Jones <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 ++ arch/arm/kvm/handle_exit.c | 14 +++++--------- arch/arm64/include/asm/kvm_host.h | 2 ++ arch/arm64/kvm/handle_exit.c | 14 +++++--------- virt/kvm/arm/arm.c | 13 +++++++++++++ virt/kvm/arm/psci.c | 15 +++++++-------- 6 files changed, 34 insertions(+), 26 deletions(-)arch/arm/include/asm/kvm_host.h +2
-0@@ -231,6 +231,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu); +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm/kvm/handle_exit.c +5
-9@@ -57,22 +57,18 @@ static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)* @run: the kvm_run structure pointer * * WFE: Yield the CPU and come back to this vcpu when the scheduler -* decides to. -* WFI: Simply call kvm_vcpu_block(), which will halt execution of -* world-switches and schedule other host processes until there is an -* incoming IRQ or FIQ to the VM. +* decides to. +* WFI: Halt execution of world-switches and schedule other host +* processes until there is an incoming IRQ or FIQ to the VM. */ static int kvm_handle_wfx (struct kvm_vcpu *vcpu,struct kvm_run *run) {if (kvm_vcpu_get_hsr (vcpu) &HSR_WFI_IS_WFE ) {trace_kvm_wfx (*vcpu_pc (vcpu), true); - vcpu->stat.wfe_exit_stat++; -kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +kvm_arm_emulate_wfe (vcpu); }else {trace_kvm_wfx (*vcpu_pc (vcpu), false); - vcpu->stat.wfi_exit_stat++; -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); +kvm_arm_emulate_wfi (vcpu); }kvm_skip_instr (vcpu,kvm_vcpu_trap_il_is32bit (vcpu));arch/arm64/include/asm/kvm_host.h +2
-0@@ -332,6 +332,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu); +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)arch/arm64/kvm/handle_exit.c +5
-9@@ -74,22 +74,18 @@ static int handle_no_fpsimd(struct kvm_vcpu *vcpu, struct kvm_run *run)* @vcpu: the vcpu pointer * * WFE: Yield the CPU and come back to this vcpu when the scheduler -* decides to. -* WFI: Simply call kvm_vcpu_block(), which will halt execution of -* world-switches and schedule other host processes until there is an -* incoming IRQ or FIQ to the VM. +* decides to. +* WFI: Halt execution of world-switches and schedule other host +* processes until there is an incoming IRQ or FIQ to the VM. */ static int kvm_handle_wfx (struct kvm_vcpu *vcpu,struct kvm_run *run) {if (kvm_vcpu_get_hsr (vcpu) & ESR_ELx_WFx_ISS_WFE) {trace_kvm_wfx_arm64 (*vcpu_pc (vcpu), true); - vcpu->stat.wfe_exit_stat++; -kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +kvm_arm_emulate_wfe (vcpu); }else {trace_kvm_wfx_arm64 (*vcpu_pc (vcpu), false); - vcpu->stat.wfi_exit_stat++; -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); +kvm_arm_emulate_wfi (vcpu); }kvm_skip_instr (vcpu,kvm_vcpu_trap_il_is32bit (vcpu));virt/kvm/arm/arm.c +13
-0@@ -573,6 +573,19 @@ void kvm_arm_resume_guest(struct kvm *kvm) } } +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu) +{ + vcpu->stat.wfe_exit_stat++; +kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +} + +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu) +{ + vcpu->stat.wfi_exit_stat++; +kvm_vcpu_block (vcpu); +kvm_clear_request (KVM_REQ_UNHALT , vcpu); +} +static void vcpu_req_sleep (struct kvm_vcpu *vcpu) {struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu);virt/kvm/arm/psci.c +1
-8@@ -50,20 +50,19 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)* This means for KVM the wakeup events are interrupts and * this is consistent with intended use of StateID as described * in section 5.4.1 of PSCI v0.2 specification (ARM DEN 0022A). -* -* Further, we also treat power-down request to be same as -* stand-by request as-per section 5.4.2 clause 3 of PSCI v0.2 -* specification (ARM DEN 0022A). This means all suspend states -* for KVM will preserve the register state. */ -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); - +kvm_arm_emulate_wfi (vcpu);return PSCI_RET_SUCCESS ; }static void kvm_psci_vcpu_off (struct kvm_vcpu *vcpu)
virt/kvm/arm/psci.c static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu) · line 68
Quoted 10 lines
virt/kvm/arm/psci.c +6
-0{ +/* +* We treat a power-off request the same as a stand-by request, +* as-per section 5.4.2 clause 3 of PSCI v0.2* specification +* (ARM DEN 0022A). This means all suspend states for KVM will +* preserve the register state. +*/ kvm_arm_vcpu_power_off (vcpu); }
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH 3/5] KVM: arm/arm64: factor out common wfe/wfi emulation code
In reply to Andrew JonesQuoted 31 lines · Andrew Jones
Before we add more common code to the wfi emulation, let's first factor out everything common.
Signed-off-by Andrew Jones <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 ++ arch/arm/kvm/handle_exit.c | 14 +++++--------- arch/arm64/include/asm/kvm_host.h | 2 ++ arch/arm64/kvm/handle_exit.c | 14 +++++--------- virt/kvm/arm/arm.c | 13 +++++++++++++ virt/kvm/arm/psci.c | 15 +++++++-------- 6 files changed, 34 insertions(+), 26 deletions(-)arch/arm/include/asm/kvm_host.h +2
-0@@ -231,6 +231,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu); +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm/kvm/handle_exit.c +0
-0
arch/arm/kvm/handle_exit.c static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 62
Quoted 10 lines
arch/arm/kvm/handle_exit.c +3
-4@@ -57,22 +57,18 @@ static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)* @run: the kvm_run structure pointer * * WFE: Yield the CPU and come back to this vcpu when the scheduler -* decides to. -* WFI: Simply call kvm_vcpu_block(), which will halt execution of -* world-switches and schedule other host processes until there is an -* incoming IRQ or FIQ to the VM. +* decides to. +* WFI: Halt execution of world-switches and schedule other host +* processes until there is an incoming IRQ or FIQ to the VM.
s/VM/VCPU/
Quoted 105 lines
arch/arm/kvm/handle_exit.c +2
-5*/ static int kvm_handle_wfx (struct kvm_vcpu *vcpu,struct kvm_run *run) {if (kvm_vcpu_get_hsr (vcpu) &HSR_WFI_IS_WFE ) {trace_kvm_wfx (*vcpu_pc (vcpu), true); - vcpu->stat.wfe_exit_stat++; -kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +kvm_arm_emulate_wfe (vcpu); }else {trace_kvm_wfx (*vcpu_pc (vcpu), false); - vcpu->stat.wfi_exit_stat++; -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); +kvm_arm_emulate_wfi (vcpu); }kvm_skip_instr (vcpu,kvm_vcpu_trap_il_is32bit (vcpu));arch/arm64/include/asm/kvm_host.h +2
-0@@ -332,6 +332,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu); +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)arch/arm64/kvm/handle_exit.c +5
-9@@ -74,22 +74,18 @@ static int handle_no_fpsimd(struct kvm_vcpu *vcpu, struct kvm_run *run)* @vcpu: the vcpu pointer * * WFE: Yield the CPU and come back to this vcpu when the scheduler -* decides to. -* WFI: Simply call kvm_vcpu_block(), which will halt execution of -* world-switches and schedule other host processes until there is an -* incoming IRQ or FIQ to the VM. +* decides to. +* WFI: Halt execution of world-switches and schedule other host +* processes until there is an incoming IRQ or FIQ to the VM. */ static int kvm_handle_wfx (struct kvm_vcpu *vcpu,struct kvm_run *run) {if (kvm_vcpu_get_hsr (vcpu) & ESR_ELx_WFx_ISS_WFE) {trace_kvm_wfx_arm64 (*vcpu_pc (vcpu), true); - vcpu->stat.wfe_exit_stat++; -kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +kvm_arm_emulate_wfe (vcpu); }else {trace_kvm_wfx_arm64 (*vcpu_pc (vcpu), false); - vcpu->stat.wfi_exit_stat++; -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); +kvm_arm_emulate_wfi (vcpu); }kvm_skip_instr (vcpu,kvm_vcpu_trap_il_is32bit (vcpu));virt/kvm/arm/arm.c +13
-0@@ -573,6 +573,19 @@ void kvm_arm_resume_guest(struct kvm *kvm) } } +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu) +{ + vcpu->stat.wfe_exit_stat++; +kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +} + +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu) +{ + vcpu->stat.wfi_exit_stat++; +kvm_vcpu_block (vcpu); +kvm_clear_request (KVM_REQ_UNHALT , vcpu); +} +static void vcpu_req_sleep (struct kvm_vcpu *vcpu) {struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu);virt/kvm/arm/psci.c +1
-8@@ -50,20 +50,19 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)* This means for KVM the wakeup events are interrupts and * this is consistent with intended use of StateID as described * in section 5.4.1 of PSCI v0.2 specification (ARM DEN 0022A). -* -* Further, we also treat power-down request to be same as -* stand-by request as-per section 5.4.2 clause 3 of PSCI v0.2 -* specification (ARM DEN 0022A). This means all suspend states -* for KVM will preserve the register state. */ -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); - +kvm_arm_emulate_wfi (vcpu);return PSCI_RET_SUCCESS ;
virt/kvm/arm/psci.c static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu) · line 65
Quoted 10 lines
virt/kvm/arm/psci.c +6
-0}static void kvm_psci_vcpu_off (struct kvm_vcpu *vcpu) { +/* +* We treat a power-off request the same as a stand-by request, +* as-per section 5.4.2 clause 3 of PSCI v0.2* specification +* (ARM DEN 0022A). This means all suspend states for KVM will +* preserve the register state. +*/
I'm not actually convinced that this part of the comment was about power-off. I think what it was trying to say was simply that a suspend operation should preverse the register state, and therefore the commentary doesn't belong in this function. I agree that the comment is potentially more confusing (I lost the exact version of the document referred to in the original comment so can't be sure if there was something unclear in the original document or if the comment is just vaguely written), than it helps. Therefore, I think you should either
(a) keep the comment as it is, (b) rewrite it to make sense, or (c) just delete it.
Quoted 6 lines
virt/kvm/arm/psci.c +0
-0kvm_arm_vcpu_power_off (vcpu); }Signature · 1 line
2.13.5
Thanks,
-Christoffer
Re: [PATCH 3/5] KVM: arm/arm64: factor out common wfe/wfi emulation code
Quoted 163 lines · Christoffer Dall
Quoted 31 lines · Andrew Jones
Before we add more common code to the wfi emulation, let's first factor out everything common.
Signed-off-by Andrew Jones <[email protected]>--- arch/arm/include/asm/kvm_host.h | 2 ++ arch/arm/kvm/handle_exit.c | 14 +++++--------- arch/arm64/include/asm/kvm_host.h | 2 ++ arch/arm64/kvm/handle_exit.c | 14 +++++--------- virt/kvm/arm/arm.c | 13 +++++++++++++ virt/kvm/arm/psci.c | 15 +++++++-------- 6 files changed, 34 insertions(+), 26 deletions(-)arch/arm/include/asm/kvm_host.h +2
-0@@ -231,6 +231,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu); +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu);int kvm_arm_copy_coproc_indices (struct kvm_vcpu *vcpu,u64 __user *uindices);unsigned long kvm_arm_num_coproc_regs (struct kvm_vcpu *vcpu);arch/arm/kvm/handle_exit.c +0
-0arch/arm/kvm/handle_exit.c static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 62
Quoted 10 lines
arch/arm/kvm/handle_exit.c +3
-4@@ -57,22 +57,18 @@ static int handle_smc(struct kvm_vcpu *vcpu, struct kvm_run *run)* @run: the kvm_run structure pointer * * WFE: Yield the CPU and come back to this vcpu when the scheduler -* decides to. -* WFI: Simply call kvm_vcpu_block(), which will halt execution of -* world-switches and schedule other host processes until there is an -* incoming IRQ or FIQ to the VM. +* decides to. +* WFI: Halt execution of world-switches and schedule other host +* processes until there is an incoming IRQ or FIQ to the VM. s/VM/VCPU/
Quoted 105 lines
arch/arm/kvm/handle_exit.c +2
-5*/ static int kvm_handle_wfx (struct kvm_vcpu *vcpu,struct kvm_run *run) {if (kvm_vcpu_get_hsr (vcpu) &HSR_WFI_IS_WFE ) {trace_kvm_wfx (*vcpu_pc (vcpu), true); - vcpu->stat.wfe_exit_stat++; -kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +kvm_arm_emulate_wfe (vcpu); }else {trace_kvm_wfx (*vcpu_pc (vcpu), false); - vcpu->stat.wfi_exit_stat++; -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); +kvm_arm_emulate_wfi (vcpu); }kvm_skip_instr (vcpu,kvm_vcpu_trap_il_is32bit (vcpu));arch/arm64/include/asm/kvm_host.h +2
-0@@ -332,6 +332,8 @@ void kvm_arm_vcpu_power_off(struct kvm_vcpu *vcpu);void kvm_arm_vcpu_power_on (struct kvm_vcpu *vcpu);void kvm_arm_halt_guest (struct kvm *kvm);void kvm_arm_resume_guest (struct kvm *kvm); +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu); +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu);u64 __kvm_call_hyp (void *hypfn, ...);#define kvm_call_hyp (f, ...) __kvm_call_hyp(kvm_ksym_ref(f), ##__VA_ARGS__)arch/arm64/kvm/handle_exit.c +5
-9@@ -74,22 +74,18 @@ static int handle_no_fpsimd(struct kvm_vcpu *vcpu, struct kvm_run *run)* @vcpu: the vcpu pointer * * WFE: Yield the CPU and come back to this vcpu when the scheduler -* decides to. -* WFI: Simply call kvm_vcpu_block(), which will halt execution of -* world-switches and schedule other host processes until there is an -* incoming IRQ or FIQ to the VM. +* decides to. +* WFI: Halt execution of world-switches and schedule other host +* processes until there is an incoming IRQ or FIQ to the VM. */ static int kvm_handle_wfx (struct kvm_vcpu *vcpu,struct kvm_run *run) {if (kvm_vcpu_get_hsr (vcpu) & ESR_ELx_WFx_ISS_WFE) {trace_kvm_wfx_arm64 (*vcpu_pc (vcpu), true); - vcpu->stat.wfe_exit_stat++; -kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +kvm_arm_emulate_wfe (vcpu); }else {trace_kvm_wfx_arm64 (*vcpu_pc (vcpu), false); - vcpu->stat.wfi_exit_stat++; -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); +kvm_arm_emulate_wfi (vcpu); }kvm_skip_instr (vcpu,kvm_vcpu_trap_il_is32bit (vcpu));virt/kvm/arm/arm.c +13
-0@@ -573,6 +573,19 @@ void kvm_arm_resume_guest(struct kvm *kvm) } } +void kvm_arm_emulate_wfe (struct kvm_vcpu *vcpu) +{ + vcpu->stat.wfe_exit_stat++; +kvm_vcpu_on_spin (vcpu,vcpu_mode_priv (vcpu)); +} + +void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu) +{ + vcpu->stat.wfi_exit_stat++; +kvm_vcpu_block (vcpu); +kvm_clear_request (KVM_REQ_UNHALT , vcpu); +} +static void vcpu_req_sleep (struct kvm_vcpu *vcpu) {struct swait_queue_head *wq =kvm_arch_vcpu_wq (vcpu);virt/kvm/arm/psci.c +1
-8@@ -50,20 +50,19 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu)* This means for KVM the wakeup events are interrupts and * this is consistent with intended use of StateID as described * in section 5.4.1 of PSCI v0.2 specification (ARM DEN 0022A). -* -* Further, we also treat power-down request to be same as -* stand-by request as-per section 5.4.2 clause 3 of PSCI v0.2 -* specification (ARM DEN 0022A). This means all suspend states -* for KVM will preserve the register state. */ -kvm_vcpu_block (vcpu); -kvm_clear_request (KVM_REQ_UNHALT , vcpu); - +kvm_arm_emulate_wfi (vcpu);return PSCI_RET_SUCCESS ;virt/kvm/arm/psci.c static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu) · line 65
Quoted 10 lines
virt/kvm/arm/psci.c +6
-0}static void kvm_psci_vcpu_off (struct kvm_vcpu *vcpu) { +/* +* We treat a power-off request the same as a stand-by request, +* as-per section 5.4.2 clause 3 of PSCI v0.2* specification +* (ARM DEN 0022A). This means all suspend states for KVM will +* preserve the register state. +*/ I'm not actually convinced that this part of the comment was about power-off. I think what it was trying to say was simply that a suspend
Quoted 8 lines
operation should preverse the register state, and therefore the commentary doesn't belong in this function. I agree that the comment is potentially more confusing (I lost the exact version of the document referred to in the original comment so can't be sure if there was something unclear in the original document or if the comment is just vaguely written), than it helps. Therefore, I think you should either
(a) keep the comment as it is, (b) rewrite it to make sense, or (c) just delete it.
I can't find version A either. I read over C just now to try and think of a way to improve the comment, but I wasn't overly enlightened, so I'll probably just opt to remove it.
Quoted 10 lines
Quoted 7 lines
virt/kvm/arm/psci.c +0
-0kvm_arm_vcpu_power_off (vcpu); }Signature · 1 line
2.13.5Thanks,
-Christoffer
Thanks,
drew
PATCH 4/5 KVM: arm/arm64: improve kvm_arch_vcpu_runnable
In reply to Andrew JonesConceptually, kvm_arch_vcpu_runnable() should be "not waiting, or waiting for interrupts and interrupts are pending",
!waiting-uninterruptable && (!waiting-for-interrupts || interrupts-pending)
but the implementation was only
!waiting-uninterruptable && interrupts-pending
Thanks to the context of the two callers there wasn't an issue, however, to future-proof the function, this patch implements the conceptual condition by applying mp_state to track waiting- uninterruptable vs. waiting-for-interrupts.
--- Documentation/virtual/kvm/api.txt | 10 ++++++---- virt/kvm/arm/arm.c | 13 +++++++++---- 2 files changed, 15 insertions(+), 8 deletions(-)
Documentation/virtual/kvm/api.txt +6 -4
@@ -1071,7 +1071,7 @@ Possible values are: - KVM_MP_STATE_INIT_RECEIVED: the vcpu has received an INIT signal, and is now ready for a SIPI [x86] - KVM_MP_STATE_HALTED: the vcpu has executed a HLT instruction and - is waiting for an interrupt [x86] + is waiting for an interrupt [x86,arm/arm64] - KVM_MP_STATE_SIPI_RECEIVED: the vcpu has just received a SIPI (vector accessible via KVM_GET_VCPU_EVENTS) [x86] - KVM_MP_STATE_STOPPED: the vcpu is stopped [s390,arm/arm64]
@@ -1087,8 +1087,9 @@ these architectures. For arm/arm64: -The only states that are valid are KVM_MP_STATE_STOPPED and -KVM_MP_STATE_RUNNABLE which reflect if the vcpu is paused or not. +The only states that are valid are KVM_MP_STATE_STOPPED, KVM_MP_STATE_HALTED, +and KVM_MP_STATE_RUNNABLE which reflect if the vcpu is powered-off, waiting +for interrupts, or powered-on and not waiting for interrupts. 4.39 KVM_SET_MP_STATE
@@ -1108,7 +1109,8 @@ these architectures. For arm/arm64: The only states that are valid are KVM_MP_STATE_STOPPED and -KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be paused or not. +KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be +powered-off or not. 4.40 KVM_SET_IDENTITY_MAP_ADDR
virt/kvm/arm/arm.c +9 -4
@@ -414,13 +414,16 @@ static bool vcpu_should_sleep(struct kvm_vcpu *vcpu)* kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled * @vcpu: The VCPU pointer * -* If the guest CPU is not waiting for interrupts or an interrupt line is -* asserted, the CPU is by definition runnable. +* If the VCPU is not waiting at all (including sleeping, which is waiting +* uninterruptably), or it's waiting for interrupts but interrupts are +* pending, then it is by definition runnable. */ int kvm_arch_vcpu_runnable (struct kvm_vcpu *vcpu) { -return (!!vcpu->arch.irq_lines ||kvm_vgic_vcpu_pending_irq (vcpu)) - && !vcpu_should_sleep (vcpu); +return !vcpu_should_sleep (vcpu) && + (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || + (!!vcpu->arch.irq_lines || +kvm_vgic_vcpu_pending_irq (vcpu))); }bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)
@@ -582,7 +585,9 @@ void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu)void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu) { vcpu->stat.wfi_exit_stat++; + vcpu->arch.mp_state =KVM_MP_STATE_HALTED ;kvm_vcpu_block (vcpu); + vcpu->arch.mp_state =KVM_MP_STATE_RUNNABLE ;kvm_clear_request (KVM_REQ_UNHALT , vcpu); }
Signature · 1 line
2.13.5
Re: [PATCH 4/5] KVM: arm/arm64: improve kvm_arch_vcpu_runnable
Quoted 83 lines · Andrew Jones
Conceptually,
kvm_arch_vcpu_runnable()should be "not waiting, or waiting for interrupts and interrupts are pending",!waiting-uninterruptable && (!waiting-for-interrupts || interrupts-pending)but the implementation was only
!waiting-uninterruptable && interrupts-pendingThanks to the context of the two callers there wasn't an issue, however, to future-proof the function, this patch implements the conceptual condition by applying
mp_stateto track waiting- uninterruptable vs. waiting-for-interrupts.Signed-off-by Andrew Jones <[email protected]>--- Documentation/virtual/kvm/api.txt | 10 ++++++---- virt/kvm/arm/arm.c | 13 +++++++++---- 2 files changed, 15 insertions(+), 8 deletions(-)Documentation/virtual/kvm/api.txt +6
-4@@ -1071,7 +1071,7 @@ Possible values are: - KVM_MP_STATE_INIT_RECEIVED: the vcpu has received an INIT signal, and is now ready for a SIPI [x86] - KVM_MP_STATE_HALTED: the vcpu has executed a HLT instruction and - is waiting for an interrupt [x86] + is waiting for an interrupt [x86,arm/arm64] - KVM_MP_STATE_SIPI_RECEIVED: the vcpu has just received a SIPI (vector accessible via KVM_GET_VCPU_EVENTS) [x86] - KVM_MP_STATE_STOPPED: the vcpu is stopped [s390,arm/arm64]@@ -1087,8 +1087,9 @@ these architectures. For arm/arm64: -The only states that are valid are KVM_MP_STATE_STOPPED and -KVM_MP_STATE_RUNNABLE which reflect if the vcpu is paused or not. +The only states that are valid are KVM_MP_STATE_STOPPED, KVM_MP_STATE_HALTED, +and KVM_MP_STATE_RUNNABLE which reflect if the vcpu is powered-off, waiting +for interrupts, or powered-on and not waiting for interrupts. 4.39 KVM_SET_MP_STATE@@ -1108,7 +1109,8 @@ these architectures. For arm/arm64: The only states that are valid are KVM_MP_STATE_STOPPED and -KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be paused or not. +KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be +powered-off or not. 4.40 KVM_SET_IDENTITY_MAP_ADDRvirt/kvm/arm/arm.c +7
-4@@ -414,13 +414,16 @@ static bool vcpu_should_sleep(struct kvm_vcpu *vcpu)* kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled * @vcpu: The VCPU pointer * -* If the guest CPU is not waiting for interrupts or an interrupt line is -* asserted, the CPU is by definition runnable. +* If the VCPU is not waiting at all (including sleeping, which is waiting +* uninterruptably), or it's waiting for interrupts but interrupts are +* pending, then it is by definition runnable. */ int kvm_arch_vcpu_runnable (struct kvm_vcpu *vcpu) { -return (!!vcpu->arch.irq_lines ||kvm_vgic_vcpu_pending_irq (vcpu)) - && !vcpu_should_sleep (vcpu); +return !vcpu_should_sleep (vcpu) && + (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || + (!!vcpu->arch.irq_lines || +kvm_vgic_vcpu_pending_irq (vcpu))); }bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)
virt/kvm/arm/arm.c void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu) · line 593
Quoted 9 lines
virt/kvm/arm/arm.c +2
-0@@ -582,7 +585,9 @@ void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu)void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu) { vcpu->stat.wfi_exit_stat++; + vcpu->arch.mp_state =KVM_MP_STATE_HALTED ;kvm_vcpu_block (vcpu); + vcpu->arch.mp_state =KVM_MP_STATE_RUNNABLE ;kvm_clear_request (KVM_REQ_UNHALT , vcpu); }
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH 4/5] KVM: arm/arm64: improve kvm_arch_vcpu_runnable
In reply to Andrew JonesQuoted 35 lines · Andrew Jones
Conceptually,
kvm_arch_vcpu_runnable()should be "not waiting, or waiting for interrupts and interrupts are pending",!waiting-uninterruptable && (!waiting-for-interrupts || interrupts-pending)but the implementation was only
!waiting-uninterruptable && interrupts-pendingThanks to the context of the two callers there wasn't an issue, however, to future-proof the function, this patch implements the conceptual condition by applying
mp_stateto track waiting- uninterruptable vs. waiting-for-interrupts.Signed-off-by Andrew Jones <[email protected]>--- Documentation/virtual/kvm/api.txt | 10 ++++++---- virt/kvm/arm/arm.c | 13 +++++++++---- 2 files changed, 15 insertions(+), 8 deletions(-)Documentation/virtual/kvm/api.txt +1
-1@@ -1071,7 +1071,7 @@ Possible values are: - KVM_MP_STATE_INIT_RECEIVED: the vcpu has received an INIT signal, and is now ready for a SIPI [x86] - KVM_MP_STATE_HALTED: the vcpu has executed a HLT instruction and - is waiting for an interrupt [x86] + is waiting for an interrupt [x86,arm/arm64] - KVM_MP_STATE_SIPI_RECEIVED: the vcpu has just received a SIPI (vector accessible via KVM_GET_VCPU_EVENTS) [x86] - KVM_MP_STATE_STOPPED: the vcpu is stopped [s390,arm/arm64]
Documentation/virtual/kvm/api.txt these architectures. · line 1093
Quoted 9 lines
Documentation/virtual/kvm/api.txt +3
-2@@ -1087,8 +1087,9 @@ these architectures. For arm/arm64: -The only states that are valid are KVM_MP_STATE_STOPPED and -KVM_MP_STATE_RUNNABLE which reflect if the vcpu is paused or not. +The only states that are valid are KVM_MP_STATE_STOPPED, KVM_MP_STATE_HALTED, +and KVM_MP_STATE_RUNNABLE which reflect if the vcpu is powered-off, waiting +for interrupts, or powered-on and not waiting for interrupts.
Is it valid to introduce another value after we've publicly declared that it's not valid? What if userspace has an assert(state == KVM_MP_STATE_STOPPED || state == KVM_MP_STATE_RUNNABLE) ?
At least we should make that is not the case.
Documentation/virtual/kvm/api.txt these architectures. · line 1114
Quoted 10 lines
Documentation/virtual/kvm/api.txt +2
-14.39 KVM_SET_MP_STATE@@ -1108,7 +1109,8 @@ these architectures. For arm/arm64: The only states that are valid are KVM_MP_STATE_STOPPED and -KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be paused or not. +KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be +powered-off or not.
It looks pretty dodgy to me that we can return an MP state which we cannot set again. Feels like this could break migration.
Quoted 14 lines
Documentation/virtual/kvm/api.txt +0
-04.40 KVM_SET_IDENTITY_MAP_ADDRvirt/kvm/arm/arm.c +2
-2@@ -414,13 +414,16 @@ static bool vcpu_should_sleep(struct kvm_vcpu *vcpu)* kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled * @vcpu: The VCPU pointer * -* If the guest CPU is not waiting for interrupts or an interrupt line is -* asserted, the CPU is by definition runnable. +* If the VCPU is not waiting at all (including sleeping, which is waiting +* uninterruptably), or it's waiting for interrupts but interrupts are
virt/kvm/arm/arm.c static bool vcpu_should_sleep(struct kvm_vcpu *vcpu) · line 426
Quoted 10 lines
virt/kvm/arm/arm.c +5
-2+* pending, then it is by definition runnable. */ int kvm_arch_vcpu_runnable (struct kvm_vcpu *vcpu) { -return (!!vcpu->arch.irq_lines ||kvm_vgic_vcpu_pending_irq (vcpu)) - && !vcpu_should_sleep (vcpu); +return !vcpu_should_sleep (vcpu) && + (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || + (!!vcpu->arch.irq_lines || +kvm_vgic_vcpu_pending_irq (vcpu)));
This is hard to read. How about:
bool irq_pending = !!vcpu->arch.irq_lines || kvm_vgic_vcpu_pending_irq(vcpu); if (vcpu_should_sleep(vcpu) return false; else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED && !irq_pending) return false; return true;
Quoted 16 lines
virt/kvm/arm/arm.c +2
-0}bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)@@ -582,7 +585,9 @@ void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu)void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu) { vcpu->stat.wfi_exit_stat++; + vcpu->arch.mp_state =KVM_MP_STATE_HALTED ;kvm_vcpu_block (vcpu); + vcpu->arch.mp_state =KVM_MP_STATE_RUNNABLE ;kvm_clear_request (KVM_REQ_UNHALT , vcpu); }Signature · 1 line
2.13.5
Thanks,
-Christoffer
Re: [PATCH 4/5] KVM: arm/arm64: improve kvm_arch_vcpu_runnable
Quoted 50 lines · Christoffer Dall
Quoted 35 lines · Andrew Jones
Conceptually,
kvm_arch_vcpu_runnable()should be "not waiting, or waiting for interrupts and interrupts are pending",!waiting-uninterruptable && (!waiting-for-interrupts || interrupts-pending)but the implementation was only
!waiting-uninterruptable && interrupts-pendingThanks to the context of the two callers there wasn't an issue, however, to future-proof the function, this patch implements the conceptual condition by applying
mp_stateto track waiting- uninterruptable vs. waiting-for-interrupts.Signed-off-by Andrew Jones <[email protected]>--- Documentation/virtual/kvm/api.txt | 10 ++++++---- virt/kvm/arm/arm.c | 13 +++++++++---- 2 files changed, 15 insertions(+), 8 deletions(-)Documentation/virtual/kvm/api.txt +1
-1@@ -1071,7 +1071,7 @@ Possible values are: - KVM_MP_STATE_INIT_RECEIVED: the vcpu has received an INIT signal, and is now ready for a SIPI [x86] - KVM_MP_STATE_HALTED: the vcpu has executed a HLT instruction and - is waiting for an interrupt [x86] + is waiting for an interrupt [x86,arm/arm64] - KVM_MP_STATE_SIPI_RECEIVED: the vcpu has just received a SIPI (vector accessible via KVM_GET_VCPU_EVENTS) [x86] - KVM_MP_STATE_STOPPED: the vcpu is stopped [s390,arm/arm64]Documentation/virtual/kvm/api.txt these architectures. · line 1093
Quoted 9 lines
Documentation/virtual/kvm/api.txt +3
-2@@ -1087,8 +1087,9 @@ these architectures. For arm/arm64: -The only states that are valid are KVM_MP_STATE_STOPPED and -KVM_MP_STATE_RUNNABLE which reflect if the vcpu is paused or not. +The only states that are valid are KVM_MP_STATE_STOPPED, KVM_MP_STATE_HALTED, +and KVM_MP_STATE_RUNNABLE which reflect if the vcpu is powered-off, waiting +for interrupts, or powered-on and not waiting for interrupts.Is it valid to introduce another value after we've publicly declared that it's not valid? What if userspace has an
assert(state == KVM_MP_STATE_STOPPED || state == KVM_MP_STATE_RUNNABLE)?
Quoted 1 line
At least we should make that is not the case.
The thought had crossed my mind that we might need a new VCPU feature, but it didn't seem necessary after checking QEMU code, and I didn't like the idea of adding one. But... if we consider some theoretical KVM userspace, then maybe we need it?
Quoted 6 lines
Quoted 6 lines
Documentation/virtual/kvm/api.txt +0
-04.39 KVM_SET_MP_STATE@@ -1108,7 +1109,8 @@ these architectures. For arm/arm64:
Quoted 8 lines
Documentation/virtual/kvm/api.txt these architectures. · line 1114
Quoted 5 lines
Documentation/virtual/kvm/api.txt +2
-1The only states that are valid are KVM_MP_STATE_STOPPED and -KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be paused or not. +KVM_MP_STATE_RUNNABLE which reflect if the vcpu should be +powered-off or not.It looks pretty dodgy to me that we can return an MP state which we cannot set again. Feels like this could break migration.
While respinning for Marc's changes, I had some second thoughts about this too. I was going to do some testing with QEMU and maybe change it and QEMU so that it would save/restore whatever the state is, including KVM_MP_STATE_HALTED.
I'll do that checking and experimenting.
Quoted 28 lines
Quoted 15 lines
Documentation/virtual/kvm/api.txt +0
-04.40 KVM_SET_IDENTITY_MAP_ADDRvirt/kvm/arm/arm.c +2
-2@@ -414,13 +414,16 @@ static bool vcpu_should_sleep(struct kvm_vcpu *vcpu)* kvm_arch_vcpu_runnable - determine if the vcpu can be scheduled * @vcpu: The VCPU pointer * -* If the guest CPU is not waiting for interrupts or an interrupt line is -* asserted, the CPU is by definition runnable. +* If the VCPU is not waiting at all (including sleeping, which is waiting +* uninterruptably), or it's waiting for interrupts but interrupts are virt/kvm/arm/arm.c static bool vcpu_should_sleep(struct kvm_vcpu *vcpu) · line 426
Quoted 10 lines
virt/kvm/arm/arm.c +5
-2+* pending, then it is by definition runnable. */ int kvm_arch_vcpu_runnable (struct kvm_vcpu *vcpu) { -return (!!vcpu->arch.irq_lines ||kvm_vgic_vcpu_pending_irq (vcpu)) - && !vcpu_should_sleep (vcpu); +return !vcpu_should_sleep (vcpu) && + (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || + (!!vcpu->arch.irq_lines || +kvm_vgic_vcpu_pending_irq (vcpu)));This is hard to read. How about:
Quoted 8 lines
bool irq_pending = !!vcpu->arch.irq_lines || kvm_vgic_vcpu_pending_irq(vcpu); if (vcpu_should_sleep(vcpu) return false; else if (vcpu->arch.mp_state == KVM_MP_STATE_HALTED && !irq_pending) return false; return true;
Using local variables does improve this, but I think I prefer
bool halted = vcpu->arch.mp_state == KVM_MP_STATE_HALTED;
bool irq_pending = !!vcpu->arch.irq_lines ||
kvm_vgic_vcpu_pending_irq(vcpu);
return !vcpu_should_sleep(vcpu) && (!halted || irq_pending);Quoted 14 lines
Quoted 14 lines
virt/kvm/arm/arm.c +2
-0}bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)@@ -582,7 +585,9 @@ void kvm_arm_emulate_wfe(struct kvm_vcpu *vcpu)void kvm_arm_emulate_wfi (struct kvm_vcpu *vcpu) { vcpu->stat.wfi_exit_stat++; + vcpu->arch.mp_state =KVM_MP_STATE_HALTED ;kvm_vcpu_block (vcpu); + vcpu->arch.mp_state =KVM_MP_STATE_RUNNABLE ;kvm_clear_request (KVM_REQ_UNHALT , vcpu); }
Quoted 6 lines
Quoted 3 lines
Signature · 1 line
2.13.5Thanks,
-Christoffer
Thanks,
drew
PATCH 5/5 KVM: arm/arm64: kvm_arch_vcpu_runnable: don't miss injected irqs
In reply to Andrew JonesWhen the vPMU is in use if a VCPU's perf event overflow handler were to fire after the VCPU started waiting, then the wake up done by the kvm_vcpu_kick() call in the handler would do nothing, as no "pmu overflow" state is checked in kvm_arch_vcpu_runnable(). Fix this by checking the IRQ_PENDING VCPU request in runnable(). Checking the request also sufficiently covers all the cases that kvm_vgic_vcpu_pending_irq() cover, so we can just replace that.
--- virt/kvm/arm/arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
virt/kvm/arm/arm.c +1 -1
@@ -423,7 +423,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)return !vcpu_should_sleep (vcpu) && (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || (!!vcpu->arch.irq_lines || -kvm_vgic_vcpu_pending_irq (vcpu))); +kvm_test_request (KVM_REQ_IRQ_PENDING , vcpu))); }bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)
Signature · 1 line
2.13.5
Re: [PATCH 5/5] KVM: arm/arm64: kvm_arch_vcpu_runnable: don't miss injected irqs
Quoted 19 lines · Andrew Jones
When the vPMU is in use if a VCPU's perf event overflow handler were to fire after the VCPU started waiting, then the wake up done by the
kvm_vcpu_kick()call in the handler would do nothing, as no "pmu overflow" state is checked inkvm_arch_vcpu_runnable(). Fix this by checking theIRQ_PENDINGVCPU request inrunnable(). Checking the request also sufficiently covers all the cases thatkvm_vgic_vcpu_pending_irq()cover, so we can just replace that.Signed-off-by Andrew Jones <[email protected]>--- virt/kvm/arm/arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)virt/kvm/arm/arm.c +0
-0
virt/kvm/arm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu) · line 429
Quoted 8 lines
virt/kvm/arm/arm.c +1
-1@@ -423,7 +423,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)return !vcpu_should_sleep (vcpu) && (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || (!!vcpu->arch.irq_lines || -kvm_vgic_vcpu_pending_irq (vcpu))); +kvm_test_request (KVM_REQ_IRQ_PENDING , vcpu))); }bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)
On a side note, I just had a look at our usage of KVM_REQ_IRQ_PENDING, and we always seem to have a make_request/kick pair (which definitely makes sense). Maybe there is room for a bit of consolidation there too.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH 5/5] KVM: arm/arm64: kvm_arch_vcpu_runnable: don't miss injected irqs
Quoted 26 lines · Marc Zyngier
Quoted 25 lines · Andrew Jones
When the vPMU is in use if a VCPU's perf event overflow handler were to fire after the VCPU started waiting, then the wake up done by the
kvm_vcpu_kick()call in the handler would do nothing, as no "pmu overflow" state is checked inkvm_arch_vcpu_runnable(). Fix this by checking theIRQ_PENDINGVCPU request inrunnable(). Checking the request also sufficiently covers all the cases thatkvm_vgic_vcpu_pending_irq()cover, so we can just replace that.Signed-off-by Andrew Jones <[email protected]>--- virt/kvm/arm/arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)virt/kvm/arm/arm.c +1
-1@@ -423,7 +423,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)return !vcpu_should_sleep (vcpu) && (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || (!!vcpu->arch.irq_lines || -kvm_vgic_vcpu_pending_irq (vcpu))); +kvm_test_request (KVM_REQ_IRQ_PENDING , vcpu))); }
Quoted 8 lines
virt/kvm/arm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu) · line 429
Quoted 2 lines
virt/kvm/arm/arm.c +0
-0bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)Reviewed-by Marc Zyngier <[email protected]>On a side note, I just had a look at our usage of
KVM_REQ_IRQ_PENDING, and we always seem to have a make_request/kick pair (which definitely makes sense). Maybe there is room for a bit of consolidation there too.
I think Radim had a plan to do that with some more VCPU request cleanups.
Thanks,
drew
Quoted 6 lines
Thanks,
M.Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH 5/5] KVM: arm/arm64: kvm_arch_vcpu_runnable: don't miss injected irqs
In reply to Andrew JonesQuoted 18 lines · Andrew Jones
When the vPMU is in use if a VCPU's perf event overflow handler were to fire after the VCPU started waiting, then the wake up done by the
kvm_vcpu_kick()call in the handler would do nothing, as no "pmu overflow" state is checked inkvm_arch_vcpu_runnable(). Fix this by checking theIRQ_PENDINGVCPU request inrunnable(). Checking the request also sufficiently covers all the cases thatkvm_vgic_vcpu_pending_irq()cover, so we can just replace that.Signed-off-by Andrew Jones <[email protected]>--- virt/kvm/arm/arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)virt/kvm/arm/arm.c +0
-0
virt/kvm/arm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu) · line 426
Quoted 5 lines
virt/kvm/arm/arm.c +1
-1@@ -423,7 +423,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)return !vcpu_should_sleep (vcpu) && (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || (!!vcpu->arch.irq_lines || -kvm_vgic_vcpu_pending_irq (vcpu))); +kvm_test_request (KVM_REQ_IRQ_PENDING , vcpu)));
So what if a VCPU blocks, a device raises an IRQ, the VCPU loops around, clears the KVM_REQ_IRQ_PENDING flag, enters the VM again, which does another WFI (for fun), and you end up here again with a pending IRQ but no KVM_REQ_IRQ_PENDING flag anymore. Doesn't this end up incorrectly stalling the VCPU?
I don't think that a transient flag will work for a persistent binary state here.
Quoted 6 lines
virt/kvm/arm/arm.c +0
-0}bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)Signature · 1 line
2.13.5
Thanks,
-Christoffer
Re: [PATCH 5/5] KVM: arm/arm64: kvm_arch_vcpu_runnable: don't miss injected irqs
Quoted 14 lines · Christoffer Dall
Quoted 13 lines · Andrew Jones
When the vPMU is in use if a VCPU's perf event overflow handler were to fire after the VCPU started waiting, then the wake up done by the
kvm_vcpu_kick()call in the handler would do nothing, as no "pmu overflow" state is checked inkvm_arch_vcpu_runnable(). Fix this by checking theIRQ_PENDINGVCPU request inrunnable(). Checking the request also sufficiently covers all the cases thatkvm_vgic_vcpu_pending_irq()cover, so we can just replace that.Signed-off-by Andrew Jones <[email protected]>--- virt/kvm/arm/arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 16 lines
virt/kvm/arm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu) · line 426
Quoted 10 lines
virt/kvm/arm/arm.c +1
-1@@ -423,7 +423,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)return !vcpu_should_sleep (vcpu) && (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || (!!vcpu->arch.irq_lines || -kvm_vgic_vcpu_pending_irq (vcpu))); +kvm_test_request (KVM_REQ_IRQ_PENDING , vcpu)));So what if a VCPU blocks, a device raises an IRQ, the VCPU loops around, clears the
KVM_REQ_IRQ_PENDINGflag, enters the VM again, which does another WFI (for fun), and you end up here again with a pending IRQ but noKVM_REQ_IRQ_PENDINGflag anymore. Doesn't this end up incorrectly stalling the VCPU?
Hmm, I see what you mean. I'm sorry I missed that.
Quoted 3 lines
I don't think that a transient flag will work for a persistent binary state here.
I think we can fix it by adding
if (kvm_vgic_vcpu_pending_irq(vcpu))
kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
to kvm_vgic_sync_hwstate(), if the additional overhead would be acceptable. Otherwise we need to find some other way to ensure vPMU irqs unblock the VCPU.
Quoted 4 lines
Quoted 4 lines
virt/kvm/arm/arm.c +0
-0}
Quoted 8 lines
Quoted 4 lines
virt/kvm/arm/arm.c +0
-0bool kvm_arch_vcpu_in_kernel (struct kvm_vcpu *vcpu)Signature · 1 line
2.13.5Thanks,
-Christoffer
Thanks,
drew
Re: [PATCH 5/5] KVM: arm/arm64: kvm_arch_vcpu_runnable: don't miss injected irqs
Quoted 38 lines · Andrew Jones
Quoted 14 lines · Christoffer Dall
Quoted 13 lines · Andrew Jones
When the vPMU is in use if a VCPU's perf event overflow handler were to fire after the VCPU started waiting, then the wake up done by the
kvm_vcpu_kick()call in the handler would do nothing, as no "pmu overflow" state is checked inkvm_arch_vcpu_runnable(). Fix this by checking theIRQ_PENDINGVCPU request inrunnable(). Checking the request also sufficiently covers all the cases thatkvm_vgic_vcpu_pending_irq()cover, so we can just replace that.Signed-off-by Andrew Jones <[email protected]>--- virt/kvm/arm/arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Quoted 16 lines
virt/kvm/arm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu) · line 426
Quoted 10 lines
virt/kvm/arm/arm.c +1
-1@@ -423,7 +423,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)return !vcpu_should_sleep (vcpu) && (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || (!!vcpu->arch.irq_lines || -kvm_vgic_vcpu_pending_irq (vcpu))); +kvm_test_request (KVM_REQ_IRQ_PENDING , vcpu)));So what if a VCPU blocks, a device raises an IRQ, the VCPU loops around, clears the
KVM_REQ_IRQ_PENDINGflag, enters the VM again, which does another WFI (for fun), and you end up here again with a pending IRQ but noKVM_REQ_IRQ_PENDINGflag anymore. Doesn't this end up incorrectly stalling the VCPU?Hmm, I see what you mean. I'm sorry I missed that.
Quoted 3 lines
I don't think that a transient flag will work for a persistent binary state here.
Quoted 9 lines
I think we can fix it by adding
if (
kvm_vgic_vcpu_pending_irq(vcpu))kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);to
kvm_vgic_sync_hwstate(), if the additional overhead would be acceptable. Otherwise we need to find some other way to ensure vPMU irqs unblock the VCPU.
I think you just need to check
kvm_vgic_vcpu_pending_irq() ||
kvm_test_request(KVM_REQ_IRQ_PENDING, vcpu)
Wouldn't that fix it?
Thanks,
-Christoffer
Re: [PATCH 5/5] KVM: arm/arm64: kvm_arch_vcpu_runnable: don't miss injected irqs
Quoted 49 lines · Christoffer Dall
Quoted 38 lines · Andrew Jones
Quoted 14 lines · Christoffer Dall
Quoted 13 lines · Andrew Jones
When the vPMU is in use if a VCPU's perf event overflow handler were to fire after the VCPU started waiting, then the wake up done by the
kvm_vcpu_kick()call in the handler would do nothing, as no "pmu overflow" state is checked inkvm_arch_vcpu_runnable(). Fix this by checking theIRQ_PENDINGVCPU request inrunnable(). Checking the request also sufficiently covers all the cases thatkvm_vgic_vcpu_pending_irq()cover, so we can just replace that.Signed-off-by Andrew Jones <[email protected]>--- virt/kvm/arm/arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Quoted 16 lines
virt/kvm/arm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu) · line 426
Quoted 10 lines
virt/kvm/arm/arm.c +1
-1@@ -423,7 +423,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)return !vcpu_should_sleep (vcpu) && (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || (!!vcpu->arch.irq_lines || -kvm_vgic_vcpu_pending_irq (vcpu))); +kvm_test_request (KVM_REQ_IRQ_PENDING , vcpu)));So what if a VCPU blocks, a device raises an IRQ, the VCPU loops around, clears the
KVM_REQ_IRQ_PENDINGflag, enters the VM again, which does another WFI (for fun), and you end up here again with a pending IRQ but noKVM_REQ_IRQ_PENDINGflag anymore. Doesn't this end up incorrectly stalling the VCPU?Hmm, I see what you mean. I'm sorry I missed that.
Quoted 3 lines
I don't think that a transient flag will work for a persistent binary state here.
Quoted 9 lines
I think we can fix it by adding
if (
kvm_vgic_vcpu_pending_irq(vcpu))kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);to
kvm_vgic_sync_hwstate(), if the additional overhead would be acceptable. Otherwise we need to find some other way to ensure vPMU irqs unblock the VCPU.
Quoted 5 lines
I think you just need to check
kvm_vgic_vcpu_pending_irq()||
kvm_test_request(KVM_REQ_IRQ_PENDING, vcpu)Wouldn't that fix it?
It would. I was just hoping to eliminate the lock-taking kvm_vgic_vcpu_pending_irq() call from kvm_arch_vcpu_runnable() though, as kvm_arch_vcpu_runnable() may be called repeatedly during halt polling.
Thanks,
drew
Re: [PATCH 5/5] KVM: arm/arm64: kvm_arch_vcpu_runnable: don't miss injected irqs
Quoted 54 lines · Andrew Jones
Quoted 53 lines · Christoffer Dall
Quoted 38 lines · Andrew Jones
Quoted 14 lines · Christoffer Dall
Quoted 13 lines · Andrew Jones
When the vPMU is in use if a VCPU's perf event overflow handler were to fire after the VCPU started waiting, then the wake up done by the
kvm_vcpu_kick()call in the handler would do nothing, as no "pmu overflow" state is checked inkvm_arch_vcpu_runnable(). Fix this by checking theIRQ_PENDINGVCPU request inrunnable(). Checking the request also sufficiently covers all the cases thatkvm_vgic_vcpu_pending_irq()cover, so we can just replace that.Signed-off-by Andrew Jones <[email protected]>--- virt/kvm/arm/arm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Quoted 16 lines
virt/kvm/arm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu) · line 426
Quoted 10 lines
virt/kvm/arm/arm.c +1
-1@@ -423,7 +423,7 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)return !vcpu_should_sleep (vcpu) && (vcpu->arch.mp_state !=KVM_MP_STATE_HALTED || (!!vcpu->arch.irq_lines || -kvm_vgic_vcpu_pending_irq (vcpu))); +kvm_test_request (KVM_REQ_IRQ_PENDING , vcpu)));So what if a VCPU blocks, a device raises an IRQ, the VCPU loops around, clears the
KVM_REQ_IRQ_PENDINGflag, enters the VM again, which does another WFI (for fun), and you end up here again with a pending IRQ but noKVM_REQ_IRQ_PENDINGflag anymore. Doesn't this end up incorrectly stalling the VCPU?Hmm, I see what you mean. I'm sorry I missed that.
Quoted 3 lines
I don't think that a transient flag will work for a persistent binary state here.
Quoted 9 lines
I think we can fix it by adding
if (
kvm_vgic_vcpu_pending_irq(vcpu))kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);to
kvm_vgic_sync_hwstate(), if the additional overhead would be acceptable. Otherwise we need to find some other way to ensure vPMU irqs unblock the VCPU.I think you just need to check
kvm_vgic_vcpu_pending_irq()||
kvm_test_request(KVM_REQ_IRQ_PENDING, vcpu)
Quoted 7 lines
Quoted 1 line
Wouldn't that fix it?
It would. I was just hoping to eliminate the lock-taking
kvm_vgic_vcpu_pending_irq()call fromkvm_arch_vcpu_runnable()though, askvm_arch_vcpu_runnable()may be called repeatedly during halt polling.
Could we make it so that the KVM_REQ_IRQ_PENDING request only gets cleared when there are really no more pending IRQs for the VCPU then?
Getting rid of the lock would be potentially good from a latency point of view, but then again, it's slow to wake up from WFI, and the lock is per-VCPU so not likely to be contended is it?
Thanks,
-Christoffer
[PATCH 6/5] KVM: arm/arm64: make kvm_vgic_vcpu_pending_irq static
In reply to Andrew JonesNow that the only caller of kvm_vgic_vcpu_pending_irq() is in the vgic code, let's make it private in order to discourage it's use outside, as checking the IRQ_PENDING VCPU request is likely the right thing for non-vgic code to do. Also, remove a vgic prototype that was mistakenly introduced with 0919e84c0fc1, and along for the ride ever since.
--- include/kvm/arm_vgic.h | 3 --- virt/kvm/arm/vgic/vgic.c | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-)
include/kvm/arm_vgic.h +0 -3
@@ -311,8 +311,6 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);int kvm_vgic_unmap_phys_irq (struct kvm_vcpu *vcpu,unsigned int virt_irq);bool kvm_vgic_map_is_active (struct kvm_vcpu *vcpu,unsigned int virt_irq); -int kvm_vgic_vcpu_pending_irq (struct kvm_vcpu *vcpu); -void kvm_vgic_load (struct kvm_vcpu *vcpu);void kvm_vgic_put (struct kvm_vcpu *vcpu);
@@ -322,7 +320,6 @@ void kvm_vgic_put(struct kvm_vcpu *vcpu);#define vgic_valid_spi (k, i) (((i) >= VGIC_NR_PRIVATE_IRQS) && \ ((i) < (k)->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS)) -bool kvm_vcpu_has_pending_irqs (struct kvm_vcpu *vcpu);void kvm_vgic_sync_hwstate (struct kvm_vcpu *vcpu);void kvm_vgic_flush_hwstate (struct kvm_vcpu *vcpu);
virt/kvm/arm/vgic/vgic.c +1 -1
@@ -730,7 +730,7 @@ void kvm_vgic_put(struct kvm_vcpu *vcpu)vgic_v3_put (vcpu); } -int kvm_vgic_vcpu_pending_irq (struct kvm_vcpu *vcpu) +static int kvm_vgic_vcpu_pending_irq (struct kvm_vcpu *vcpu) {struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;struct vgic_irq *irq;
Signature · 1 line
2.13.5
Marc Zyngier
Acked-byRe: [PATCH 6/5] KVM: arm/arm64: make kvm_vgic_vcpu_pending_irq static
Quoted 40 lines · Andrew Jones
Now that the only caller of
kvm_vgic_vcpu_pending_irq()is in the vgic code, let's make it private in order to discourage it's use outside, as checking theIRQ_PENDINGVCPU request is likely the right thing for non-vgic code to do. Also, remove a vgic prototype that was mistakenly introduced with0919e84c0fc1, and along for the ride ever since.Signed-off-by Andrew Jones <[email protected]>--- include/kvm/arm_vgic.h | 3 --- virt/kvm/arm/vgic/vgic.c | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-)include/kvm/arm_vgic.h +0
-3@@ -311,8 +311,6 @@ int kvm_vgic_map_phys_irq(struct kvm_vcpu *vcpu, u32 virt_irq, u32 phys_irq);int kvm_vgic_unmap_phys_irq (struct kvm_vcpu *vcpu,unsigned int virt_irq);bool kvm_vgic_map_is_active (struct kvm_vcpu *vcpu,unsigned int virt_irq); -int kvm_vgic_vcpu_pending_irq (struct kvm_vcpu *vcpu); -void kvm_vgic_load (struct kvm_vcpu *vcpu);void kvm_vgic_put (struct kvm_vcpu *vcpu);@@ -322,7 +320,6 @@ void kvm_vgic_put(struct kvm_vcpu *vcpu);#define vgic_valid_spi (k, i) (((i) >= VGIC_NR_PRIVATE_IRQS) && \ ((i) < (k)->arch.vgic.nr_spis + VGIC_NR_PRIVATE_IRQS)) -bool kvm_vcpu_has_pending_irqs (struct kvm_vcpu *vcpu);void kvm_vgic_sync_hwstate (struct kvm_vcpu *vcpu);void kvm_vgic_flush_hwstate (struct kvm_vcpu *vcpu);virt/kvm/arm/vgic/vgic.c +0
-0
virt/kvm/arm/vgic/vgic.c void kvm_vgic_put(struct kvm_vcpu *vcpu) · line 736
Quoted 8 lines
virt/kvm/arm/vgic/vgic.c +1
-1@@ -730,7 +730,7 @@ void kvm_vgic_put(struct kvm_vcpu *vcpu)vgic_v3_put (vcpu); } -int kvm_vgic_vcpu_pending_irq (struct kvm_vcpu *vcpu) +static int kvm_vgic_vcpu_pending_irq (struct kvm_vcpu *vcpu) {struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;struct vgic_irq *irq;
M.
Signature · 1 line
Jazz is not dead. It just smells funny...