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 1/2] KVM: arm: rename pause into power_off Date: Fri, 07 Aug 2015 14:36:40 +0200 [thread overview] Message-ID: <55C4A658.3060302@linaro.org> (raw) In-Reply-To: <20150718090913.GN14024@cbox> Hi Christoffer, On 07/18/2015 11:09 AM, Christoffer Dall wrote: > On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote: >> The kvm_vcpu_arch pause field is renamed into power_off to prepare >> for the introduction of a new pause field. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> v4 -> v5: >> - fix compilation issue on arm64 (add power_off field in kvm_host.h) >> --- >> arch/arm/include/asm/kvm_host.h | 4 ++-- >> arch/arm/kvm/arm.c | 10 +++++----- >> arch/arm/kvm/psci.c | 10 +++++----- >> arch/arm64/include/asm/kvm_host.h | 4 ++-- >> 4 files changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index e896d2c..304004d 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch { >> * here. >> */ >> >> - /* Don't run the guest on this vcpu */ >> - bool pause; >> + /* vcpu power-off state */ >> + bool power_off; >> >> /* IO related fields */ >> struct kvm_decode mmio_decode; >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index bcdf799..7537e68 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu) >> { >> wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu); >> >> - wait_event_interruptible(*wq, !vcpu->arch.pause); >> + wait_event_interruptible(*wq, !vcpu->arch.power_off); > > would there be any benefit to simply calling kvm_vcpu_block() instead of > vcpu_pause, and rewrite kvm_arch_vcpu_runnable to: Wouldn't it somehow change the known behavior or kvm_vcpu_block which is expected/used to exit on IRQ/FIQ (WFI). Here it would exit when power_off changes to false (or maybe you meant pause below in the new context?). > > int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) > { > ▸ return !vcpu->arch.power_off && > (!!v->arch.irq_lines || kvm_vgic_vcpu_pending_irq(v)); > } > > Not sure really, certainly the runnable function does not become more > readable. To me the usage of kvm_vcpu_block looks more complex than this code and I would prefer keeping that version if you don't mind. > >> } >> >> static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu) >> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> >> update_vttbr(vcpu->kvm); >> >> - if (vcpu->arch.pause) >> + if (vcpu->arch.power_off) >> vcpu_pause(vcpu); > > looking back over this code, how does this actually guarantee that we > don't run a powered-off cpu? > > vcpu_pause() just does a wait_event_interruptible(), so if we get > scheduled again, we'll just proceed running. actually it also checks the !vcpu->arch.power_off condition, right? Is there any case where we > could get scheduled without signal_pending() being true and therefore > inadvertedly run the vcpu? kvm_arm_halt_guest can happen at any time, including after the execution of above vcpu_pause(vcpu) call. This is the reason why I added the second check below, once we entered the critical section and just before running the vcpu. With regard to renamed power_off boolean my understanding is: power_off is set - on kvm_arch_vcpu_ioctl_vcpu_init/KVM_ARM_VCPU_POWER_OFF - on PSCI calls from guests through traps from HVC instructions in that case I don't think this can happen - on KVM_SET_MP_STATE ioctl: I think in that case, what you describe can happen. Do you share the same understanding? Best Regards Eric > > if so, we should change the line below like this: > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index bc738d2..98f31e6 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -542,7 +542,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.power_off) { > local_irq_enable(); > preempt_enable(); > kvm_timer_sync_hwstate(vcpu); > > > Sorry for polluting your patch with these questions, I'm otherwise fine > with the rename. > > Thanks, > -Christoffer > >> >> /* >> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu, >> vcpu_reset_hcr(vcpu); >> >> /* >> - * Handle the "start in power-off" case by marking the VCPU as paused. >> + * Handle the "start in power-off" case. >> */ >> if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features)) >> - vcpu->arch.pause = true; >> + vcpu->arch.power_off = true; >> else >> - vcpu->arch.pause = false; >> + vcpu->arch.power_off = false; >> >> return 0; >> } >> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c >> index 4b94b51..134971a 100644 >> --- a/arch/arm/kvm/psci.c >> +++ b/arch/arm/kvm/psci.c >> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu) >> >> static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu) >> { >> - vcpu->arch.pause = true; >> + vcpu->arch.power_off = true; >> } >> >> static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) >> @@ -87,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.pause) { >> + if (!vcpu->arch.power_off) { >> if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1) >> return PSCI_RET_ALREADY_ON; >> else >> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) >> * the general puspose registers are undefined upon CPU_ON. >> */ >> *vcpu_reg(vcpu, 0) = context_id; >> - vcpu->arch.pause = false; >> + vcpu->arch.power_off = false; >> smp_mb(); /* Make sure the above is visible */ >> >> wq = kvm_arch_vcpu_wq(vcpu); >> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu) >> kvm_for_each_vcpu(i, tmp, kvm) { >> mpidr = kvm_vcpu_get_mpidr_aff(tmp); >> if (((mpidr & target_affinity_mask) == target_affinity) && >> - !tmp->arch.pause) { >> + !tmp->arch.power_off) { >> return PSCI_0_2_AFFINITY_LEVEL_ON; >> } >> } >> @@ -175,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.pause = true; >> + tmp->arch.power_off = true; >> kvm_vcpu_kick(tmp); >> } >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 2709db2..009da6b 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch { >> * here. >> */ >> >> - /* Don't run the guest */ >> - bool pause; >> + /* vcpu power-off state */ >> + bool power_off; >> >> /* IO related fields */ >> struct kvm_decode mmio_decode; >> -- >> 1.9.1 >> _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
WARNING: multiple messages have this Message-ID (diff)
From: eric.auger@linaro.org (Eric Auger) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/2] KVM: arm: rename pause into power_off Date: Fri, 07 Aug 2015 14:36:40 +0200 [thread overview] Message-ID: <55C4A658.3060302@linaro.org> (raw) In-Reply-To: <20150718090913.GN14024@cbox> Hi Christoffer, On 07/18/2015 11:09 AM, Christoffer Dall wrote: > On Mon, Jul 06, 2015 at 02:49:55PM +0200, Eric Auger wrote: >> The kvm_vcpu_arch pause field is renamed into power_off to prepare >> for the introduction of a new pause field. >> >> Signed-off-by: Eric Auger <eric.auger@linaro.org> >> >> v4 -> v5: >> - fix compilation issue on arm64 (add power_off field in kvm_host.h) >> --- >> arch/arm/include/asm/kvm_host.h | 4 ++-- >> arch/arm/kvm/arm.c | 10 +++++----- >> arch/arm/kvm/psci.c | 10 +++++----- >> arch/arm64/include/asm/kvm_host.h | 4 ++-- >> 4 files changed, 14 insertions(+), 14 deletions(-) >> >> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h >> index e896d2c..304004d 100644 >> --- a/arch/arm/include/asm/kvm_host.h >> +++ b/arch/arm/include/asm/kvm_host.h >> @@ -129,8 +129,8 @@ struct kvm_vcpu_arch { >> * here. >> */ >> >> - /* Don't run the guest on this vcpu */ >> - bool pause; >> + /* vcpu power-off state */ >> + bool power_off; >> >> /* IO related fields */ >> struct kvm_decode mmio_decode; >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index bcdf799..7537e68 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -475,7 +475,7 @@ static void vcpu_pause(struct kvm_vcpu *vcpu) >> { >> wait_queue_head_t *wq = kvm_arch_vcpu_wq(vcpu); >> >> - wait_event_interruptible(*wq, !vcpu->arch.pause); >> + wait_event_interruptible(*wq, !vcpu->arch.power_off); > > would there be any benefit to simply calling kvm_vcpu_block() instead of > vcpu_pause, and rewrite kvm_arch_vcpu_runnable to: Wouldn't it somehow change the known behavior or kvm_vcpu_block which is expected/used to exit on IRQ/FIQ (WFI). Here it would exit when power_off changes to false (or maybe you meant pause below in the new context?). > > int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) > { > ? return !vcpu->arch.power_off && > (!!v->arch.irq_lines || kvm_vgic_vcpu_pending_irq(v)); > } > > Not sure really, certainly the runnable function does not become more > readable. To me the usage of kvm_vcpu_block looks more complex than this code and I would prefer keeping that version if you don't mind. > >> } >> >> static int kvm_vcpu_initialized(struct kvm_vcpu *vcpu) >> @@ -525,7 +525,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) >> >> update_vttbr(vcpu->kvm); >> >> - if (vcpu->arch.pause) >> + if (vcpu->arch.power_off) >> vcpu_pause(vcpu); > > looking back over this code, how does this actually guarantee that we > don't run a powered-off cpu? > > vcpu_pause() just does a wait_event_interruptible(), so if we get > scheduled again, we'll just proceed running. actually it also checks the !vcpu->arch.power_off condition, right? Is there any case where we > could get scheduled without signal_pending() being true and therefore > inadvertedly run the vcpu? kvm_arm_halt_guest can happen at any time, including after the execution of above vcpu_pause(vcpu) call. This is the reason why I added the second check below, once we entered the critical section and just before running the vcpu. With regard to renamed power_off boolean my understanding is: power_off is set - on kvm_arch_vcpu_ioctl_vcpu_init/KVM_ARM_VCPU_POWER_OFF - on PSCI calls from guests through traps from HVC instructions in that case I don't think this can happen - on KVM_SET_MP_STATE ioctl: I think in that case, what you describe can happen. Do you share the same understanding? Best Regards Eric > > if so, we should change the line below like this: > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index bc738d2..98f31e6 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -542,7 +542,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.power_off) { > local_irq_enable(); > preempt_enable(); > kvm_timer_sync_hwstate(vcpu); > > > Sorry for polluting your patch with these questions, I'm otherwise fine > with the rename. > > Thanks, > -Christoffer > >> >> /* >> @@ -766,12 +766,12 @@ static int kvm_arch_vcpu_ioctl_vcpu_init(struct kvm_vcpu *vcpu, >> vcpu_reset_hcr(vcpu); >> >> /* >> - * Handle the "start in power-off" case by marking the VCPU as paused. >> + * Handle the "start in power-off" case. >> */ >> if (test_bit(KVM_ARM_VCPU_POWER_OFF, vcpu->arch.features)) >> - vcpu->arch.pause = true; >> + vcpu->arch.power_off = true; >> else >> - vcpu->arch.pause = false; >> + vcpu->arch.power_off = false; >> >> return 0; >> } >> diff --git a/arch/arm/kvm/psci.c b/arch/arm/kvm/psci.c >> index 4b94b51..134971a 100644 >> --- a/arch/arm/kvm/psci.c >> +++ b/arch/arm/kvm/psci.c >> @@ -63,7 +63,7 @@ static unsigned long kvm_psci_vcpu_suspend(struct kvm_vcpu *vcpu) >> >> static void kvm_psci_vcpu_off(struct kvm_vcpu *vcpu) >> { >> - vcpu->arch.pause = true; >> + vcpu->arch.power_off = true; >> } >> >> static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) >> @@ -87,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.pause) { >> + if (!vcpu->arch.power_off) { >> if (kvm_psci_version(source_vcpu) != KVM_ARM_PSCI_0_1) >> return PSCI_RET_ALREADY_ON; >> else >> @@ -115,7 +115,7 @@ static unsigned long kvm_psci_vcpu_on(struct kvm_vcpu *source_vcpu) >> * the general puspose registers are undefined upon CPU_ON. >> */ >> *vcpu_reg(vcpu, 0) = context_id; >> - vcpu->arch.pause = false; >> + vcpu->arch.power_off = false; >> smp_mb(); /* Make sure the above is visible */ >> >> wq = kvm_arch_vcpu_wq(vcpu); >> @@ -152,7 +152,7 @@ static unsigned long kvm_psci_vcpu_affinity_info(struct kvm_vcpu *vcpu) >> kvm_for_each_vcpu(i, tmp, kvm) { >> mpidr = kvm_vcpu_get_mpidr_aff(tmp); >> if (((mpidr & target_affinity_mask) == target_affinity) && >> - !tmp->arch.pause) { >> + !tmp->arch.power_off) { >> return PSCI_0_2_AFFINITY_LEVEL_ON; >> } >> } >> @@ -175,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.pause = true; >> + tmp->arch.power_off = true; >> kvm_vcpu_kick(tmp); >> } >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 2709db2..009da6b 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -122,8 +122,8 @@ struct kvm_vcpu_arch { >> * here. >> */ >> >> - /* Don't run the guest */ >> - bool pause; >> + /* vcpu power-off state */ >> + bool power_off; >> >> /* IO related fields */ >> struct kvm_decode mmio_decode; >> -- >> 1.9.1 >>
next prev parent reply other threads:[~2015-08-07 12:24 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 [this message] 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 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=55C4A658.3060302@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.