From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@citrix.com>
Subject: Re: [PATCH v5 3/6] xen/arm: vgic-v2: Don't ignore a
 write in ITARGETSR if one field is 0
Date: Fri, 13 Nov 2015 14:52:27 +0000
Message-ID: <5645F92B.1000405@citrix.com>
References: <1447084181-13677-1-git-send-email-julien.grall@citrix.com>
	<1447084181-13677-4-git-send-email-julien.grall@citrix.com>
	<alpine.DEB.2.02.1511131411060.2653@kaball.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <prvs=7522f59ca=julien.grall@citrix.com>)
	id 1ZxFje-0003D2-0W
	for xen-devel@lists.xenproject.org; Fri, 13 Nov 2015 14:53:54 +0000
In-Reply-To: <alpine.DEB.2.02.1511131411060.2653@kaball.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel@lists.xenproject.org, ian.campbell@citrix.com
List-Id: xen-devel@lists.xenproject.org

On 13/11/15 14:37, Stefano Stabellini wrote:
> On Mon, 9 Nov 2015, Julien Grall wrote:
>> +#define NR_TARGETS_PER_ITARGETSR    4U
>> +#define NR_BITS_PER_TARGET  (32U / NR_TARGETS_PER_ITARGETSR)
>> +
>> +/*
>> + * Store an 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_TARGETS_PER_ITARGETSR - 1);
>> +
>> +    virq = rank->index * NR_INTERRUPT_PER_RANK + offset;
> 
> The patch looks good, but these three lines I think could be replaced
> with:
> 
> virq = offset & ~(NR_TARGETS_PER_ITARGETSR - 1);
> 
> isn't it right?

[...]

>> +    for ( i = 0; i < NR_TARGETS_PER_ITARGETSR; i++, offset++, virq++ )
> 
> offset is not needed in the loop

offset is used in patch #4. Until v3, this patch was melted in patch #4
and by mistake I move the change here.

So all the changes, the 3 lines above + this one, are valid after the
patch #4.

Given that I don't expect much changes in this version, I won't bother
to rework it for this minor and harmless changes.

Regards,

-- 
Julien Grall