From patchwork Mon Aug 11 15:23:58 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefano Stabellini X-Patchwork-Id: 35209 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-oa0-f72.google.com (mail-oa0-f72.google.com [209.85.219.72]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id C95F020AEC for ; Mon, 11 Aug 2014 15:27:06 +0000 (UTC) Received: by mail-oa0-f72.google.com with SMTP id m1sf39070828oag.7 for ; Mon, 11 Aug 2014 08:27:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:date:from:to:in-reply-to:message-id :references:user-agent:mime-version:cc:subject:precedence:list-id :list-unsubscribe:list-post:list-help:list-subscribe:sender :errors-to:x-original-sender:x-original-authentication-results :mailing-list:list-archive:content-type:content-transfer-encoding; bh=u2EDQXrBtg7CKOA9itx03xE+r+k6WEk1JDw38r29dSo=; b=AkpKHDojoA6YgMy6+lEYNDYIg9iHHiGqnhmol37mLZ4briMyExt3HlEgvlkaL0fCHe aPkoIuVG05YxVhITRwnOGaO462mQyCpHnEkwVI18X8saGmsbmy3iFnBRnu/qrCLLbLjF PWjvKje0aoGED/84gw5nxZCzg+X8vlAFS0lTvN1mxFScQkFQdLw7aw6hueBdCgo4+4Ld CS7XkvkXbuSEpUljy1+tJ9VLG07ow8klCq7kWEfXjr5Y/VAtdWu2VpGp7utrUMeOBJo/ nS+I77MNX9GeVmF7Q5VB52QCXHbK04q71CwHo/1JbX/kpMT8H8I0wp9mnjjYckBbrU6x 3m+g== X-Gm-Message-State: ALoCoQk22PS27ok6sQNBLao+vOeFPhNhsqbuhMAwUlaic+lZ4Uk3US5EZzUvqU8u6u6MoVzNIT/p X-Received: by 10.42.90.209 with SMTP id l17mr399900icm.34.1407770826398; Mon, 11 Aug 2014 08:27:06 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.17.45 with SMTP id 42ls1480342qgc.47.gmail; Mon, 11 Aug 2014 08:27:06 -0700 (PDT) X-Received: by 10.52.1.39 with SMTP id 7mr19422696vdj.17.1407770826271; Mon, 11 Aug 2014 08:27:06 -0700 (PDT) Received: from mail-vc0-f180.google.com (mail-vc0-f180.google.com [209.85.220.180]) by mx.google.com with ESMTPS id sb10si6640950vdc.89.2014.08.11.08.27.06 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Mon, 11 Aug 2014 08:27:06 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.180 as permitted sender) client-ip=209.85.220.180; Received: by mail-vc0-f180.google.com with SMTP id ij19so11478594vcb.25 for ; Mon, 11 Aug 2014 08:27:06 -0700 (PDT) X-Received: by 10.52.129.165 with SMTP id nx5mr19309924vdb.25.1407770826168; Mon, 11 Aug 2014 08:27:06 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.221.37.5 with SMTP id tc5csp173252vcb; Mon, 11 Aug 2014 08:27:05 -0700 (PDT) X-Received: by 10.50.50.175 with SMTP id d15mr26033045igo.35.1407770825103; Mon, 11 Aug 2014 08:27:05 -0700 (PDT) Received: from lists.xen.org (lists.xen.org. [50.57.142.19]) by mx.google.com with ESMTPS id ib3si32518374icc.94.2014.08.11.08.27.04 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Mon, 11 Aug 2014 08:27:05 -0700 (PDT) Received-SPF: none (google.com: xen-devel-bounces@lists.xen.org does not designate permitted sender hosts) client-ip=50.57.142.19; Received: from localhost ([127.0.0.1] helo=lists.xen.org) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XGrTN-0002H4-E5; Mon, 11 Aug 2014 15:25:21 +0000 Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XGrTL-0002Gx-Qb for xen-devel@lists.xensource.com; Mon, 11 Aug 2014 15:25:20 +0000 Received: from [85.158.137.68:27385] by server-7.bemta-3.messagelabs.com id C5/1D-01084-F50E8E35; Mon, 11 Aug 2014 15:25:19 +0000 X-Env-Sender: Stefano.Stabellini@citrix.com X-Msg-Ref: server-2.tower-31.messagelabs.com!1407770716!13035057!1 X-Originating-IP: [66.165.176.63] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni42MyA9PiAzMDYwNDg=\n X-StarScan-Received: X-StarScan-Version: 6.11.3; banners=-,-,- X-VirusChecked: Checked Received: (qmail 1723 invoked from network); 11 Aug 2014 15:25:18 -0000 Received: from smtp02.citrix.com (HELO SMTP02.CITRIX.COM) (66.165.176.63) by server-2.tower-31.messagelabs.com with RC4-SHA encrypted SMTP; 11 Aug 2014 15:25:18 -0000 X-IronPort-AV: E=Sophos;i="5.01,842,1400025600"; d="scan'208";a="161279394" Received: from ukmail1.uk.xensource.com (10.80.16.128) by smtprelay.citrix.com (10.13.107.78) with Microsoft SMTP Server id 14.3.181.6; Mon, 11 Aug 2014 11:25:15 -0400 Received: from kaball.uk.xensource.com ([10.80.2.59]) by ukmail1.uk.xensource.com with esmtp (Exim 4.69) (envelope-from ) id 1XGrTH-0004Nt-FZ; Mon, 11 Aug 2014 16:25:15 +0100 Date: Mon, 11 Aug 2014 16:23:58 +0100 From: Stefano Stabellini X-X-Sender: sstabellini@kaball.uk.xensource.com To: Julien Grall In-Reply-To: <53E508C8.6080708@linaro.org> Message-ID: References: <1407518033-10694-3-git-send-email-stefano.stabellini@eu.citrix.com> <53E508C8.6080708@linaro.org> User-Agent: Alpine 2.02 (DEB 1266 2009-07-14) MIME-Version: 1.0 X-DLP: MIA2 Cc: julien.grall@citrix.com, xen-devel@lists.xensource.com, Ian.Campbell@citrix.com, Stefano Stabellini Subject: Re: [Xen-devel] [PATCH v11 03/10] xen/arm: inflight irqs during migration X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: List-Unsubscribe: , List-Post: , List-Help: , List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: stefano.stabellini@eu.citrix.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.220.180 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Archive: On Fri, 8 Aug 2014, Julien Grall wrote: > Hi Stefano, > > On 08/08/2014 06:13 PM, Stefano Stabellini wrote: > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > > index 63d4f65..39d8272 100644 > > --- a/xen/arch/arm/vgic-v2.c > > +++ b/xen/arch/arm/vgic-v2.c > > @@ -356,34 +356,60 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info) > > goto write_ignore; > > > > case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN: > > + { > > + /* unsigned long needed for find_next_bit */ > > + unsigned long target; > > + int i; > > if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; > > rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD); > > if ( rank == NULL) goto write_ignore; > > /* 8-bit vcpu mask for this domain */ > > BUG_ON(v->domain->max_vcpus > 8); > > - tr = (1 << v->domain->max_vcpus) - 1; > > + target = (1 << v->domain->max_vcpus) - 1; > > if ( dabt.size == 2 ) > > - tr = tr | (tr << 8) | (tr << 16) | (tr << 24); > > + target = target | (target << 8) | (target << 16) | (target << 24); > > else > > - tr = (tr << (8 * (gicd_reg & 0x3))); > > - tr &= *r; > > + target = (target << (8 * (gicd_reg & 0x3))); > > + target &= *r; > > Renaming tr in target in this patch while most of the code has been > introduced in patch #1 is very odd. Actually it make the code harder to > read. > > Anyway, I think it's fine a V11. I don't want to delay the merge just > for that. > > > +void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) > > +{ > > + unsigned long flags; > > + struct pending_irq *p = irq_to_pending(old, irq); > > + > > + /* nothing to do for virtual interrupts */ > > + if ( p->desc == NULL ) > > + return; > > + > > + /* migration already in progress, no need to do anything */ > > + if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) > > + return; > > + > > + spin_lock_irqsave(&old->arch.vgic.lock, flags); > > + > > + if ( list_empty(&p->inflight) ) > > + { > > + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); > > + return; > > + } > > NIT: I would create a label unlock below and jump to it. It would avoid > at least one of the 3 call to spin_unlock. I would rather avoid making this kind of code style changes at this stage. > > + /* If the IRQ is still lr_pending, re-inject it to the new vcpu */ > > + if ( !list_empty(&p->lr_queue) ) > > + { > > + list_del_init(&p->lr_queue); > > + list_del_init(&p->inflight); > > + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); > > + vgic_vcpu_inject_irq(new, irq); > > Shouldn't we also clear the p->status? At least for consistency? We should probably clear GIC_IRQ_GUEST_QUEUED, but in practice it doesn't make any difference because vgic_vcpu_inject_irq immediately sets it again. This is the updated patch with the GIC_IRQ_GUEST_QUEUED clear. Acked-by: Julien Grall --- xen/arm: inflight irqs during migration We need to take special care when migrating irqs that are already inflight from one vcpu to another. See "The effect of changes to an GICD_ITARGETSR", part of chapter 4.3.12 of the ARM Generic Interrupt Controller Architecture Specification, IHI 0048B. The main issue from the Xen point of view is that the lr_pending and inflight lists are per-vcpu. The lock we take to protect them is also per-vcpu. In order to avoid issues, if the irq is still lr_pending, we can immediately move it to the new vcpu for injection. Otherwise if it is in a GICH_LR register, set a new flag GIC_IRQ_GUEST_MIGRATING. If GIC_IRQ_GUEST_MIGRATING is set we'll change the affinity of the physical irq when clearing the LR (in a following commit). Therefore if the irq is inflight in an LR when the guest writes to itarget, we wait until the LR is cleared before changing physical irq affinity. This guarantees that the old vcpu is going to be interrupted and clear the LR, either because it has been descheduled after EOIing the interrupt or because it is interrupted by the second physical irq coming. Signed-off-by: Stefano Stabellini --- Changes in v12: - clear GIC_IRQ_GUEST_QUEUED in vgic_migrate_irq. Changes in v11: - fix irq calculation in vgic_v2_distr_mmio_write. Changes in v9: - simplify the code: rely on "physical irq follow virtual irq" to ensure that physical irqs are injected into the right pcpu. Changes in v8: - rebase on ab78724fc5628318b172b4344f7280621a151e1b; - rename target to new_target to avoid conflicts. Changes in v7: - move the _VPF_down check before setting GIC_IRQ_GUEST_QUEUED; - fix comments; - rename trl to target; - introduce vcpu_migrate_from; - do not kick new vcpu on MIGRATING, kick the old vcpu instead; - separate moving GIC_IRQ_GUEST_QUEUED earlier into a different patch. Changes in v6: - remove unnecessary casts to (const unsigned long *) to the arguments of find_first_bit and find_next_bit; - instroduce a corresponding smb_rmb call; - deal with migrating an irq that is inflight and still lr_pending; - replace the dsb with smb_wmb and smb_rmb, use them to ensure the order of accesses to GIC_IRQ_GUEST_QUEUED and GIC_IRQ_GUEST_MIGRATING; Changes in v5: - pass unsigned long to find_next_bit for alignment on aarch64; - call vgic_get_target_vcpu instead of gic_add_to_lr_pending to add the irq in the right inflight queue; - add barrier and bit tests to make sure that vgic_migrate_irq and gic_update_one_lr can run simultaneously on different cpus without issues; - rework the loop to identify the new vcpu when ITARGETSR is written; - use find_first_bit instead of find_next_bit. --- xen/arch/arm/gic.c | 8 ++++++-- xen/arch/arm/vgic-v2.c | 44 +++++++++++++++++++++++++++++++++++--------- xen/arch/arm/vgic.c | 38 ++++++++++++++++++++++++++++++++++++++ xen/include/asm-arm/vgic.h | 6 ++++++ 4 files changed, 85 insertions(+), 11 deletions(-) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 256d9cf..3e75fc5 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -33,6 +33,7 @@ #include #include #include +#include static void gic_restore_pending_irqs(struct vcpu *v); @@ -375,10 +376,13 @@ static void gic_update_one_lr(struct vcpu *v, int i) clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status); p->lr = GIC_INVALID_LR; if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) && - test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) ) + test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) && + !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) gic_raise_guest_irq(v, irq, p->priority); - else + else { list_del_init(&p->inflight); + clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status); + } } } diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c index 63d4f65..39d8272 100644 --- a/xen/arch/arm/vgic-v2.c +++ b/xen/arch/arm/vgic-v2.c @@ -356,34 +356,60 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info) goto write_ignore; case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN: + { + /* unsigned long needed for find_next_bit */ + unsigned long target; + int i; if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD); if ( rank == NULL) goto write_ignore; /* 8-bit vcpu mask for this domain */ BUG_ON(v->domain->max_vcpus > 8); - tr = (1 << v->domain->max_vcpus) - 1; + target = (1 << v->domain->max_vcpus) - 1; if ( dabt.size == 2 ) - tr = tr | (tr << 8) | (tr << 16) | (tr << 24); + target = target | (target << 8) | (target << 16) | (target << 24); else - tr = (tr << (8 * (gicd_reg & 0x3))); - tr &= *r; + target = (target << (8 * (gicd_reg & 0x3))); + target &= *r; /* ignore zero writes */ - if ( !tr ) + if ( !target ) goto write_ignore; /* For word reads ignore writes where any single byte is zero */ if ( dabt.size == 2 && - !((tr & 0xff) && (tr & (0xff << 8)) && - (tr & (0xff << 16)) && (tr & (0xff << 24)))) + !((target & 0xff) && (target & (0xff << 8)) && + (target & (0xff << 16)) && (target & (0xff << 24)))) goto write_ignore; vgic_lock_rank(v, rank); + i = 0; + while ( (i = find_next_bit(&target, 32, i)) < 32 ) + { + unsigned int irq, new_target, old_target; + unsigned long old_target_mask; + struct vcpu *v_target, *v_old; + + new_target = i % 8; + old_target_mask = vgic_byte_read(rank->itargets[REG_RANK_INDEX(8, + gicd_reg - GICD_ITARGETSR, DABT_WORD)], 0, i/8); + old_target = find_first_bit(&old_target_mask, 8); + + if ( new_target != old_target ) + { + irq = gicd_reg - GICD_ITARGETSR + (i / 8); + v_target = v->domain->vcpu[new_target]; + v_old = v->domain->vcpu[old_target]; + vgic_migrate_irq(v_old, v_target, irq); + } + i += 8 - new_target; + } if ( dabt.size == DABT_WORD ) rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR, - DABT_WORD)] = tr; + DABT_WORD)] = target; else vgic_byte_write(&rank->itargets[REG_RANK_INDEX(8, - gicd_reg - GICD_ITARGETSR, DABT_WORD)], tr, gicd_reg); + gicd_reg - GICD_ITARGETSR, DABT_WORD)], target, gicd_reg); vgic_unlock_rank(v, rank); return 1; + } case GICD_IPRIORITYR ... GICD_IPRIORITYRN: if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width; diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 9947e8c..e0dcaf6 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -165,6 +165,44 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq) return v_target; } +void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) +{ + unsigned long flags; + struct pending_irq *p = irq_to_pending(old, irq); + + /* nothing to do for virtual interrupts */ + if ( p->desc == NULL ) + return; + + /* migration already in progress, no need to do anything */ + if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) + return; + + spin_lock_irqsave(&old->arch.vgic.lock, flags); + + if ( list_empty(&p->inflight) ) + { + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); + return; + } + /* If the IRQ is still lr_pending, re-inject it to the new vcpu */ + if ( !list_empty(&p->lr_queue) ) + { + clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); + list_del_init(&p->lr_queue); + list_del_init(&p->inflight); + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); + vgic_vcpu_inject_irq(new, irq); + return; + } + /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING + * and wait for the EOI */ + if ( !list_empty(&p->inflight) ) + set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status); + + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); +} + void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n) { struct domain *d = v->domain; diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 81a3eef..434a625 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -55,11 +55,16 @@ struct pending_irq * GIC_IRQ_GUEST_ENABLED: the guest IRQ is enabled at the VGICD * level (GICD_ICENABLER/GICD_ISENABLER). * + * GIC_IRQ_GUEST_MIGRATING: the irq is being migrated to a different + * vcpu while it is still inflight and on an GICH_LR register on the + * old vcpu. + * */ #define GIC_IRQ_GUEST_QUEUED 0 #define GIC_IRQ_GUEST_ACTIVE 1 #define GIC_IRQ_GUEST_VISIBLE 2 #define GIC_IRQ_GUEST_ENABLED 3 +#define GIC_IRQ_GUEST_MIGRATING 4 unsigned long status; struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */ int irq; @@ -169,6 +174,7 @@ extern int vcpu_vgic_free(struct vcpu *v); extern int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq, unsigned long vcpu_mask); +extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq); #endif /* __ASM_ARM_VGIC_H__ */ /*