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

[Peter Maydell]

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