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

[Peter Maydell]

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);
>  }
>
>  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