From: Andrew Jones <drjones@redhat.com>
To: Christoffer Dall <cdall@linaro.org>
Cc: Marc Zyngier <marc.zyngier@arm.com>,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
Christoffer Dall <christoffer.dall@linaro.org>,
Thomas Gleixner <tglx@linutronix.de>,
Jason Cooper <jason@lakedaemon.net>,
Eric Auger <eric.auger@redhat.com>,
Shanker Donthineni <shankerd@codeaurora.org>,
Mark Rutland <mark.rutland@arm.com>,
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
Subject: Re: [PATCH v3 51/59] KVM: arm/arm64: GICv4: Add doorbell interrupt handling
Date: Wed, 30 Aug 2017 14:28:30 +0200 [thread overview]
Message-ID: <20170830122830.qfa4gbooguee6v5r@kamzik.brq.redhat.com> (raw)
In-Reply-To: <20170830115510.GE24522@cbox>
On Wed, Aug 30, 2017 at 01:55:10PM +0200, Christoffer Dall wrote:
> On Wed, Aug 30, 2017 at 12:31:41PM +0200, Andrew Jones wrote:
> > On Mon, Aug 28, 2017 at 08:18:50PM +0200, Christoffer Dall wrote:
> > > On Fri, Aug 04, 2017 at 08:44:04AM +0100, Marc Zyngier wrote:
> > > > On 31/07/17 18:26, Marc Zyngier wrote:
> > > > > When a vPE is not running, a VLPI being made pending results in a
> > > > > doorbell interrupt being delivered. Let's handle this interrupt
> > > > > and update the pending_last flag that indicates that VLPIs are
> > > > > pending. The corresponding vcpu is also kicked into action.
> > > > >
> > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> > > > > ---
> > > > > virt/kvm/arm/vgic/vgic-v4.c | 34 ++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 34 insertions(+)
> > > > >
> > > > > diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> > > > > index 534d3051a078..6af3cde6d7d4 100644
> > > > > --- a/virt/kvm/arm/vgic/vgic-v4.c
> > > > > +++ b/virt/kvm/arm/vgic/vgic-v4.c
> > > > > @@ -21,6 +21,19 @@
> > > > >
> > > > > #include "vgic.h"
> > > > >
> > > > > +static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
> > > > > +{
> > > > > + struct kvm_vcpu *vcpu = info;
> > > > > +
> > > > > + if (!kvm_vgic_vcpu_pending_irq(vcpu)) {
> > > > > + vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
> > > > > + kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> > > > > + kvm_vcpu_kick(vcpu);
> > > > > + }
> > > >
> > > > This code is so obviously broken that I completely overlooked it.
> > > >
> > > > If we have take a doorbell interrupt, then it means nothing was
> > > > otherwise pending (because we'd have been kicked out of the blocking
> > > > state, and will have masked the doorbell). So checking for pending
> > > > interrupts is pointless.
> > > >
> > > > Furthermore, calling kvm_vgic_vcpu_pending_irq() takes the ap_list
> > > > lock. If we take a doorbell interrupt while injecting a virtual
> > > > interrupt (from userspace, for example) on the same CPU, we end-up
> > > > in deadlock land. This would be solved by Christoffer's latest
> > > > crop of timer patches, but there is no point getting there the first
> > > > place.
> > > >
> > > > The patchlet below solves it:
> > > >
> > > > diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> > > > index 15feb1151797..48e4d6ebeaa8 100644
> > > > --- a/virt/kvm/arm/vgic/vgic-v4.c
> > > > +++ b/virt/kvm/arm/vgic/vgic-v4.c
> > > > @@ -94,11 +94,9 @@ static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
> > > > {
> > > > struct kvm_vcpu *vcpu = info;
> > > >
> > > > - if (!kvm_vgic_vcpu_pending_irq(vcpu)) {
> > > > - vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
> > > > - kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> > > > - kvm_vcpu_kick(vcpu);
> > > > - }
> > > > + vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
> > > > + kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> > > > + kvm_vcpu_kick(vcpu);
> > >
> > > I don't think you need the request and kick, because if you're getting
> > > this doorbell, doesn't that also mean that the VCPU is not running in
> > > the guest and you simply need to make sure the VCPU thread gets
> > > scheduled again, so you could call kvm_vcpu_wake_up() instead.
> >
> > While we do that in kvm_timer_inject_irq_work(), which is scheduled from
> > the same function that vgic_v4_doorbell_handler() would be enabled from
> > (kvm_vcpu_block->kvm_arch_vcpu_blocking), just a wake up isn't sufficient
> > in this case.
> >
> > >
> > > Unless the request is there to ensure proper memory barriers around
> > > setting pending_last?
> >
> > Right, unlike pending timers, we need the barriers in this handler.
> > Pending timers are safe because their pending test compares state set by
> > the VCPU thread itself to state acquired by the VCPU thread reading a host
> > sysreg itself. IOW, handlers making "set vcpu timer pending requests"
> > don't need to do anything but wake up the VCPU. Handlers that set some
> > sort of irq pending state, like vgic_v4_doorbell_handler(), do need to
> > worry about the visibility of that state though.
> >
> > >
> > > In that case, is the read barrier taken care of by prepare_to_swait in
> > > kvm_vcpu_block()?
> >
> > Thanks for the bug report :-)
> >
> > There's a barrier, but it's not properly paired. Currently we have
> >
> > VCPU handler
> > ---- -------
> > for (;;) {
> > WRITE_ONCE(task->state, INTERRUPTIBLE); pending=true;
> > smp_mb(); smp_wmb(); // kvm_make_request()
> > if (pending) { WRITE_ONCE(vcpu->state, NORMAL);
> > ... stop waiting ...
> > }
> > schedule();
> > }
> >
> > Proper barrier use with swait/swake should instead look like this
> >
> > VCPU handler
> > ---- -------
> > for (;;) {
> > WRITE_ONCE(task->state, INTERRUPTIBLE);
> > smp_mb();
> > if (READ_ONCE(task->state) == NORMAL) { pending=true;
> > smp_rmb(); smp_wmb();
> > if (pending) WRITE_ONCE(vcpu->state, NORMAL);
> > ... stop waiting ...
> > else
> > continue;
> > }
> > schedule();
> > }
> >
> > But checking task state adds complexity and would only cover a small
> > window anyway (the window between prepare_to_swait() and
> > kvm_vcpu_check_block()). We need to cover a larger window, for this
> > particular case it's from kvm_arch_vcpu_blocking() to
> > kvm_vcpu_check_block(). Better use of VCPU requests is the answer:
> >
> > VCPU handler
> > ---- -------
> > kvm_arch_vcpu_blocking();
> > for (;;) {
> > prepare_to_swait();
> > if (test_bit(IRQ_PENDING)) { pending=true;
> > smp_rmb(); smp_wmb();
> > if (pending) set_bit(IRQ_PENDING);
> > ... stop waiting ...
> > else
> > continue;
> > }
> > schedule();
> > }
> >
> > The handler side is covered by kvm_make_request() and the vcpu kick, so
> > this patch looks good. However we need a patch to kvm_arch_vcpu_runnable()
> > to fix the VCPU side.
> >
> > int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
> > {
> > - return ((!!v->arch.irq_lines || kvm_vgic_vcpu_pending_irq(v))
> > - && !v->arch.power_off && !v->arch.pause);
> > + if (v->arch.power_off || v->arch.pause)
> > + return false;
>
> Why don't we need to ensure reads of these flags are observable when
> waking up, if the thread waking us up had se the variables prior to
> issuing the wake up call?
They're "special". I analyzed the cases for them and I *think* they're
all safe. I can do another round, or perhaps better would be to replace
them in some way with VCPU requests to make the analysis consistent.
>
> > +
> > + if (kvm_test_request(KVM_REQ_IRQ_PENDING, vcpu)) {
> > + smp_rmb();
>
> This looks wrong. What does this barrier ensure exactly? Some kind of
> read following whoever called this function?
Yes, it's similar to how kvm_check_request() is written. Actually, I
should probably just copy that more completely, i.e.
if (kvm_test_request(KVM_REQ_IRQ_PENDING, vcpu)) {
/*
* Ensure the rest of the request is visible to the caller.
* Pairs with the smp_wmb in kvm_make_request.
*/
smp_mb__after_atomic();
return true;
}
>
> > + return true;
> > + }
> > }
> >
> > We can keep the kvm_vgic_vcpu_pending_irq() check, after the IRQ_PENDING
> > check, if there are cases where IRQ_PENDING wouldn't be set, but
> > kvm_vgic_vcpu_pending_irq() would return true. Either way we also get a
> > bit of an optimization with this fix.
> >
> > I'll think/test some more, and then send a patch.
> >
>
> I'll review it more thoroughly then.
Thanks,
drew
next prev parent reply other threads:[~2017-08-30 12:28 UTC|newest]
Thread overview: 118+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-31 17:25 [PATCH v3 00/59] irqchip: KVM: Add support for GICv4 Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 01/59] genirq: Let irq_set_vcpu_affinity() iterate over hierarchy Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 02/59] irqchip/gic-v3: Add redistributor iterator Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 03/59] irqchip/gic-v3: Add VLPI/DirectLPI discovery Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 04/59] irqchip/gic-v3-its: Move LPI definitions around Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 05/59] irqchip/gic-v3-its: Add probing for VLPI properties Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 06/59] irqchip/gic-v3-its: Macro-ize its_send_single_command Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 07/59] irqchip/gic-v3-its: Implement irq_set_irqchip_state for pending state Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 08/59] irqchip/gic-v3-its: Split out property table allocation Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 09/59] irqchip/gic-v3-its: Allow use of indirect VCPU tables Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 10/59] irqchip/gic-v3-its: Split out pending table allocation Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 11/59] irqchip/gic-v3-its: Rework LPI freeing Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 12/59] irqchip/gic-v3-its: Generalize device table allocation Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 13/59] irqchip/gic-v3-its: Generalize LPI configuration Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 14/59] irqchip/gic-v4: Add management structure definitions Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 15/59] irqchip/gic-v3-its: Add GICv4 ITS command definitions Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 16/59] irqchip/gic-v3-its: Add VLPI configuration hook Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 17/59] irqchip/gic-v3-its: Add VLPI map/unmap operations Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 18/59] irqchip/gic-v3-its: Add VLPI configuration handling Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 19/59] irqchip/gic-v3-its: Add VPE domain infrastructure Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 20/59] irqchip/gic-v3-its: Add VPE irq domain allocation/teardown Marc Zyngier
2017-07-31 17:25 ` [PATCH v3 21/59] irqchip/gic-v3-its: Add VPE irq domain [de]activation Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 22/59] irqchip/gic-v3-its: Add VPENDBASER/VPROPBASER accessors Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 23/59] irqchip/gic-v3-its: Add VPE scheduling Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 24/59] irqchip/gic-v3-its: Add VPE invalidation hook Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 25/59] irqchip/gic-v3-its: Add VPE affinity changes Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 26/59] irqchip/gic-v3-its: Add VPE interrupt masking Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 27/59] irqchip/gic-v3-its: Support VPE doorbell invalidation even when !DirectLPI Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 28/59] irqchip/gic-v3-its: Allow doorbell interrupts to be injected/cleared Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 29/59] irqchip/gic-v3-its: Set implementation defined bit to enable VLPIs Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 30/59] irqchip/gic-v4: Add per-VM VPE domain creation Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 31/59] irqchip/gic-v4: Add VPE command interface Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 32/59] irqchip/gic-v4: Add VLPI configuration interface Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 33/59] irqchip/gic-v4: Add some basic documentation Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 34/59] irqchip/gic-v4: Enable low-level GICv4 operations Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 35/59] irqchip/gic-v3: Advertise GICv4 support to KVM Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 36/59] KVM: arm: Select ARM_GIC_V3 and ARM_GIC_V3_ITS Marc Zyngier
2017-08-26 19:49 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 37/59] KVM: arm/arm64: vgic: Move kvm_vgic_destroy call around Marc Zyngier
2017-08-26 19:49 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 38/59] KVM: arm/arm64: vITS: Add MSI translation helpers Marc Zyngier
2017-08-26 19:49 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 39/59] KVM: arm/arm64: GICv4: Add property field and per-VM predicate Marc Zyngier
2017-08-26 19:49 ` Christoffer Dall
2017-08-30 9:46 ` Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 40/59] KVM: arm/arm64: GICv4: Add init/teardown of the per-VM vPE irq domain Marc Zyngier
2017-08-26 19:49 ` Christoffer Dall
2017-08-30 9:50 ` Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 41/59] KVM: arm/arm64: GICv4: Wire mapping/unmapping of VLPIs in VFIO irq bypass Marc Zyngier
2017-08-26 19:48 ` Christoffer Dall
2017-08-30 9:42 ` Marc Zyngier
2017-08-30 10:20 ` Auger Eric
2017-08-30 10:42 ` Marc Zyngier
2017-08-30 12:54 ` Auger Eric
2017-08-30 10:28 ` Marc Zyngier
2017-08-30 11:46 ` Christoffer Dall
2017-08-30 12:53 ` Marc Zyngier
2017-08-30 19:59 ` Christoffer Dall
2017-08-31 10:24 ` Marc Zyngier
2017-08-31 12:36 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 42/59] KVM: arm/arm64: GICv4: Handle INT command applied to a VLPI Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 43/59] KVM: arm/arm64: GICv4: Unmap VLPI when freeing an LPI Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-08-30 11:03 ` Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 44/59] KVM: arm/arm64: GICv4: Handle MOVI applied to a VLPI Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-08-30 14:08 ` Marc Zyngier
2017-08-30 20:04 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 45/59] KVM: arm/arm64: GICv4: Handle CLEAR " Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 46/59] KVM: arm/arm64: GICv4: Handle MOVALL applied to a vPE Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-08-30 14:46 ` Marc Zyngier
2017-08-30 20:10 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 47/59] KVM: arm/arm64: GICv4: Propagate property updates to VLPIs Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 48/59] KVM: arm/arm64: GICv4: Handle INVALL applied to a vPE Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 49/59] KVM: arm/arm64: GICv4: Propagate VLPI properties at map time Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-08-30 14:56 ` Marc Zyngier
2017-08-30 20:12 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 50/59] KVM: arm/arm64: GICv4: Use pending_last as a scheduling hint Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 51/59] KVM: arm/arm64: GICv4: Add doorbell interrupt handling Marc Zyngier
2017-08-04 7:44 ` Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-08-30 10:31 ` Andrew Jones
2017-08-30 11:55 ` Christoffer Dall
2017-08-30 12:28 ` Andrew Jones [this message]
2017-08-31 12:18 ` Marc Zyngier
2017-08-31 12:41 ` Christoffer Dall
2017-08-28 18:18 ` Christoffer Dall
2017-08-30 15:36 ` Marc Zyngier
2017-08-30 20:58 ` Christoffer Dall
2017-08-31 8:19 ` Marc Zyngier
2017-09-06 9:06 ` Shannon Zhao
2017-07-31 17:26 ` [PATCH v3 52/59] KVM: arm/arm64: GICv4: Use the doorbell interrupt as an unblocking source Marc Zyngier
2017-08-28 18:19 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 53/59] KVM: arm/arm64: GICv4: Hook vPE scheduling into vgic flush/sync Marc Zyngier
2017-08-28 18:17 ` Christoffer Dall
2017-08-30 9:59 ` Marc Zyngier
2017-08-30 11:56 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 54/59] KVM: arm/arm64: GICv4: Enable virtual cpuif if VLPIs can be delivered Marc Zyngier
2017-08-28 18:20 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 55/59] KVM: arm/arm64: GICv4: Enable VLPI support Marc Zyngier
2017-08-28 18:25 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 56/59] KVM: arm/arm64: GICv4: Prevent heterogenous systems from using GICv4 Marc Zyngier
2017-08-28 18:35 ` Christoffer Dall
2017-08-30 16:03 ` Marc Zyngier
2017-08-30 21:00 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 57/59] KVM: arm/arm64: GICv4: Theory of operations Marc Zyngier
2017-08-28 18:18 ` Christoffer Dall
2017-08-30 11:30 ` Marc Zyngier
2017-08-30 11:58 ` Christoffer Dall
2017-07-31 17:26 ` [PATCH v3 58/59] irqchip/gic-v3-its: Pass its_node pointer to each command bulder Marc Zyngier
2017-07-31 17:26 ` [PATCH v3 59/59] irqchip/gic-v3-its: Workaround Huawei D05 redistributor addressing 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=20170830122830.qfa4gbooguee6v5r@kamzik.brq.redhat.com \
--to=drjones@redhat.com \
--cc=cdall@linaro.org \
--cc=christoffer.dall@linaro.org \
--cc=eric.auger@redhat.com \
--cc=jason@lakedaemon.net \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marc.zyngier@arm.com \
--cc=mark.rutland@arm.com \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=shankerd@codeaurora.org \
--cc=tglx@linutronix.de \
/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).