From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs Date: Thu, 19 Jun 2014 19:07:55 +0100 Message-ID: References: <1402504032-13267-3-git-send-email-stefano.stabellini@eu.citrix.com> <1403088487.24051.7.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1403088487.24051.7.camel@kazak.uk.xensource.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: Ian Campbell Cc: julien.grall@citrix.com, xen-devel@lists.xensource.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Wed, 18 Jun 2014, Ian Campbell wrote: > 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? Yeah, I'll make the change. > > + } > > 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? I think so: target is unsigned long and find_first_bit expects an unsigned long. The problem is when target is a uint32_t or an unsigned int and we cast it to unsigned long on armv8. > Also the cast isn't necessary, the type is already unsigned long * and > if find_first_bit takes a const that'll happen automatically. OK > > @@ -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. The reference is 4.3.12 of the GIC architecture specification but unfortunately it is not clearly written how the gic should behave in case of 0 writes to GICD_ITARGETSR. I wrote the patch thinking that a 0 write is invalid and therefore should be ignored. Now I am not sure anymore. You could effectively disable an interrupt by writing 0 to GICD_ITARGETSR. Why do you even need GICD_ICENABLER in that case? That said, the current code cannot deal with itargets being 0, so this check is needed for security. I could change the code to be able to deal with invalid values or 0 values to itargets, then we could remove the check. I am not sure what is the best course of action. > > + 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. tr & 0xffffffff is non-zero if any of the bytes in tr is non-zero. ((tr & 0xff) && (tr & (0xff << 8)) && (tr & (0xff << 16)) && (tr & (0xff << 24))) is non-zero if all the bytes in tr are non-zero. > > 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 > > > >