RE: [PATCH v2 1/3] arm_gic: Mask the un-supported priority bits

[Sai Pavan Boddu]

Hi Peter,

Will send V3 for below comments.
In v2 I may have confused with functionality of group priority interrupt bits. 
Now things look clear. Thanks.

Regards,
Sai Pavan

> -----Original Message-----
> From: Peter Maydell <address@hidden>
> Sent: Friday, February 21, 2020 9:00 PM
> To: Sai Pavan Boddu <address@hidden>
> Cc: Edgar E . Iglesias <address@hidden>; Alistair Francis
> <address@hidden>; Anthony Liguori <address@hidden>;
> Andreas Färber <address@hidden>; qemu-arm <address@hidden>;
> QEMU Developers <address@hidden>
> Subject: Re: [PATCH v2 1/3] arm_gic: Mask the un-supported priority bits
> 
> On Fri, 21 Feb 2020 at 07:46, Sai Pavan Boddu
> <address@hidden> wrote:
> >
> > Priority bits implemented in arm-gic can be 8 to 4, un-implemented
> > bits are read as zeros(RAZ).
> >
> > Signed-off-by: Sai Pavan Boddu <address@hidden>
> > ---
> >  hw/intc/arm_gic.c                | 26 ++++++++++++++++++++++++--
> >  hw/intc/arm_gic_common.c         |  1 +
> >  include/hw/intc/arm_gic_common.h |  1 +
> >  3 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index
> > 1d7da7b..dec8767 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -641,6 +641,23 @@ uint32_t gic_acknowledge_irq(GICState *s, int
> cpu, MemTxAttrs attrs)
> >      return ret;
> >  }
> >
> > +static uint32_t gic_fullprio_mask(GICState *s, int cpu) {
> > +    /*
> > +     * Return a mask word which clears the unimplemented priority
> > +     * bits from a priority value for an interrupt. (Not to be
> > +     * confused with the group priority, whose mask depends on BPR.)
> > +     */
> > +    int unimpBits;
> > +
> > +    if (gic_is_vcpu(cpu)) {
> > +        unimpBits = GIC_VIRT_MAX_GROUP_PRIO_BITS;
> > +    } else {
> > +        unimpBits = 8 - s->n_prio_bits;
> 
> This isn't right; GIC_VIRT_MAX_GROUP_PRIO_BITS should be handled the
> same way as s->n_prio_bits. The expression I suggested in my comment on
> your v1 should work:
> 
>     if (gic_is_vcpu(cpu)) {
>         pribits = GIC_VIRT_MAX_GROUP_PRIO_BITS;
>     } else {
>         pribits = s->n_prio_bits;
>     }
>     return ~0U << (8 - s->n_prio_bits);
> 
> > +    }
> > +    return ~0U << unimpBits;
> > +}
> > +
> >  void gic_dist_set_priority(GICState *s, int cpu, int irq, uint8_t val,
> >                        MemTxAttrs attrs)  {
> 
> 
> You seem to have lost the part of the patch which applies the mask in
> gic_dist_set_priority(). If the GIC only has 5 bits of priority we should not
> allow the guest to set more than that.
> 
> > @@ -669,7 +686,7 @@ static uint32_t gic_dist_get_priority(GICState *s,
> int cpu, int irq,
> >          }
> >          prio = (prio << 1) & 0xff; /* Non-secure view */
> >      }
> > -    return prio;
> > +    return prio & gic_fullprio_mask(s, cpu);
> >  }
> >
> >  static void gic_set_priority_mask(GICState *s, int cpu, uint8_t
> > pmask, @@ -684,7 +701,7 @@ static void gic_set_priority_mask(GICState
> *s, int cpu, uint8_t pmask,
> >              return;
> >          }
> >      }
> > -    s->priority_mask[cpu] = pmask;
> > +    s->priority_mask[cpu] = pmask & gic_fullprio_mask(s, cpu);
> >  }
> >
> >  static uint32_t gic_get_priority_mask(GICState *s, int cpu,
> > MemTxAttrs attrs) @@ -2055,6 +2072,11 @@ static void
> arm_gic_realize(DeviceState *dev, Error **errp)
> >          return;
> >      }
> >
> > +    if (s->n_prio_bits > 8) {
> > +        error_setg(errp, "num-priority-bits cannot be greater than 8");
> > +        return;
> > +    }
> 
> You need to also check that the value is at least as large as the lowest
> permitted value, as I suggested in my v1 comment.
> 
> thanks
> -- PMM