From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v4 2/5] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0 Date: Thu, 22 Oct 2015 17:07:51 +0100 Message-ID: <1445530071.2374.40.camel@citrix.com> References: <1444659760-24123-1-git-send-email-julien.grall@citrix.com> <1444659760-24123-3-git-send-email-julien.grall@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZpIPF-0000Sg-FC for xen-devel@lists.xenproject.org; Thu, 22 Oct 2015 16:07:57 +0000 In-Reply-To: <1444659760-24123-3-git-send-email-julien.grall@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: Julien Grall , xen-devel@lists.xenproject.org Cc: stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On Mon, 2015-10-12 at 15:22 +0100, Julien Grall wrote: > The current implementation ignores the whole write if one of the field is > 0. Although, based on the spec (4.3.12 IHI 0048B.b), 0 is a valid value > when: > - The interrupt is not wired in the distributor. From the Xen > point of view, it means that the corresponding bit is not set in > d->arch.vgic.allocated_irqs. > - The user wants to disable the IRQ forwarding in the distributor. > I.e the IRQ stays pending in the distributor and never received by > the guest. > > Implementing the later will require more work in Xen because we always > assume the interrupt is forwarded to vCPU. So for now, ignore any field > where the value is 0. > > The emulation of the write access of ITARGETSR has been reworked and > moved to a new function because it would have been difficult to > implement properly the behavior with the current code. > > The new implementation is breaking the register in 4 distinct bytes. For > each byte, it will check the validity of the target list, find the new > target, migrate the interrupt and store the value if necessary. > > In the new implementation there is nearly no distinction of the access > size to avoid having too many different path which is harder to test. > > Signed-off-by: Julien Grall > > --- > This change used to be embedded in "xen/arm: vgic: Optimize the way > to store the target vCPU in the rank". It has been moved out to > avoid having too much functional changes in a single patch. > > I'm not sure if this patch should be backported to Xen 4.6. Without > it any guest writing 0 in one the field won't be able to migrate > other interrupts. Although, in all the use case I've seen, the guest > is read ITARGETSR first and write-back the value with the > corresponding byte changed. > > Changes in v4: > - Patch added > --- > xen/arch/arm/vgic-v2.c | 141 ++++++++++++++++++++++++++++++++++--------- > ------ > 1 file changed, 98 insertions(+), 43 deletions(-) > > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c > index 665afeb..6b7eab3 100644 > --- a/xen/arch/arm/vgic-v2.c > +++ b/xen/arch/arm/vgic-v2.c > @@ -50,6 +50,94 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, > paddr_t vbase) > vgic_v2_hw.vbase = vbase; > } > > +#define NR_TARGET_PER_REG 4U > +#define NR_BIT_PER_TARGET 8U NR_TARGETS_ and NR_BITS_... "REG" there is a bit generic, given this only works for registers with 8 -bit fields, _ITARGETSR instead? > +/* > + * Store a ITARGETSR register. This function only deals with ITARGETSR8 > + * and onwards. > + * > + * Note the offset will be aligned to the appropriate boundary. > + */ > +static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank > *rank, > + unsigned int offset, uint32_t > itargetsr) > +{ > + unsigned int i; > + unsigned int regidx = REG_RANK_INDEX(8, offset, DABT_WORD); > + unsigned int virq; > + > + ASSERT(spin_is_locked(&rank->lock)); > + > + /* > + * The ITARGETSR0-7, used for SGIs/PPIs, are implemented RO in the > + * emulation and should never call this function. > + * > + * They all live in the first rank. > + */ > + BUILD_BUG_ON(NR_INTERRUPT_PER_RANK != 32); > + ASSERT(rank->index >= 1); > + > + offset &= INTERRUPT_RANK_MASK; > + offset &= ~(NR_TARGET_PER_REG - 1); > + > + virq = rank->index * NR_INTERRUPT_PER_RANK + offset; > + > + for ( i = 0; i < NR_TARGET_PER_REG; i++, offset++, virq++ ) > + { > + unsigned int new_target, old_target; > + uint8_t new_mask, old_mask; > + > + new_mask = itargetsr & ((1 << NR_BIT_PER_TARGET) - 1); > + old_mask = vgic_byte_read(rank->v2.itargets[regidx], i); > + > + itargetsr >>= NR_BIT_PER_TARGET; This is really a part of the for() iteration expression, but oddly place here. If you turn the "((1 << NR_BIT_PER_TARGET) - 1)" thing into a #define or constant, then you may find that extracting the relevant byte from the unshifted itargetsr using (itargetsr >> (i*NR_BITS_PER_TARGET) and then applying the mask is clean enough to use here instead. I don't think you rely on the shifting of itargetsr apart from when calculating new_mask. > + /* > + * SPIs are using the 1-N model (see 1.4.3 in ARM IHI 0048B). > + * While the interrupt could be set pending to all the vCPUs in > + * target list, it's not guarantee by the spec. "guaranteed" > + * For simplicity, always route the vIRQ to the first interrupt > + * in the target list > + */ > + new_target = ffs(new_mask); > + old_target = ffs(old_mask); > + > + /* The current target should always be valid */ > + ASSERT(old_target && (old_target <= d->max_vcpus)); > + > + /* > + * Ignore the write request for this interrupt if the new target > + * is invalid. > + * XXX: From the spec, if the target list is not valid, the > + * interrupt should be ignored (i.e not forwarding to the "forwarded". > + * guest). > + */ > + if ( !new_target || (new_target > d->max_vcpus) ) > + { > + printk(XENLOG_G_DEBUG gdprintk? > + "d%d: No valid vCPU found for vIRQ%u in the target list (%#x). Skip it\n", > + d->domain_id, virq, new_mask); > + continue; > + } > + > + /* The vCPU ID always starts from 0 */ > + new_target--; > + old_target--; > + > + /* Only migrate the vIRQ if the target vCPU has changed */ > + if ( new_target != old_target ) > + { > + vgic_migrate_irq(d->vcpu[old_target], > + d->vcpu[new_target], > + virq); > + } > + > + /* Bit corresponding to unimplemented CPU is write-ignored. */ "write-ignore" > + new_mask &= (1 << d->max_vcpus) - 1; > + vgic_byte_write(&rank->v2.itargets[regidx], new_mask, i); > + } > +} > + > static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info, > register_t *r, void *priv) > { > @@ -337,56 +425,23 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, > mmio_info_t *info, > > case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN: > { > - /* unsigned long needed for find_next_bit */ > - unsigned long target; > - int i; > + uint32_t itargetsr; > + > 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); > - target = (1 << v->domain->max_vcpus) - 1; > - target = target | (target << 8) | (target << 16) | (target << > 24); > + vgic_lock_rank(v, rank, flags); > if ( dabt.size == DABT_WORD ) > - target &= r; > + itargetsr = r; > else > - target &= (r << (8 * (gicd_reg & 0x3))); > - /* ignore zero writes */ > - if ( !target ) > - goto write_ignore; > - /* For word reads ignore writes where any single byte is zero */ > - if ( dabt.size == 2 && > - !((target & 0xff) && (target & (0xff << 8)) && > - (target & (0xff << 16)) && (target & (0xff << 24)))) > - goto write_ignore; > - vgic_lock_rank(v, rank, flags); > - 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 > ->v2.itargets[REG_RANK_INDEX(8, > - gicd_reg - GICD_ITARGETSR, > DABT_WORD)], i/8); > - old_target = find_first_bit(&old_target_mask, 8); > - > - if ( new_target != old_target ) > - { > - irq = (gicd_reg & ~0x3) - 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; > + itargetsr = rank->v2.itargets[REG_RANK_INDEX(8, > + gicd_reg - GICD_ITARGETSR, > + DABT_WORD)]; > + vgic_byte_write(&itargetsr, r, gicd_reg); > } > - if ( dabt.size == DABT_WORD ) > - rank->v2.itargets[REG_RANK_INDEX(8, gicd_reg - > GICD_ITARGETSR, > - DABT_WORD)] = target; > - else > - vgic_byte_write(&rank->v2.itargets[REG_RANK_INDEX(8, > - gicd_reg - GICD_ITARGETSR, DABT_WORD)], r, > gicd_reg); > + vgic_store_itargetsr(v->domain, rank, gicd_reg - GICD_ITARGETSR, > + itargetsr); > vgic_unlock_rank(v, rank, flags); > return 1; > }