From: Eric Auger <eric.auger@linaro.org> To: Christoffer Dall <christoffer.dall@linaro.org> Cc: eric.auger@st.com, patches@linaro.org, marc.zyngier@arm.com, alex.williamson@redhat.com, pbonzini@redhat.com, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest Date: Fri, 07 Aug 2015 18:17:05 +0200 [thread overview] Message-ID: <55C4DA01.7040603@linaro.org> (raw) In-Reply-To: <20150718091851.GO14024@cbox> Hi Chistoffer, On 07/18/2015 11:18 AM, Christoffer Dall wrote: > On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote: >> On halt, the guest is forced to exit and prevented from being >> re-entered. This is synchronous. >> >> Those two operations will be needed for IRQ forwarding setting. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> RFC v1 -> v2: >> - add __maybe_unused >> >> RFC: >> - rename the function and this latter becomes static >> - remove __KVM_HAVE_ARCH_HALT_GUEST >> >> v4 -> v5: add arm64 support >> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64 >> - add pause field >> --- >> arch/arm/include/asm/kvm_host.h | 3 +++ >> arch/arm/kvm/arm.c | 35 ++++++++++++++++++++++++++++++++--- >> arch/arm64/include/asm/kvm_host.h | 3 +++ >> 3 files changed, 38 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index 304004d..899ae27 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch { >> /* vcpu power-off state */ >> bool power_off; >> >> + /* Don't run the guest */ >> + bool pause; >> + >> /* IO related fields */ >> struct kvm_decode mmio_decode; >> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index 7537e68..46d4ef6 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm) >> return vgic_initialized(kvm); >> } >> >> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused; >> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused; >> + >> +static void kvm_arm_halt_guest(struct kvm *kvm) >> +{ >> + int i; >> + struct kvm_vcpu *vcpu; >> + >> + kvm_for_each_vcpu(i, vcpu, kvm) >> + vcpu->arch.pause = true; >> + force_vm_exit(cpu_all_mask); >> +} >> + >> +static void kvm_arm_resume_guest(struct kvm *kvm) >> +{ >> + int i; >> + struct kvm_vcpu *vcpu; >> + >> + kvm_for_each_vcpu(i, vcpu, kvm) { >> + wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu); >> + >> + vcpu->arch.pause = false; >> + wake_up_interruptible(wq); >> + } >> +} >> + >> + >> static void vcpu_pause(struct kvm_vcpu *vcpu) >> { >> wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu); >> >> - wait_event_interruptible(*wq, !vcpu->arch.power_off); >> + wait_event_interruptible(*wq, ((!vcpu->arch.power_off) && >> + (!vcpu->arch.pause))); >> } >> >> static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu) >> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> >> update_vttbr(vcpu->kvm); >> >> - if (vcpu->arch.power_off) >> + if (vcpu->arch.power_off || vcpu->arch.pause) >> vcpu_pause(vcpu); > > These two changes really make me feel like kvm_arch_vcpu_runnable() > should be changed to check for both flags and all thes eplaces should > check for runnable instead. > > To make the runnable function more robust, perhaps it should check: > > 1. The vcpu is neither paused nor powered off > 2. The vcpu is not in a WFI state (vcpu->arch.wfi_request) OR there are > pending interrupts (the current check). I did not understand what you meant by this vcpu->arch.wfi_request, which I could not find. In case we call runnable from kvm_vcpu_block/kvm_vcpu_check_block we sometimes are in WFI (kvm_handle_wfx)? Thanks Eric > > Then the logic would fit the name of the function, instead of being a > specific check only called through handle_wfx. > >> >> /* >> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> run->exit_reason = KVM_EXIT_INTR; >> } >> >> - if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) { >> + if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) || >> + vcpu->arch.pause) { > > so why do we need to re-check the pause flag, but not the power_off > flag? That is non-trivial for sure, so if it's correct, deserves a > comment. Also see my comment on the last patch. > >> local_irq_enable(); >> preempt_enable(); >> kvm_vgic_sync_hwstate(vcpu); >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 009da6b..69e3785 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch { >> /* vcpu power-off state */ >> bool power_off; >> >> + /* Don't run the guest */ > > Probably need to be more clear about this being an implementation > requirement rather than being guest visible or related to any > architectural concept. > >> + bool pause; >> + >> /* IO related fields */ >> struct kvm_decode mmio_decode; >> >> -- >> 1.9.1 >> > > Thanks, > -Christoffer >
WARNING: multiple messages have this Message-ID (diff)
From: eric.auger@linaro.org (Eric Auger) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest Date: Fri, 07 Aug 2015 18:17:05 +0200 [thread overview] Message-ID: <55C4DA01.7040603@linaro.org> (raw) In-Reply-To: <20150718091851.GO14024@cbox> Hi Chistoffer, On 07/18/2015 11:18 AM, Christoffer Dall wrote: > On Mon, Jul 06, 2015 at 02:49:56PM +0200, Eric Auger wrote: >> On halt, the guest is forced to exit and prevented from being >> re-entered. This is synchronous. >> >> Those two operations will be needed for IRQ forwarding setting. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> --- >> RFC v1 -> v2: >> - add __maybe_unused >> >> RFC: >> - rename the function and this latter becomes static >> - remove __KVM_HAVE_ARCH_HALT_GUEST >> >> v4 -> v5: add arm64 support >> - also defines __KVM_HAVE_ARCH_HALT_GUEST for arm64 >> - add pause field >> --- >> arch/arm/include/asm/kvm_host.h | 3 +++ >> arch/arm/kvm/arm.c | 35 ++++++++++++++++++++++++++++++++--- >> arch/arm64/include/asm/kvm_host.h | 3 +++ >> 3 files changed, 38 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index 304004d..899ae27 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -132,6 +132,9 @@ struct kvm_vcpu_arch { >> /* vcpu power-off state */ >> bool power_off; >> >> + /* Don't run the guest */ >> + bool pause; >> + >> /* IO related fields */ >> struct kvm_decode mmio_decode; >> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index 7537e68..46d4ef6 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -471,11 +471,39 @@ bool kvm_arch_intc_initialized(struct kvm *kvm) >> return vgic_initialized(kvm); >> } >> >> +static void kvm_arm_halt_guest(struct kvm *kvm) __maybe_unused; >> +static void kvm_arm_resume_guest(struct kvm *kvm) __maybe_unused; >> + >> +static void kvm_arm_halt_guest(struct kvm *kvm) >> +{ >> + int i; >> + struct kvm_vcpu *vcpu; >> + >> + kvm_for_each_vcpu(i, vcpu, kvm) >> + vcpu->arch.pause = true; >> + force_vm_exit(cpu_all_mask); >> +} >> + >> +static void kvm_arm_resume_guest(struct kvm *kvm) >> +{ >> + int i; >> + struct kvm_vcpu *vcpu; >> + >> + kvm_for_each_vcpu(i, vcpu, kvm) { >> + wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu); >> + >> + vcpu->arch.pause = false; >> + wake_up_interruptible(wq); >> + } >> +} >> + >> + >> static void vcpu_pause(struct kvm_vcpu *vcpu) >> { >> wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu); >> >> - wait_event_interruptible(*wq, !vcpu->arch.power_off); >> + wait_event_interruptible(*wq, ((!vcpu->arch.power_off) && >> + (!vcpu->arch.pause))); >> } >> >> static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu) >> @@ -525,7 +553,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> >> update_vttbr(vcpu->kvm); >> >> - if (vcpu->arch.power_off) >> + if (vcpu->arch.power_off || vcpu->arch.pause) >> vcpu_pause(vcpu); > > These two changes really make me feel like kvm_arch_vcpu_runnable() > should be changed to check for both flags and all thes eplaces should > check for runnable instead. > > To make the runnable function more robust, perhaps it should check: > > 1. The vcpu is neither paused nor powered off > 2. The vcpu is not in a WFI state (vcpu->arch.wfi_request) OR there are > pending interrupts (the current check). I did not understand what you meant by this vcpu->arch.wfi_request, which I could not find. In case we call runnable from kvm_vcpu_block/kvm_vcpu_check_block we sometimes are in WFI (kvm_handle_wfx)? Thanks Eric > > Then the logic would fit the name of the function, instead of being a > specific check only called through handle_wfx. > >> >> /* >> @@ -551,7 +579,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> run->exit_reason = KVM_EXIT_INTR; >> } >> >> - if (ret <= 0 || need_new_vmid_gen(vcpu->kvm)) { >> + if (ret <= 0 || need_new_vmid_gen(vcpu->kvm) || >> + vcpu->arch.pause) { > > so why do we need to re-check the pause flag, but not the power_off > flag? That is non-trivial for sure, so if it's correct, deserves a > comment. Also see my comment on the last patch. > >> local_irq_enable(); >> preempt_enable(); >> kvm_vgic_sync_hwstate(vcpu); >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 009da6b..69e3785 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -125,6 +125,9 @@ struct kvm_vcpu_arch { >> /* vcpu power-off state */ >> bool power_off; >> >> + /* Don't run the guest */ > > Probably need to be more clear about this being an implementation > requirement rather than being guest visible or related to any > architectural concept. > >> + bool pause; >> + >> /* IO related fields */ >> struct kvm_decode mmio_decode; >> >> -- >> 1.9.1 >> > > Thanks, > -Christoffer >
next prev parent reply other threads:[~2015-08-07 16:04 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-07-06 12:49 [PATCH 0/2] KVM: arm/arm64: Guest synchronous halt/resume Eric Auger 2015-07-06 12:49 ` Eric Auger 2015-07-06 12:49 ` [PATCH 1/2] KVM: arm: rename pause into power_off Eric Auger 2015-07-06 12:49 ` Eric Auger 2015-07-07 13:36 ` Andrew Jones 2015-07-07 13:36 ` Andrew Jones 2015-07-07 14:08 ` Eric Auger 2015-07-07 14:08 ` Eric Auger 2015-07-18 9:09 ` Christoffer Dall 2015-07-18 9:09 ` Christoffer Dall 2015-08-07 12:36 ` Eric Auger 2015-08-07 12:36 ` Eric Auger 2015-07-06 12:49 ` [PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt,resume]_guest Eric Auger 2015-07-06 12:49 ` Eric Auger 2015-07-07 13:41 ` [PATCH 2/2] kvm: arm/arm64: implement kvm_arm_[halt, resume]_guest Andrew Jones 2015-07-07 13:41 ` Andrew Jones 2015-07-07 14:10 ` Eric Auger 2015-07-07 14:10 ` Eric Auger 2015-07-18 9:18 ` Christoffer Dall 2015-07-18 9:18 ` Christoffer Dall 2015-08-07 12:56 ` Eric Auger 2015-08-07 12:56 ` Eric Auger 2015-08-07 16:17 ` Eric Auger [this message] 2015-08-07 16:17 ` Eric Auger
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=55C4DA01.7040603@linaro.org \ --to=eric.auger@linaro.org \ --cc=alex.williamson@redhat.com \ --cc=christoffer.dall@linaro.org \ --cc=eric.auger@st.com \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=marc.zyngier@arm.com \ --cc=patches@linaro.org \ --cc=pbonzini@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.