From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs Date: Wed, 18 Jun 2014 11:48:07 +0100 Message-ID: <1403088487.24051.7.camel@kazak.uk.xensource.com> References: <1402504032-13267-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: <1402504032-13267-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 Cc: julien.grall@citrix.com, xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote: > vgic_enable_irqs should enable irq delivery to the vcpu specified by > GICD_ITARGETSR, rather than the vcpu that wrote to GICD_ISENABLER. > Similarly vgic_disable_irqs should use the target vcpu specified by > itarget to disable irqs. > > itargets can be set to a mask but vgic_get_target_vcpu always returns > the lower vcpu in the mask. > > Correctly initialize itargets for SPIs. > > Ignore bits in GICD_ITARGETSR corresponding to invalid vcpus. > > Signed-off-by: Stefano Stabellini > > --- > > Changes in v5: > - improve in-code comments; > - use vgic_rank_irq; > - use bit masks to write-ignore GICD_ITARGETSR; > - introduce an version of vgic_get_target_vcpu that doesn't take the > rank lock; > - keep the rank lock while enabling/disabling irqs; > - use find_first_bit instead of find_next_bit. > > Changes in v4: > - remove assert that could allow a guest to crash Xen; > - add itargets validation to vgic_distr_mmio_write; > - export vgic_get_target_vcpu. > > Changes in v3: > - add assert in get_target_vcpu; > - rename get_target_vcpu to vgic_get_target_vcpu. > > Changes in v2: > - refactor the common code in get_target_vcpu; > - unify PPI and SPI paths; > - correctly initialize itargets for SPI; > - use byte_read. > --- > xen/arch/arm/vgic.c | 71 +++++++++++++++++++++++++++++++++++++-------- > xen/include/asm-arm/gic.h | 2 ++ > 2 files changed, 61 insertions(+), 12 deletions(-) > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 757707e..4d655af 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -111,7 +111,13 @@ int domain_vgic_init(struct domain *d) > INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue); > } > for (i=0; i + { > spin_lock_init(&d->arch.vgic.shared_irqs[i].lock); > + /* By default deliver to CPU0 */ > + memset(d->arch.vgic.shared_irqs[i].itargets, > + 0x1, > + 8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0])); 8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]) == sizeof(d->arch.vgic.shared_irqs[i].itargets) doesn't it? > + } > return 0; > } > > @@ -374,6 +380,32 @@ read_as_zero: > return 1; > } > > +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq) > +{ > + unsigned long target; > + struct vcpu *v_target; > + struct vgic_irq_rank *rank = vgic_rank_irq(v, irq); > + > + target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4); > + /* 1-N SPI should be delivered as pending to all the vcpus in the > + * mask, but here we just return the first vcpu for simplicity and > + * because it would be too slow to do otherwise. */ > + target = find_first_bit((const unsigned long *) &target, 8); Is this definitely aligned ok? Also the cast isn't necessary, the type is already unsigned long * and if find_first_bit takes a const that'll happen automatically. > @@ -589,12 +625,23 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info) > if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width; > rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR); > if ( rank == NULL) goto write_ignore; > + /* 8-bit vcpu mask for this domain */ > + tr = (1 << v->domain->max_vcpus) - 1; > + tr = tr | (tr << 8) | (tr << 16) | (tr << 24); > + tr &= *r; > + /* ignore zero writes */ > + if ( !tr ) > + goto write_ignore; Is this per the spec? Can you provide a reference. If not then I think writing target==0 is the guest's problem. > + if ( dabt.size == 2 && > + !((tr & 0xff) && (tr & (0xff << 8)) && > + (tr & (0xff << 16)) && (tr & (0xff << 24)))) > + goto write_ignore; Isn't this just !(tr & 0xffffffff) ? Even then I'm not sure what it is actually for. > vgic_lock_rank(v, rank); > if ( dabt.size == 2 ) > - rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r; > + rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = tr; > else > byte_write(&rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], > - *r, offset); > + tr, offset); > vgic_unlock_rank(v, rank); > return 1; > > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index bf6fb1e..bd40628 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -227,6 +227,8 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize, > unsigned int *out_hwirq, unsigned int *out_type); > void gic_clear_lrs(struct vcpu *v); > > +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq); > + > #endif /* __ASSEMBLY__ */ > #endif >