| Message ID | 1402504032-13267-3-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 11/06/14 17:27, Stefano Stabellini wrote: > +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq) [..] > +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq) Can you add comment explaining what are the differences between these 2 functions? AFAIU, the first one is assuming the rank lock is taken. If so, I would add an ASSERT in it. I would avoid people misuse the 2 functions. > case GICD_ISPENDR ... GICD_ISPENDRN: > @@ -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 */ BUG_ON(v->domain->max_vcpus > 8)? Just for enforcement. > + 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; > + if ( dabt.size == 2 && > + !((tr & 0xff) && (tr & (0xff << 8)) && > + (tr & (0xff << 16)) && (tr & (0xff << 24)))) > + goto write_ignore; I quite difficult to understand this check. Does this check is only for word-access? AFAIU, with byte-access it's possible to write 0 in itargets. For this case, the register may contain some 1 out of the byte offset. Regards,
On Thu, 12 Jun 2014, Julien Grall wrote: > Hi Stefano, > > On 11/06/14 17:27, Stefano Stabellini wrote: > > +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq) > > [..] > > > +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq) > > Can you add comment explaining what are the differences between these 2 > functions? > > AFAIU, the first one is assuming the rank lock is taken. If so, I would add an > ASSERT in it. I would avoid people misuse the 2 functions. OK > > case GICD_ISPENDR ... GICD_ISPENDRN: > > @@ -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 */ > > BUG_ON(v->domain->max_vcpus > 8)? Just for enforcement. OK > > + 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; > > + if ( dabt.size == 2 && > > + !((tr & 0xff) && (tr & (0xff << 8)) && > > + (tr & (0xff << 16)) && (tr & (0xff << 24)))) > > + goto write_ignore; > > I quite difficult to understand this check. Does this check is only for > word-access? The previous test covers byte-access after the change of the following patch. I should move it to this patch to make it clearer. > AFAIU, with byte-access it's possible to write 0 in itargets. For this case, > the register may contain some 1 out of the byte offset.
On 12/06/14 15:42, Stefano Stabellini wrote: >>> + 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; >>> + if ( dabt.size == 2 && >>> + !((tr & 0xff) && (tr & (0xff << 8)) && >>> + (tr & (0xff << 16)) && (tr & (0xff << 24)))) >>> + goto write_ignore; >> >> I quite difficult to understand this check. Does this check is only for >> word-access? > > The previous test covers byte-access after the change of the following > patch. I should move it to this patch to make it clearer. Yes please. Regards,
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 <stefano.stabellini@eu.citrix.com> > > --- > > 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<DOMAIN_NR_RANKS(d); 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 >
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 <stefano.stabellini@eu.citrix.com> > > > > --- > > > > 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<DOMAIN_NR_RANKS(d); 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 > > > >
On Thu, 19 Jun 2014, Stefano Stabellini wrote: > 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 <stefano.stabellini@eu.citrix.com> > > > > > > --- > > > > > > 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<DOMAIN_NR_RANKS(d); 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. I confirm that a real GICv2 (the one on Midway) ignores 0 writes. I strongly suggest we do the same.
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<DOMAIN_NR_RANKS(d); 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])); + } 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); + v_target = v->domain->vcpu[target]; + return v_target; +} + +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq) +{ + struct vcpu *v_target; + struct vgic_irq_rank *rank = vgic_rank_irq(v, irq); + + vgic_lock_rank(v, rank); + v_target = _vgic_get_target_vcpu(v, irq); + vgic_unlock_rank(v, rank); + return v_target; +} + static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n) { const unsigned long mask = r; @@ -381,12 +413,14 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n) unsigned int irq; unsigned long flags; int i = 0; + struct vcpu *v_target; while ( (i = find_next_bit(&mask, 32, i)) < 32 ) { irq = i + (32 * n); - p = irq_to_pending(v, irq); + v_target = _vgic_get_target_vcpu(v, irq); + p = irq_to_pending(v_target, irq); clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); - gic_remove_from_queues(v, irq); + gic_remove_from_queues(v_target, irq); if ( p->desc != NULL ) { spin_lock_irqsave(&p->desc->lock, flags); @@ -404,24 +438,26 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n) unsigned int irq; unsigned long flags; int i = 0; + struct vcpu *v_target; while ( (i = find_next_bit(&mask, 32, i)) < 32 ) { irq = i + (32 * n); - p = irq_to_pending(v, irq); + v_target = _vgic_get_target_vcpu(v, irq); + p = irq_to_pending(v_target, irq); set_bit(GIC_IRQ_GUEST_ENABLED, &p->status); /* We need to force the first injection of evtchn_irq because * evtchn_upcall_pending is already set by common code on vcpu * creation. */ - if ( irq == v->domain->arch.evtchn_irq && + if ( irq == v_target->domain->arch.evtchn_irq && vcpu_info(current, evtchn_upcall_pending) && list_empty(&p->inflight) ) - vgic_vcpu_inject_irq(v, irq); + vgic_vcpu_inject_irq(v_target, irq); else { unsigned long flags; - spin_lock_irqsave(&v->arch.vgic.lock, flags); + spin_lock_irqsave(&v_target->arch.vgic.lock, flags); if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) - gic_raise_guest_irq(v, irq, p->priority); - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); + gic_raise_guest_irq(v_target, irq, p->priority); + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); } if ( p->desc != NULL ) { @@ -536,8 +572,8 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info) vgic_lock_rank(v, rank); tr = rank->ienable; rank->ienable |= *r; - vgic_unlock_rank(v, rank); vgic_enable_irqs(v, (*r) & (~tr), gicd_reg - GICD_ISENABLER); + vgic_unlock_rank(v, rank); return 1; case GICD_ICENABLER ... GICD_ICENABLERN: @@ -547,8 +583,8 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info) vgic_lock_rank(v, rank); tr = rank->ienable; rank->ienable &= ~*r; - vgic_unlock_rank(v, rank); vgic_disable_irqs(v, (*r) & tr, gicd_reg - GICD_ICENABLER); + vgic_unlock_rank(v, rank); return 1; case GICD_ISPENDR ... GICD_ISPENDRN: @@ -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; + if ( dabt.size == 2 && + !((tr & 0xff) && (tr & (0xff << 8)) && + (tr & (0xff << 16)) && (tr & (0xff << 24)))) + goto write_ignore; 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
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 <stefano.stabellini@eu.citrix.com> --- 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(-)