From: Andrew Jones <drjones@redhat.com>
To: Christoffer Dall <christoffer.dall@arm.com>
Cc: kvmarm@lists.cs.columbia.edu,
linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org
Subject: Re: [PATCH 3/5] KVM: arm/arm64: Require VCPU threads to turn them self off
Date: Wed, 30 Jan 2019 16:31:34 +0100 [thread overview]
Message-ID: <20190130153134.bqxqknn6t75cea5t@kamzik.brq.redhat.com> (raw)
In-Reply-To: <20190130094909.GF13482@e113682-lin.lund.arm.com>
On Wed, Jan 30, 2019 at 10:49:09AM +0100, Christoffer Dall wrote:
> On Tue, Jan 29, 2019 at 05:16:44PM +0100, Andrew Jones wrote:
> >
> > Summary edit
> >
> > > KVM: arm/arm64: Require VCPU threads to turn them self off
> >
> > themselves
> >
> > On Fri, Jan 25, 2019 at 10:46:54AM +0100, Christoffer Dall wrote:
> > > To avoid a race between turning VCPUs off and turning them on, make sure
> > > that only the VCPU threat itself turns off the VCPU. When other threads
> > > want to turn of a VCPU, they now do this via a request.
> > >
> > > Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
> > > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> > > ---
> > > arch/arm/include/asm/kvm_host.h | 2 ++
> > > arch/arm64/include/asm/kvm_host.h | 2 ++
> > > virt/kvm/arm/arm.c | 8 ++++++--
> > > virt/kvm/arm/psci.c | 11 ++---------
> > > 4 files changed, 12 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> > > index 50e89869178a..b1cfae222441 100644
> > > --- a/arch/arm/include/asm/kvm_host.h
> > > +++ b/arch/arm/include/asm/kvm_host.h
> > > @@ -49,6 +49,8 @@
> > > KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > #define KVM_REQ_IRQ_PENDING KVM_ARCH_REQ(1)
> > > #define KVM_REQ_VCPU_RESET KVM_ARCH_REQ(2)
> > > +#define KVM_REQ_VCPU_OFF \
> > > + KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > >
> > > DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > >
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index da3fc7324d68..d43b13421987 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -49,6 +49,8 @@
> > > KVM_ARCH_REQ_FLAGS(0, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > #define KVM_REQ_IRQ_PENDING KVM_ARCH_REQ(1)
> > > #define KVM_REQ_VCPU_RESET KVM_ARCH_REQ(2)
> > > +#define KVM_REQ_VCPU_OFF \
> > > + KVM_ARCH_REQ_FLAGS(3, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > >
> > > DECLARE_STATIC_KEY_FALSE(userspace_irqchip_in_use);
> > >
> > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> > > index 9c486fad3f9f..785076176814 100644
> > > --- a/virt/kvm/arm/arm.c
> > > +++ b/virt/kvm/arm/arm.c
> > > @@ -404,8 +404,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> > >
> > > static void vcpu_power_off(struct kvm_vcpu *vcpu)
> > > {
> > > - vcpu->arch.power_off = true;
> > > - kvm_make_request(KVM_REQ_SLEEP, vcpu);
> > > + kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
> > > kvm_vcpu_kick(vcpu);
> > > }
> >
> > I think we should leave this function alone. Otherwise if userspace sets
> > the MP state to STOPPED and then queries the state before the vcpu
> > has a chance to manage its vcpu requests, the state will still indicate
> > RUNNBLE. The same goes for a query right after doing a vcpu init.
> >
>
> We can't leave this alone, because that could lead to userspace racing
> with two PSCI_VCPU_ON requests which could then both enter the critical
> section gated only by the cmpxchg in kvm_psci_vcpu_on.
Right. I keep forgetting about that race.
>
> But we could do something like this (completely untested):
>
>
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 1e3195155860..538b5eb9d920 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -404,6 +404,17 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>
> static void vcpu_power_off(struct kvm_vcpu *vcpu)
> {
> + enum vcpu_power_state old_power_state;
> +
> + /*
> + * Set power_state directly to reflect the power state back to user
> + * space even when the VCPU thread has not had a chance to run, but
> + * only if this doesn't accidentally allow interleaved PSCI_VCPU_ON
> + * requests.
> + */
> + old_power_state = cmpxchg(&vcpu->arch.power_state,
> + KVM_ARM_VCPU_ON,
> + KVM_ARM_VCPU_OFF);
> kvm_make_request(KVM_REQ_VCPU_OFF, vcpu);
> kvm_vcpu_kick(vcpu);
> }
Something like that sounds good to me.
>
>
> > >
> > > @@ -646,6 +645,11 @@ static void check_vcpu_requests(struct kvm_vcpu *vcpu)
> > > if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> > > vcpu_req_sleep(vcpu);
> > >
> > > + if (kvm_check_request(KVM_REQ_VCPU_OFF, vcpu)) {
> > > + vcpu->arch.power_off = true;
> > > + vcpu_req_sleep(vcpu);
> > > + }
> > > +
> > > if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
> > > kvm_reset_vcpu(vcpu);
> > >
> > > diff --git a/virt/kvm/arm/psci.c b/virt/kvm/arm/psci.c
> > > index b9cff1d4b06d..20255319e193 100644
> > > --- a/virt/kvm/arm/psci.c
> > > +++ b/virt/kvm/arm/psci.c
> > > @@ -97,9 +97,7 @@ 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_make_request(KVM_REQ_VCPU_OFF, vcpu);
> > > }
> >
> > This was currently fine since it implements CPU_OFF which only applies to
> > the calling vcpu, but there's also no reason not to change it to be
> > consistent with the below change
> >
>
> Same problem as above, we don't want two VCPUs to be messing with a
> target VCPU state at the same time.
>
Yup
Thanks,
drew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-01-30 15:31 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-25 9:46 [PATCH 0/5] KVM: arm/arm64: Fix VCPU power management problems Christoffer Dall
2019-01-25 9:46 ` [PATCH 1/5] KVM: arm/arm64: Reset the VCPU without preemption and vcpu state loaded Christoffer Dall
2019-01-29 15:48 ` Andrew Jones
2019-01-29 16:05 ` Marc Zyngier
2019-01-30 9:22 ` Christoffer Dall
2019-02-04 15:15 ` Andrew Jones
2019-02-20 19:14 ` Dave Martin
2019-02-20 19:41 ` Marc Zyngier
2019-02-26 13:53 ` Dave Martin
2019-02-27 12:05 ` Dave Martin
2019-02-26 12:34 ` Christoffer Dall
2019-02-26 14:00 ` Dave Martin
2019-01-25 9:46 ` [PATCH 2/5] arm/arm64: KVM: Allow a VCPU to fully reset itself Christoffer Dall
2019-01-29 16:03 ` Andrew Jones
2019-01-30 9:34 ` Christoffer Dall
2019-01-30 15:27 ` Andrew Jones
2019-01-31 7:43 ` Christoffer Dall
2019-01-31 10:12 ` Andrew Jones
2019-01-31 11:51 ` Christoffer Dall
2019-01-31 12:57 ` Andrew Jones
2019-01-31 14:13 ` Christoffer Dall
2019-01-31 14:52 ` Marc Zyngier
2019-01-31 17:06 ` Andrew Jones
2019-02-01 7:58 ` Christoffer Dall
2019-02-04 15:08 ` Andrew Jones
2019-02-04 15:15 ` Andrew Jones
2019-01-25 9:46 ` [PATCH 3/5] KVM: arm/arm64: Require VCPU threads to turn them self off Christoffer Dall
2019-01-29 16:16 ` Andrew Jones
2019-01-30 9:49 ` Christoffer Dall
2019-01-30 15:31 ` Andrew Jones [this message]
2019-01-25 9:46 ` [PATCH 4/5] KVM: arm/arm64: Implement PSCI ON_PENDING when turning on VCPUs Christoffer Dall
2019-01-29 16:27 ` Andrew Jones
2019-01-25 9:46 ` [PATCH 5/5] arm/arm64: KVM: Don't panic on failure to properly reset system registers Christoffer Dall
2019-01-29 16:33 ` Andrew Jones
2019-01-30 9:51 ` Christoffer Dall
2019-01-30 10:56 ` Marc Zyngier
2019-01-30 15:36 ` Andrew Jones
2019-01-31 10:15 ` Marc Zyngier
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=20190130153134.bqxqknn6t75cea5t@kamzik.brq.redhat.com \
--to=drjones@redhat.com \
--cc=christoffer.dall@arm.com \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).