From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall 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:51:32 +0100 Message-ID: <56291414.8030007@citrix.com> References: <1444659760-24123-1-git-send-email-julien.grall@citrix.com> <1444659760-24123-3-git-send-email-julien.grall@citrix.com> <1445530071.2374.40.camel@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 1ZpJ6p-0005oL-JU for xen-devel@lists.xenproject.org; Thu, 22 Oct 2015 16:52:59 +0000 In-Reply-To: <1445530071.2374.40.camel@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: Ian Campbell , xen-devel@lists.xenproject.org Cc: stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 22/10/15 17:07, Ian Campbell wrote: >> 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? Well this is within the vgic-v2.c and only one register contains target. So I found pointless to add ITARGETSR to the name. > >> +/* >> + * 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 placed this shift here because I didn't want to use ... >> (i * NR_BIT_..) which require a multiplication rather than a shift in the resulting code. Furthermore, I found the for loop more complicate to read with itargetsr >>= inside. Anyway, I prefer the latter solution so I will use it in the next version. > > I don't think you rely on the shifting of itargetsr apart from when > calculating new_mask. Correct. >> + /* >> + * 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? I would prefer to keep this printk in non-debug to help us catching any OS potentially using this trick. Based on that I would even use XENLOG_G_WARNING because this is not really compliant to the spec and we are meant to fix it. Regards, -- Julien Grall