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

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

[Sai Pavan Boddu]

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 <sai.pavan.boddu@xilinx.com>
---
 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;
+    }
+    return ~0U << unimpBits;
+}
+
 void gic_dist_set_priority(GICState *s, int cpu, int irq, uint8_t val,
                       MemTxAttrs attrs)
 {
@@ -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;
+    }
+
     /* This creates distributor, main CPU interface (s->cpuiomem[0]) and if
      * enabled, virtualization extensions related interfaces (main virtual
      * interface (s->vifaceiomem[0]) and virtual CPU interface).
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index e6c4fe7..7b44d56 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-priority-bits", GICState, n_prio_bits, 8),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index b5585fe..6e0d6b8 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -96,6 +96,7 @@ typedef struct GICState {
     uint16_t priority_mask[GIC_NCPU_VCPU];
     uint16_t running_priority[GIC_NCPU_VCPU];
     uint16_t current_pending[GIC_NCPU_VCPU];
+    uint32_t n_prio_bits;
 
     /* If we present the GICv2 without security extensions to a guest,
      * the guest can configure the GICC_CTLR to configure group 1 binary point
-- 
2.7.4

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
<sai.pavan.boddu@xilinx.com> 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 <sai.pavan.boddu@xilinx.com>
> ---
>  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

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 <peter.maydell@linaro.org>
> Sent: Friday, February 21, 2020 9:00 PM
> To: Sai Pavan Boddu <saipava@xilinx.com>
> Cc: Edgar E . Iglesias <edgar.iglesias@gmail.com>; Alistair Francis
> <alistair@alistair23.me>; Anthony Liguori <anthony@codemonkey.ws>;
> Andreas Färber <afaerber@suse.de>; qemu-arm <qemu-arm@nongnu.org>;
> QEMU Developers <qemu-devel@nongnu.org>
> 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
> <sai.pavan.boddu@xilinx.com> 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 <sai.pavan.boddu@xilinx.com>
> > ---
> >  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

[PATCH v2 2/3] cpu/a9mpcore: Set number of GIC priority bits to 5

[Sai Pavan Boddu]

All A9 CPUs have a GIC with 5 bits of priority.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/cpu/a9mpcore.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index 1f8bc8a..b4f6a7e 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -16,6 +16,8 @@
 #include "hw/qdev-properties.h"
 #include "hw/core/cpu.h"
 
+#define A9_GIC_NUM_PRIORITY_BITS    5
+
 static void a9mp_priv_set_irq(void *opaque, int irq, int level)
 {
     A9MPPrivState *s = (A9MPPrivState *)opaque;
@@ -68,6 +70,8 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     gicdev = DEVICE(&s->gic);
     qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
     qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
+    qdev_prop_set_uint32(gicdev, "num-priority-bits",
+                         A9_GIC_NUM_PRIORITY_BITS);
 
     /* Make the GIC's TZ support match the CPUs. We assume that
      * either all the CPUs have TZ, or none do.
-- 
2.7.4

Re: [PATCH v2 2/3] cpu/a9mpcore: Set number of GIC priority bits to 5

[Peter Maydell]

On Fri, 21 Feb 2020 at 07:46, Sai Pavan Boddu
<sai.pavan.boddu@xilinx.com> wrote:
>
> All A9 CPUs have a GIC with 5 bits of priority.
>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/cpu/a9mpcore.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
> index 1f8bc8a..b4f6a7e 100644
> --- a/hw/cpu/a9mpcore.c
> +++ b/hw/cpu/a9mpcore.c
> @@ -16,6 +16,8 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/core/cpu.h"
>
> +#define A9_GIC_NUM_PRIORITY_BITS    5
> +
>  static void a9mp_priv_set_irq(void *opaque, int irq, int level)
>  {
>      A9MPPrivState *s = (A9MPPrivState *)opaque;
> @@ -68,6 +70,8 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
>      gicdev = DEVICE(&s->gic);
>      qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
>      qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
> +    qdev_prop_set_uint32(gicdev, "num-priority-bits",
> +                         A9_GIC_NUM_PRIORITY_BITS);
>
>      /* Make the GIC's TZ support match the CPUs. We assume that
>       * either all the CPUs have TZ, or none do.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 3/3] cpu/arm11mpcore: Set number of GIC priority bits to 4

[Sai Pavan Boddu]

ARM11MPCore GIC is implemented with 4 priority bits.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/cpu/arm11mpcore.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/cpu/arm11mpcore.c b/hw/cpu/arm11mpcore.c
index 2e3e87c..ab9fadb 100644
--- a/hw/cpu/arm11mpcore.c
+++ b/hw/cpu/arm11mpcore.c
@@ -15,6 +15,7 @@
 #include "hw/irq.h"
 #include "hw/qdev-properties.h"
 
+#define ARM11MPCORE_NUM_GIC_PRIORITY_BITS    4
 
 static void mpcore_priv_set_irq(void *opaque, int irq, int level)
 {
@@ -86,6 +87,10 @@ static void mpcore_priv_realize(DeviceState *dev, Error **errp)
 
     qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
     qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
+    qdev_prop_set_uint32(gicdev, "num-priority-bits",
+                         ARM11MPCORE_NUM_GIC_PRIORITY_BITS);
+
+
     object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
     if (err != NULL) {
         error_propagate(errp, err);
-- 
2.7.4

Re: [PATCH v2 3/3] cpu/arm11mpcore: Set number of GIC priority bits to 4

[Peter Maydell]

On Fri, 21 Feb 2020 at 07:46, Sai Pavan Boddu
<sai.pavan.boddu@xilinx.com> wrote:
>
> ARM11MPCore GIC is implemented with 4 priority bits.
>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/cpu/arm11mpcore.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/cpu/arm11mpcore.c b/hw/cpu/arm11mpcore.c
> index 2e3e87c..ab9fadb 100644
> --- a/hw/cpu/arm11mpcore.c
> +++ b/hw/cpu/arm11mpcore.c
> @@ -15,6 +15,7 @@
>  #include "hw/irq.h"
>  #include "hw/qdev-properties.h"
>
> +#define ARM11MPCORE_NUM_GIC_PRIORITY_BITS    4
>
>  static void mpcore_priv_set_irq(void *opaque, int irq, int level)
>  {
> @@ -86,6 +87,10 @@ static void mpcore_priv_realize(DeviceState *dev, Error **errp)
>
>      qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
>      qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
> +    qdev_prop_set_uint32(gicdev, "num-priority-bits",
> +                         ARM11MPCORE_NUM_GIC_PRIORITY_BITS);
> +
> +
>      object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
>      if (err != NULL) {
>          error_propagate(errp, err);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM