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

[Sai Pavan Boddu]

Hi Peter,

All your suggestions look good, I will send at V2. But  I think I have done a 
mistake in V1, More comments inline below.

> -----Original Message-----
> From: Peter Maydell <address@hidden>
> Sent: Tuesday, February 18, 2020 11:40 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 1/3] arm_gic: Mask the un-supported priority bits
> 
> On Fri, 14 Feb 2020 at 13:21, Sai Pavan Boddu
> <address@hidden> wrote:
> >
> > Priority bits implemented in arm-gic can 8 to 4, un-implemented bits
> > are read as zeros(RAZ).
> 
> This is nice to see -- I've known our GIC was a bit out-of-spec in this area 
> but
> it's good to see it's less painful to retrofit than I thought it might be.
> 
> > Signed-off-by: Sai Pavan Boddu <address@hidden>
> > ---
> >  hw/intc/arm_gic.c                | 9 ++++++---
> >  hw/intc/arm_gic_common.c         | 1 +
> >  include/hw/intc/arm_gic_common.h | 1 +
> >  3 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index
> > 1d7da7b..8875330 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -43,6 +43,9 @@
> >          }                                                               \
> >      } while (0)
> >
> > +#define UMASK(n) \
> > +    ((((1 << n) - 1) << (8 - n)) & 0xFF)
> 
> This is a bit confusingly named (usually 'umask' is the file-permission mask 
> on
> unix). I think it's worth following the pattern used in
> hw/intc/arm_gicv3_cpuif.c for icv_fullprio_mask(), and using a function with
> a comment describing it. Also, you've not considered the virtualization parts
> of the GIC, which also use these codepaths. In those cases, the value of the
> mask is always based on GIC_VIRT_MAX_GROUP_PRIO_BITS of priority (a
> GICv2 has 5 bits of priority in the VGIC, always). So:
> 
> 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 pribits;
> 
>     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);
> }
> 
> > +
> >  static const uint8_t gic_id_11mpcore[] = {
> >      0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05,
> > 0xb1  }; @@ -652,9 +655,9 @@ void gic_dist_set_priority(GICState *s,
> > int cpu, int irq, uint8_t val,
> >      }
> >
> >      if (irq < GIC_INTERNAL) {
> > -        s->priority1[irq][cpu] = val;
> > +        s->priority1[irq][cpu] = val & UMASK(s->n_prio_bits) ;
> >      } else {
> > -        s->priority2[(irq) - GIC_INTERNAL] = val;
> > +        s->priority2[(irq) - GIC_INTERNAL] = val &
> > + UMASK(s->n_prio_bits);
> >      }
> >  }
> 
> Slightly cleaner to just put
>    val &= gic_fullprio_mask(s);
> before the if() rather than doing the same thing in both branches.
> 
> >
> > @@ -684,7 +687,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 & UMASK(s->n_prio_bits);
[Sai Pavan Boddu] mask should be applied in " gic_dist_get_priority ",  as we 
miss group priority bits here.
Let me know, if my understanding is correct.

Thanks for the review.

Regards,
Sai Pavan
> >  }
> >
> >  static uint32_t gic_get_priority_mask(GICState *s, int cpu,
> > MemTxAttrs attrs) diff --git a/hw/intc/arm_gic_common.c
> > b/hw/intc/arm_gic_common.c index e6c4fe7..e4668c7 100644
> > --- a/hw/intc/arm_gic_common.c
> > +++ b/hw/intc/arm_gic_common.c
> > @@ -357,6 +357,7 @@ static Property arm_gic_common_properties[] = {
> >      DEFINE_PROP_BOOL("has-security-extensions", GICState, security_extn,
> 0),
> >      /* True if the GIC should implement the virtualization extensions */
> >      DEFINE_PROP_BOOL("has-virtualization-extensions", GICState,
> > virt_extn, 0),
> > +    DEFINE_PROP_UINT32("num-prio-bits", GICState, n_prio_bits, 8),
> 
> In patch 2 you use "num-priority-bits" for the proprety name on the
> a9mpcore object. I like that better, and I think we should name the property
> the same thing on both devices.
> 
> You should have some code in the realize method which sanity checks the
> n_prio_bits value we are passed. It can't be more than 8, and I'm not sure
> what the lowest valid value is. Your commit message says 4. I'm pretty sure
> that if the GIC has the virt extensions then it can't be less than
> GIC_VIRT_MAX_GROUP_PRIO_BITS (ie 5).
> 
> thanks
> -- PMM