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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 13 Nov 2015, Julien Grall wrote:
> 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.

Each patch should stand on its own. What if a week from now we want to
revert patch #4 because it causes regressions?

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel