From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v11 03/10] xen/arm: inflight irqs during migration Date: Fri, 08 Aug 2014 18:28:40 +0100 Message-ID: <53E508C8.6080708@linaro.org> References: <1407518033-10694-3-git-send-email-stefano.stabellini@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1407518033-10694-3-git-send-email-stefano.stabellini@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini , xen-devel@lists.xensource.com Cc: julien.grall@citrix.com, Ian.Campbell@citrix.com List-Id: xen-devel@lists.xenproject.org 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. > + /* 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? Regards, -- Julien Grall