Re: [PATCH for-4.5 v11 2/7] xen/arm: Add vgic callback to read irq priority

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Tue, 16 Sep 2014, Vijay Kilari wrote:
> On Tue, Sep 16, 2014 at 12:35 AM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 15 Sep 2014, Vijay Kilari wrote:
> >> On Fri, Sep 12, 2014 at 11:38 PM, Stefano Stabellini
> >> <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Fri, 12 Sep 2014, vijay.kilari@gmail.com wrote:
> >> >> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >> >>
> >> >> Use callback in vgic driver to read priority for
> >> >> a given irq
> >> >>
> >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> >> >> ---
> >> >> v11: use vgic_irq_rank
> >> >> ---
> >> >>  xen/arch/arm/vgic-v2.c     |   12 ++++++++++++
> >> >>  xen/arch/arm/vgic.c        |    2 +-
> >> >>  xen/include/asm-arm/vgic.h |    2 ++
> >> >>  3 files changed, 15 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> >> >> index 54751b6..e674192 100644
> >> >> --- a/xen/arch/arm/vgic-v2.c
> >> >> +++ b/xen/arch/arm/vgic-v2.c
> >> >> @@ -521,6 +521,17 @@ static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
> >> >>      return v_target;
> >> >>  }
> >> >>
> >> >> +static int vgic_v2_get_irq_priority(struct vcpu *v, unsigned int irq)
> >> >> +{
> >> >> +    int priority;
> >> >> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> >> >
> >> > this is good
> >> >
> >> >
> >> >> +    ASSERT(spin_is_locked(&rank->lock));
> >> >> +    priority = vgic_byte_read(rank->ipriority[(irq%32)/4], 0, irq % 4);
> >> >
> >> > this has been changed without mention in the change log for the patch, it was
> >> >
> >> > priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8, irq,
> >> >             DABT_WORD)], 0, irq & 0x3);
> >> >
> >> > It is best to use the previous version, as it uses the common conversion
> >> > functions.
> >>
> >>  I changed it because we don't need do clumsy things
> >> to get ipriority index with irq number in hand. If you don't
> >> have any further comments, I will send final version with this change
> >> for your ack.
> >
> > TBH I think that the open coded version is more readable than the macro,
> > but we need to be consistent (so that we can easily change the macro and
> > fix all the call sites in one change for example).
> >
> > In any case I don't think that you need to resend just for this small
> > change. We can fix in a follow up patch.
> 
>  I see that you have made similar changes using in vgic-v2.c in
> 
>  static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
>  {
> ....
>      target = vgic_byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);

Ops, I see that I talk the talk but I don't walk the walk


> shall I revert back to use REG_RANK_INDEX here as well in my next
> commit?

Yes, please