Re: [PATCH v5 3/6] xen/arm: vgic-v2: Don't ignore a write in ITARGETSR if one field is 0

[Julien Grall <julien.grall@citrix.com>]

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